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
Remove the unused variable introduced by 19c1b90e1979.
Fixes: 19c1b90e1979 ("af_vsock: separate receive data loop")
Signed-off-by: Dexuan Cui <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
>---
> net/vmw_vsock/af_vsock.c | 2 --
> 1 file changed, 2 deletions(-)
Good catch!
Reviewed-by: Stefano Garzarella <[email protected]>
>
>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
>
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 <[email protected]>
>---
> 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 <[email protected]>
> 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.