* [PATCH 0/2] vsock: remove an unused variable and fix infinite sleep @ 2022-10-28 20:56 Dexuan Cui 2022-10-28 20:56 ` [PATCH 1/2] vsock: remove the unused 'wait' in vsock_connectible_recvmsg() Dexuan Cui 2022-10-28 20:56 ` [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data() Dexuan Cui 0 siblings, 2 replies; 6+ messages in thread From: Dexuan Cui @ 2022-10-28 20:56 UTC (permalink / raw) To: sgarzare, davem, edumazet, kuba, pabeni, arseny.krasnov, netdev Cc: virtualization, linux-kernel, kys, haiyangz, stephen, wei.liu, linux-hyperv, Dexuan Cui Patch 1 removes the unused 'wait' variable. Patch 2 fixes an infinite sleep issue reported by a hv_sock user. Dexuan Cui (2): vsock: remove the unused 'wait' in vsock_connectible_recvmsg() vsock: fix possible infinite sleep in vsock_connectible_wait_data() net/vmw_vsock/af_vsock.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] vsock: remove the unused 'wait' in vsock_connectible_recvmsg() 2022-10-28 20:56 [PATCH 0/2] vsock: remove an unused variable and fix infinite sleep Dexuan Cui @ 2022-10-28 20:56 ` Dexuan Cui 2022-10-31 8:37 ` Stefano Garzarella 2022-10-28 20:56 ` [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data() Dexuan Cui 1 sibling, 1 reply; 6+ messages in thread From: Dexuan Cui @ 2022-10-28 20:56 UTC (permalink / raw) To: sgarzare, davem, edumazet, kuba, pabeni, arseny.krasnov, netdev Cc: virtualization, linux-kernel, kys, haiyangz, stephen, wei.liu, linux-hyperv, Dexuan Cui Remove the unused variable introduced by 19c1b90e1979. Fixes: 19c1b90e1979 ("af_vsock: separate receive data loop") Signed-off-by: Dexuan Cui <decui@microsoft.com> --- net/vmw_vsock/af_vsock.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index ee418701cdee..d258fd43092e 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -2092,8 +2092,6 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, const struct vsock_transport *transport; int err; - DEFINE_WAIT(wait); - sk = sock->sk; vsk = vsock_sk(sk); err = 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vsock: remove the unused 'wait' in vsock_connectible_recvmsg() 2022-10-28 20:56 ` [PATCH 1/2] vsock: remove the unused 'wait' in vsock_connectible_recvmsg() Dexuan Cui @ 2022-10-31 8:37 ` Stefano Garzarella 0 siblings, 0 replies; 6+ messages in thread From: Stefano Garzarella @ 2022-10-31 8:37 UTC (permalink / raw) To: Dexuan Cui Cc: davem, edumazet, kuba, pabeni, arseny.krasnov, netdev, virtualization, linux-kernel, kys, haiyangz, stephen, wei.liu, linux-hyperv On Fri, Oct 28, 2022 at 01:56:45PM -0700, Dexuan Cui wrote: >Remove the unused variable introduced by 19c1b90e1979. > >Fixes: 19c1b90e1979 ("af_vsock: separate receive data loop") >Signed-off-by: Dexuan Cui <decui@microsoft.com> >--- > net/vmw_vsock/af_vsock.c | 2 -- > 1 file changed, 2 deletions(-) Good catch! Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >index ee418701cdee..d258fd43092e 100644 >--- a/net/vmw_vsock/af_vsock.c >+++ b/net/vmw_vsock/af_vsock.c >@@ -2092,8 +2092,6 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, > const struct vsock_transport *transport; > int err; > >- DEFINE_WAIT(wait); >- > sk = sock->sk; > vsk = vsock_sk(sk); > err = 0; >-- >2.25.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data() 2022-10-28 20:56 [PATCH 0/2] vsock: remove an unused variable and fix infinite sleep Dexuan Cui 2022-10-28 20:56 ` [PATCH 1/2] vsock: remove the unused 'wait' in vsock_connectible_recvmsg() Dexuan Cui @ 2022-10-28 20:56 ` Dexuan Cui 2022-10-31 8:43 ` Stefano Garzarella 1 sibling, 1 reply; 6+ messages in thread From: Dexuan Cui @ 2022-10-28 20:56 UTC (permalink / raw) To: sgarzare, davem, edumazet, kuba, pabeni, arseny.krasnov, netdev Cc: virtualization, linux-kernel, kys, haiyangz, stephen, wei.liu, linux-hyperv, Dexuan Cui 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(-) 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); -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data() 2022-10-28 20:56 ` [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data() Dexuan Cui @ 2022-10-31 8:43 ` Stefano Garzarella 2022-11-01 1:58 ` Dexuan Cui 0 siblings, 1 reply; 6+ messages in thread From: Stefano Garzarella @ 2022-10-31 8:43 UTC (permalink / raw) To: Dexuan Cui Cc: davem, edumazet, kuba, pabeni, arseny.krasnov, netdev, virtualization, linux-kernel, kys, haiyangz, stephen, wei.liu, linux-hyperv 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data() 2022-10-31 8:43 ` Stefano Garzarella @ 2022-11-01 1:58 ` Dexuan Cui 0 siblings, 0 replies; 6+ messages in thread From: Dexuan Cui @ 2022-11-01 1:58 UTC (permalink / raw) To: Stefano Garzarella Cc: davem, edumazet, kuba, pabeni, arseny.krasnov, netdev, virtualization, linux-kernel, KY Srinivasan, Haiyang Zhang, stephen, wei.liu, linux-hyperv > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-01 1:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-28 20:56 [PATCH 0/2] vsock: remove an unused variable and fix infinite sleep Dexuan Cui 2022-10-28 20:56 ` [PATCH 1/2] vsock: remove the unused 'wait' in vsock_connectible_recvmsg() Dexuan Cui 2022-10-31 8:37 ` Stefano Garzarella 2022-10-28 20:56 ` [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data() Dexuan Cui 2022-10-31 8:43 ` Stefano Garzarella 2022-11-01 1:58 ` Dexuan Cui
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).