Re: [dpdk-dev] [PATCH] vhost: fix race condition in fdset_add

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 12/14/18 10:51 AM, Maxime Coquelin wrote:
> 
> 
> On 12/14/18 10:32 AM, Matthias Gatto wrote:
>> On Tue, Dec 11, 2018 at 7:11 PM Maxime Coquelin
>> <maxime.coquelin@redhat.com> wrote:
>>>
>>> Hi Matthias,
>>>
>>> On 12/6/18 5:00 PM, Matthias Gatto wrote:
>>>> fdset_add can call fdset_shrink_nolock which call fdset_move
>>>> concurrently to poll that is call in fdset_event_dispatch.
>>>>
>>>> This patch add a mutex to protect poll from been call at the same time
>>>> fdset_add call fdset_shrink_nolock.
>>>>
>>>> Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>
>>>> ---
>>>>    lib/librte_vhost/fd_man.c | 4 ++++
>>>>    lib/librte_vhost/fd_man.h | 1 +
>>>>    lib/librte_vhost/socket.c | 1 +
>>>>    3 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
>>>> index 38347ab..55d4856 100644
>>>> --- a/lib/librte_vhost/fd_man.c
>>>> +++ b/lib/librte_vhost/fd_man.c
>>>> @@ -129,7 +129,9 @@
>>>>        pthread_mutex_lock(&pfdset->fd_mutex);
>>>>        i = pfdset->num < MAX_FDS ? pfdset->num++ : -1;
>>>>        if (i == -1) {
>>>> +             pthread_mutex_lock(&pfdset->fd_pooling_mutex);
>>>>                fdset_shrink_nolock(pfdset);
>>>> +             pthread_mutex_unlock(&pfdset->fd_pooling_mutex);
>>>>                i = pfdset->num < MAX_FDS ? pfdset->num++ : -1;
>>>>                if (i == -1) {
>>>>                        pthread_mutex_unlock(&pfdset->fd_mutex);
>>>> @@ -246,7 +248,9 @@
>>>>                numfds = pfdset->num;
>>>>                pthread_mutex_unlock(&pfdset->fd_mutex);
>>>>
>>>> +             pthread_mutex_lock(&pfdset->fd_pooling_mutex);
>>>>                val = poll(pfdset->rwfds, numfds, 1000 /* millisecs */);
>>>> +             pthread_mutex_unlock(&pfdset->fd_pooling_mutex);
>>>
>>> Any reason we cannot use the existing fd_mutex?
>>
>> yes, using the existing fd_mutex would block fdset_add during the 
>> polling in
>> fdset_event_dispatch.
>>
>> here fd_pooling_mutex block only fdset_shrink_nolock inside
>> fdset_add which happen only in very rare occasions.
> 
> 
> Thanks for the clarification:
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

I guess we need to cc: stable, can you help with specifying which
commit it fixes?

Thanks in advance,
Maxime

> Maxime