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

[Matthias Gatto <matthias.gatto@outscale.com>]

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.

Thanks for the review,

Matthias
> > Maxime