Stefano Garzarella
2023-Mar-30 08:00 UTC
[PATCH net v3] virtio/vsock: fix leaks due to missing skb owner
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 at bytedance.com> >Reported-by: Cong Wang <xiyou.wangcong at gmail.com> >Link: https://lore.kernel.org/all/ZCCbATwov4U+GBUv at 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 at bytedance.com > >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 at bytedance.com >--- > 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 at redhat.com> Thanks, Stefano