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

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

On 12/14/18 11:07 AM, Matthias Gatto wrote:
> On Fri, Dec 14, 2018 at 10:53 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>>
>>
>> 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
>>
> 
> this commit 1b815b89599cdd9b54e5aa70f5b97088225b2bcc
> which was actually a commit I've made, sorry for that.

Don't be sorry, your contributions are welcome!
I'll fixup the commit message with adding Fixes line.

Thanks,
Maxime

> Thanks for the review,
> 
> Matthias
>>> Maxime