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

[Maxime Coquelin]

On 12/14/18 11:07 AM, Matthias Gatto wrote:

On Fri, Dec 14, 2018 at 10:53 AM Maxime Coquelin
<maxime.coque...@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.coque...@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.ga...@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.coque...@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