Re: [PATCH net v3] virtio/vsock: fix leaks due to missing skb owner

[Stefano Garzarella]

On Wed, Mar 29, 2023 at 04:51:58PM +0000, Bobby Eshleman wrote:
> This patch sets the skb owner in the recv and send path for virtio.
>
> For the send path, this solves the leak caused when
> virtio_transport_purge_skbs() finds skb->sk is always NULL and therefore
> never matches it with the current socket. Setting the owner upon
> allocation fixes this.
>
> For the recv path, this ensures correctness of accounting and also
> correct transfer of ownership in vsock_loopback (when skbs are sent from
> one socket and received by another).
>
> Fixes: 71dc9ec9ac7d ("virtio/vsock: replace virtio_vsock_pkt with sk_buff")
> Signed-off-by: Bobby Eshleman <bobby.eshleman@xxxxxxxxxxxxx>
> Reported-by: Cong Wang <xiyou.wangcong@xxxxxxxxx>
> Link: https://lore.kernel.org/all/ZCCbATwov4U+GBUv@pop-os.localdomain/
> ---
> Changes in v3:
> - virtio/vsock: use skb_set_owner_sk_safe() instead of
>  skb_set_owner_{r,w}
> - virtio/vsock: reject allocating/receiving skb if sk_refcnt==0 and WARN_ONCE
> - Link to v2: https://lore.kernel.org/r/20230327-vsock-fix-leak-v2-1-f6619972dee0@xxxxxxxxxxxxx
>
> Changes in v2:
> - virtio/vsock: add skb_set_owner_r to recv_pkt()
> - Link to v1: https://lore.kernel.org/r/20230327-vsock-fix-leak-v1-1-3fede367105f@xxxxxxxxxxxxx
> ---
> net/vmw_vsock/virtio_transport_common.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 957cdc01c8e8..c927dc302faa 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -94,6 +94,11 @@ virtio_transport_alloc_skb(struct virtio_vsock_pkt_info *info,
> 					 info->op,
> 					 info->flags);
>
> +	if (info->vsk && !skb_set_owner_sk_safe(skb, sk_vsock(info->vsk))) {
> +		WARN_ONCE(1, "failed to allocate skb on vsock socket with sk_refcnt == 0\n");
> +		goto out;
> +	}
> +
> 	return skb;
>
> out:
> @@ -1294,6 +1299,11 @@ void virtio_transport_recv_pkt(struct virtio_transport *t,
> 		goto free_pkt;
> 	}
>
> +	if (!skb_set_owner_sk_safe(skb, sk)) {
> +		WARN_ONCE(1, "receiving vsock socket has sk_refcnt == 0\n");
> +		goto free_pkt;
> +	}
> +

LGTM!

I would have put the condition inside WARN_ONCE() because I find it
more readable (e.g. WARN_ONCE(!skb_set_owner_sk_safe(skb, sk), ...),
but I don't have a strong opinion on that, so that's fine too:

Reviewed-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>

Thanks,
Stefano