| Message ID | 20221028205646.28084-3-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vsock: remove an unused variable and fix infinite sleep | expand |
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@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
> From: Stefano Garzarella <sgarzare@redhat.com> > Sent: Monday, October 31, 2022 1:43 AM > ... > s/qeuue/queue Will fix this. > >@@ -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? It's not needed. Will remove this and send v2. > 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 I'd like to stay with my version, as it only needs one line of prepare_to_wait(), and IMO it's more readable if we only exit from inside the while loop.
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@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
> Did you use scripts/get_maintainer.pl? Not really, I just picked the list that seemed narrow enough for the topic > respond with your Tested-by? Done > I would like to give credit to both, so I asked to add your Reported-by > to the Dexuan's patch. Thank you! Regards, Frédéric
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); } finish_wait(sk_sleep(sk), wait);
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 vsock_connectible_has_data(). Fixes: b3f7fd54881b ("af_vsock: separate wait data loop") Signed-off-by: Dexuan Cui <decui@microsoft.com> --- net/vmw_vsock/af_vsock.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)