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@xxxxxxxxxxxxx>
> ---
> 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