Patch 1 removes the unused 'wait' variable.
Patch 2 fixes an infinite sleep issue reported by a hv_sock user.
Made v2 to address Stefano's comments.
Please see each patch's header for changes in v2.
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 | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
--
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 queue before checking
vsock_connectible_has_data().
Fixes: b3f7fd54881b ("af_vsock: separate wait data loop")
Signed-off-by: Dexuan Cui <[email protected]>
---
Changes in v2 (Thanks Stefano!):
Fixed a typo in the commit message.
Removed the unnecessary finish_wait() at the end of the loop.
net/vmw_vsock/af_vsock.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index d258fd43092e..884eca7f6743 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) ||
--
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]>
Reviewed-by: Stefano Garzarella <[email protected]>
---
Changes in v2:
Added Stefano's Reviewed-by.
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
On Wed, Nov 02, 2022 at 10:31:37AM +0100, Stefano Garzarella wrote:
>On Mon, Oct 31, 2022 at 07:17:06PM -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 queue before checking
>>vsock_connectible_has_data().
>>
>>Fixes: b3f7fd54881b ("af_vsock: separate wait data loop")
>>Signed-off-by: Dexuan Cui <[email protected]>
>>---
>>
>>Changes in v2 (Thanks Stefano!):
>> Fixed a typo in the commit message.
>> Removed the unnecessary finish_wait() at the end of the loop.
>
>LGTM:
>
>Reviewed-by: Stefano Garzarella <[email protected]>
>
And I would add
Reported-by: Fr?d?ric Dalleau <[email protected]>
Since Fr?d?ric posted a similar patch some months ago (I lost it because
netdev and I were not in cc):
https://lore.kernel.org/virtualization/[email protected]/
Thanks,
Stefano
On Mon, Oct 31, 2022 at 07:17:06PM -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 queue before checking
>vsock_connectible_has_data().
>
>Fixes: b3f7fd54881b ("af_vsock: separate wait data loop")
>Signed-off-by: Dexuan Cui <[email protected]>
>---
>
>Changes in v2 (Thanks Stefano!):
> Fixed a typo in the commit message.
> Removed the unnecessary finish_wait() at the end of the loop.
LGTM:
Reviewed-by: Stefano Garzarella <[email protected]>
>
> net/vmw_vsock/af_vsock.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index d258fd43092e..884eca7f6743 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) ||
>--
>2.25.1
>
Hi Dexuan, Stefano,
Tested-by: Frédéric Dalleau <[email protected]>
Regards,
Frédéric
On Wed, Nov 2, 2022 at 10:42 AM Stefano Garzarella <[email protected]> wrote:
>
> On Wed, Nov 02, 2022 at 10:31:37AM +0100, Stefano Garzarella wrote:
> >On Mon, Oct 31, 2022 at 07:17:06PM -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 queue before checking
> >>vsock_connectible_has_data().
> >>
> >>Fixes: b3f7fd54881b ("af_vsock: separate wait data loop")
> >>Signed-off-by: Dexuan Cui <[email protected]>
> >>---
> >>
> >>Changes in v2 (Thanks Stefano!):
> >> Fixed a typo in the commit message.
> >> Removed the unnecessary finish_wait() at the end of the loop.
> >
> >LGTM:
> >
> >Reviewed-by: Stefano Garzarella <[email protected]>
> >
>
> And I would add
>
> Reported-by: Frédéric Dalleau <[email protected]>
>
> Since Frédéric posted a similar patch some months ago (I lost it because
> netdev and I were not in cc):
> https://lore.kernel.org/virtualization/[email protected]/
>
> Thanks,
> Stefano
>
> From: Frederic Dalleau <[email protected]>
> Sent: Wednesday, November 2, 2022 6:31 AM
> To: Stefano Garzarella <[email protected]>
> ...
> Hi Dexuan, Stefano,
>
> Tested-by: Fr?d?ric Dalleau <[email protected]>
>
> Regards,
> Fr?d?ric
Thank you, Frederic! I didn't realize you posted a similar patch in Aug :-)
Thanks Stefano for reviewing!
Thanks,
-- Dexuan
Hello:
This series was applied to netdev/net.git (master)
by Paolo Abeni <[email protected]>:
On Mon, 31 Oct 2022 19:17:04 -0700 you wrote:
> Patch 1 removes the unused 'wait' variable.
> Patch 2 fixes an infinite sleep issue reported by a hv_sock user.
>
> Made v2 to address Stefano's comments.
> Please see each patch's header for changes in v2.
>
> Dexuan Cui (2):
> vsock: remove the unused 'wait' in vsock_connectible_recvmsg()
> vsock: fix possible infinite sleep in vsock_connectible_wait_data()
>
> [...]
Here is the summary with links:
- [v2,1/2] vsock: remove the unused 'wait' in vsock_connectible_recvmsg()
https://git.kernel.org/netdev/net/c/cf6ff0df0fd1
- [v2,2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data()
https://git.kernel.org/netdev/net/c/466a85336fee
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html