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

login
register
mail settings
Submitter Matthias Gatto
Date Dec. 6, 2018, 4 p.m.
Message ID <1544112007-23177-1-git-send-email-matthias.gatto@outscale.com>
Download mbox | patch
Permalink /patch/674317/
State New
Headers show

Comments

Matthias Gatto - Dec. 6, 2018, 4 p.m.
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(+)
Maxime Coquelin - Dec. 11, 2018, 6:11 p.m.
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?

>   		if (val < 0)
>   			continue;
>   
> diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
> index 3331bcd..3ab5cfd 100644
> --- a/lib/librte_vhost/fd_man.h
> +++ b/lib/librte_vhost/fd_man.h
> @@ -24,6 +24,7 @@ struct fdset {
>   	struct pollfd rwfds[MAX_FDS];
>   	struct fdentry fd[MAX_FDS];
>   	pthread_mutex_t fd_mutex;
> +	pthread_mutex_t fd_pooling_mutex;
>   	int num;	/* current fd number of this fdset */
>   
>   	union pipefds {
> diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> index d630317..cc4e748 100644
> --- a/lib/librte_vhost/socket.c
> +++ b/lib/librte_vhost/socket.c
> @@ -88,6 +88,7 @@ struct vhost_user {
>   	.fdset = {
>   		.fd = { [0 ... MAX_FDS - 1] = {-1, NULL, NULL, NULL, 0} },
>   		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
> +		.fd_pooling_mutex = PTHREAD_MUTEX_INITIALIZER,
>   		.num = 0
>   	},
>   	.vsocket_cnt = 0,
>
Matthias Gatto - Dec. 14, 2018, 9:32 a.m.
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.

> >               if (val < 0)
> >                       continue;
> >
> > diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
> > index 3331bcd..3ab5cfd 100644
> > --- a/lib/librte_vhost/fd_man.h
> > +++ b/lib/librte_vhost/fd_man.h
> > @@ -24,6 +24,7 @@ struct fdset {
> >       struct pollfd rwfds[MAX_FDS];
> >       struct fdentry fd[MAX_FDS];
> >       pthread_mutex_t fd_mutex;
> > +     pthread_mutex_t fd_pooling_mutex;
> >       int num;        /* current fd number of this fdset */
> >
> >       union pipefds {
> > diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> > index d630317..cc4e748 100644
> > --- a/lib/librte_vhost/socket.c
> > +++ b/lib/librte_vhost/socket.c
> > @@ -88,6 +88,7 @@ struct vhost_user {
> >       .fdset = {
> >               .fd = { [0 ... MAX_FDS - 1] = {-1, NULL, NULL, NULL, 0} },
> >               .fd_mutex = PTHREAD_MUTEX_INITIALIZER,
> > +             .fd_pooling_mutex = PTHREAD_MUTEX_INITIALIZER,
> >               .num = 0
> >       },
> >       .vsocket_cnt = 0,
> >
Maxime Coquelin - Dec. 14, 2018, 9:51 a.m.
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>

Maxime
Maxime Coquelin - Dec. 14, 2018, 9:53 a.m.
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
Matthias Gatto - Dec. 14, 2018, 10:07 a.m.
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
Maxime Coquelin - Dec. 14, 2018, 10:08 a.m.
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
Maxime Coquelin - Dec. 18, 2018, 2:01 p.m.
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(+)


Applied to dpdk-next-virtio

Thanks,
Maxime

Patch

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);
 		if (val < 0)
 			continue;
 
diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
index 3331bcd..3ab5cfd 100644
--- a/lib/librte_vhost/fd_man.h
+++ b/lib/librte_vhost/fd_man.h
@@ -24,6 +24,7 @@  struct fdset {
 	struct pollfd rwfds[MAX_FDS];
 	struct fdentry fd[MAX_FDS];
 	pthread_mutex_t fd_mutex;
+	pthread_mutex_t fd_pooling_mutex;
 	int num;	/* current fd number of this fdset */
 
 	union pipefds {
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index d630317..cc4e748 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -88,6 +88,7 @@  struct vhost_user {
 	.fdset = {
 		.fd = { [0 ... MAX_FDS - 1] = {-1, NULL, NULL, NULL, 0} },
 		.fd_mutex = PTHREAD_MUTEX_INITIALIZER,
+		.fd_pooling_mutex = PTHREAD_MUTEX_INITIALIZER,
 		.num = 0
 	},
 	.vsocket_cnt = 0,