| Message ID | 20221101021706.26152-3-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 466a85336fee6e3b35eb97b8405a28302fd25809 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vsock: remove an unused variable and fix infinite sleep | expand |
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 <decui@microsoft.com> >--- > >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 <sgarzare@redhat.com> > > 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 >
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 <decui@microsoft.com> >>--- >> >>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 <sgarzare@redhat.com> > And I would add Reported-by: Frédéric Dalleau <frederic.dalleau@docker.com> 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/20220824074251.2336997-2-frederic.dalleau@docker.com/ Thanks, Stefano
Hi Dexuan, Stefano, Tested-by: Frédéric Dalleau <frederic.dalleau@docker.com> Regards, Frédéric On Wed, Nov 2, 2022 at 10:42 AM Stefano Garzarella <sgarzare@redhat.com> 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 <decui@microsoft.com> > >>--- > >> > >>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 <sgarzare@redhat.com> > > > > And I would add > > Reported-by: Frédéric Dalleau <frederic.dalleau@docker.com> > > 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/20220824074251.2336997-2-frederic.dalleau@docker.com/ > > Thanks, > Stefano >
> From: Frederic Dalleau <frederic.dalleau@docker.com> > Sent: Wednesday, November 2, 2022 6:31 AM > To: Stefano Garzarella <sgarzare@redhat.com> > ... > Hi Dexuan, Stefano, > > Tested-by: Frédéric Dalleau <frederic.dalleau@docker.com> > > Regards, > Frédéric Thank you, Frederic! I didn't realize you posted a similar patch in Aug :-) Thanks Stefano for reviewing! Thanks, -- Dexuan
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) ||
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 <decui@microsoft.com> --- 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(-)