Re: [PATCH 3/5] vhost/vsock: Allocate nonlinear SKBs for handling large receive buffers

[Will Deacon <will@kernel.org>]

On Tue, Jul 01, 2025 at 12:44:58PM +0200, Stefano Garzarella wrote:
> On Mon, Jun 30, 2025 at 03:20:57PM +0100, Will Deacon wrote:
> > On Fri, Jun 27, 2025 at 12:45:45PM +0200, Stefano Garzarella wrote:
> > > On Wed, Jun 25, 2025 at 02:15:41PM +0100, Will Deacon wrote:
> > > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > > > index 66a0f060770e..cfa4e1bcf367 100644
> > > > --- a/drivers/vhost/vsock.c
> > > > +++ b/drivers/vhost/vsock.c
> > > > @@ -344,11 +344,16 @@ vhost_vsock_alloc_skb(struct vhost_virtqueue *vq,
> > > >
> > > > 	len = iov_length(vq->iov, out);
> > > >
> > > > -	if (len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE + VIRTIO_VSOCK_SKB_HEADROOM)
> > > > +	if (len < VIRTIO_VSOCK_SKB_HEADROOM ||
> > > 
> > > Why moving this check here?
> > 
> > I moved it here because virtio_vsock_alloc_skb_with_frags() does:
> > 
> > +       size -= VIRTIO_VSOCK_SKB_HEADROOM;
> > +       return __virtio_vsock_alloc_skb_with_frags(VIRTIO_VSOCK_SKB_HEADROOM,
> > +                                                  size, mask);
> > 
> > and so having the check in __virtio_vsock_alloc_skb_with_frags() looks
> > strange as, by then, it really only applies to the linear case. It also
> > feels weird to me to have the upper-bound of the length checked by the
> > caller but the lower-bound checked in the callee. I certainly find it
> > easier to reason about if they're in the same place.
> > 
> > Additionally, the lower-bound check is only needed by the vhost receive
> > code, as the transmit path uses virtio_vsock_alloc_skb(), which never
> > passes a size smaller than VIRTIO_VSOCK_SKB_HEADROOM.
> > 
> > Given all that, moving it to the one place that needs it seemed like the
> > best option. What do you think?
> 
> Okay, I see now. Yep, it's fine, but please mention in the commit
> description.

Great, I'll do that.

> > > > 	len = le32_to_cpu(virtio_vsock_hdr(skb)->len);
> > > >
> > > > -	if (len > 0)
> > > 
> > > Why removing this check?
> > 
> > I think it's redundant: len is a u32, so we're basically just checking
> > to see if it's non-zero. All the callers have already checked for this
> > but, even if they didn't, skb_put(skb, 0) is harmless afaict.
> 
> Yep, I see, but now I don't remember why we have it, could it be more
> expensive to call `skb_put(skb, 0)`, instead of just having the if for
> control packets with no payload?

That sounds like a questionable optimisation, but I can preserve it in
the only caller that doesn't already check for a non-zero size
(virtio_transport_rx_work()). I mistakenly thought that it was already
checking it, but on closer inspection it only checks the size of the
virtqueue buffer and doesn't look at the packet header at all.

In fact, that is itself a bug because nothing prevents an SKB overflow
on the put path...

I'll add an extra fix for that in v2 so that it can be backported
independently.

Will