Virtualization - Nov 2022 - [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data()

Hi Dexan, Stephano,

This solution has been proposed here,
https://lists.linuxfoundation.org/pipermail/virtualization/2022-August/062656.html

Best regards,
Fr?d?ric

On Mon, Oct 31, 2022 at 9:43 AM Stefano Garzarella <sgarzare at
redhat.com>
wrote:
> On Fri, Oct 28, 2022 at 01:56:46PM -0700, Dexuan Cui wrote:
> >Currently vsock_connectible_has_data() may miss a wakeup operation
> >between vsock_connectible_has_data() == 0 and the prepare_to_wait().
> >
> >Fix the race by adding the process to the wait qeuue before checking
>
> s/qeuue/queue
>
> >vsock_connectible_has_data().
> >
> >Fixes: b3f7fd54881b ("af_vsock: separate wait data loop")
> >Signed-off-by: Dexuan Cui <decui at microsoft.com>
> >---
> > net/vmw_vsock/af_vsock.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> >index d258fd43092e..03a6b5bc6ba7 100644
> >--- a/net/vmw_vsock/af_vsock.c
> >+++ b/net/vmw_vsock/af_vsock.c
> >@@ -1905,8 +1905,11 @@ static int vsock_connectible_wait_data(struct
sock
> *sk,
> >       err = 0;
> >       transport = vsk->transport;
> >
> >-      while ((data = vsock_connectible_has_data(vsk)) == 0) {
> >+      while (1) {
> >               prepare_to_wait(sk_sleep(sk), wait, TASK_INTERRUPTIBLE);
> >+              data = vsock_connectible_has_data(vsk);
> >+              if (data != 0)
> >+                      break;
> >
> >               if (sk->sk_err != 0 ||
> >                   (sk->sk_shutdown & RCV_SHUTDOWN) ||
> >@@ -1937,6 +1940,8 @@ static int vsock_connectible_wait_data(struct
sock
> *sk,
> >                       err = -EAGAIN;
> >                       break;
> >               }
> >+
> >+              finish_wait(sk_sleep(sk), wait);
>
> Since we are going to call again prepare_to_wait() on top of the loop,
> is finish_wait() call here really needed?
>
> What about following what we do in vsock_accept and vsock_connect?
>
>      prepare_to_wait()
>
>      while (condition) {
>          ...
>          prepare_to_wait();
>      }
>
>      finish_wait()
>
> I find it a little more readable, but your solution is fine too.
>
> Thanks,
> Stefano
>
> _______________________________________________
> Virtualization mailing list
> Virtualization at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20221101/0f45bb1f/attachment.html>

On Tue, Nov 01, 2022 at 09:21:06PM +0100, Frederic Dalleau via Virtualization
wrote:
>Hi Dexan, Stephano,
>
>This solution has been proposed here,
>https://lists.linuxfoundation.org/pipermail/virtualization/2022-August/062656.html
Ops, I missed it!

Did you use scripts/get_maintainer.pl?
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#select-the-recipients-for-your-patch

Since your patch should be reposted (hasn't been sent to 
netdev at vger.kernel.org, missing Fixes tag, etc.) and Dexuan's patch on 
the other hand is ready (I just reviewed it), can you test it and 
respond with your Tested-by?

I would like to give credit to both, so I asked to add your Reported-by 
to the Dexuan's patch.

Thanks,
Stefano