[PATCH 0/2] vsock: remove an unused variable and fix infinite sleep

[PATCH 0/2] vsock: remove an unused variable and fix infinite sleep

[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

[PATCH 1/2] vsock: remove the unused 'wait' in vsock_connectible_recvmsg()

[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

[PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data()

[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

Re: [PATCH 1/2] vsock: remove the unused 'wait' in vsock_connectible_recvmsg()

[Stefano Garzarella]

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
>

Re: [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data()

[Stefano Garzarella]

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

RE: [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data()

[Dexuan Cui]

> 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.

Re: [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data()

[Stefano Garzarella]

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

Re: [PATCH 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data()

[Frederic Dalleau]

> 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