Virtualization - Nov 2022 - [PATCH v2 2/2] vsock: fix possible infinite sleep in vsock_connectible_wait_data()

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 at 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 at 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 at 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 at redhat.com>
>
And I would add

Reported-by: Fr?d?ric Dalleau <frederic.dalleau at 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
at docker.com/

Thanks,
Stefano