On Sat, Aug 16, 2025 at 06:34:29AM -0400, Michael S. Tsirkin wrote:
> On Fri, Aug 15, 2025 at 04:48:00PM +0100, Will Deacon wrote:
> > On Fri, Aug 15, 2025 at 01:00:59PM +0100, Will Deacon wrote:
> > > On Fri, Aug 15, 2025 at 06:44:47AM -0400, Michael S. Tsirkin wrote:
> > > > On Fri, Aug 15, 2025 at 11:09:24AM +0100, Will Deacon wrote:
> > > > > On Tue, Aug 12, 2025 at 06:15:46AM -0400, Michael S. Tsirkin wrote:
> > > > > > On Tue, Aug 12, 2025 at 03:03:02AM -0700, syzbot wrote:
> > > > > > > Hello,
> > > > > > >
> > > > > > > syzbot has tested the proposed patch but the reproducer is still
> > > > > > > triggering an issue:
> > > > > > > WARNING in virtio_transport_send_pkt_info
> > > > > >
> > > > > > OK so the issue triggers on
> > > > > > commit 6693731487a8145a9b039bc983d77edc47693855
> > > > > > Author: Will Deacon <w...@kernel.org>
> > > > > > Date: Thu Jul 17 10:01:16 2025 +0100
> > > > > >
> > > > > > vsock/virtio: Allocate nonlinear SKBs for handling large
> > > > > > transmit buffers
> > > > > >
> > > > > >
> > > > > > but does not trigger on:
> > > > > >
> > > > > > commit 8ca76151d2c8219edea82f1925a2a25907ff6a9d
> > > > > > Author: Will Deacon <w...@kernel.org>
> > > > > > Date: Thu Jul 17 10:01:15 2025 +0100
> > > > > >
> > > > > > vsock/virtio: Rename virtio_vsock_skb_rx_put()
> > > > > >
> > > > > >
> > > > > >
> > > > > > Will, I suspect your patch merely uncovers a latent bug
> > > > > > in zero copy handling elsewhere.
> > >
> > > I'm still looking at this, but I'm not sure zero-copy is the right place
> > > to focus on.
> > >
> > > The bisected patch 6693731487a8 ("vsock/virtio: Allocate nonlinear SKBs
> > > for handling large transmit buffers") only has two hunks. The first is
> > > for the non-zcopy case and the latter is a no-op for zcopy, as
> > > skb_len == VIRTIO_VSOCK_SKB_HEADROOM and so we end up with a linear SKB
> > > regardless.
> >
> > It's looking like this is caused by moving from memcpy_from_msg() to
> > skb_copy_datagram_from_iter(), which is necessary to handle non-linear
> > SKBs correctly.
> >
> > In the case of failure (i.e. faulting on the source and returning
> > -EFAULT), memcpy_from_msg() rewinds the message iterator whereas
> > skb_copy_datagram_from_iter() does not. If we have previously managed to
> > transmit some of the packet, then I think
> > virtio_transport_send_pkt_info() can end up returning a positive "bytes
> > written" error code and the caller will call it again. If we've advanced
> > the message iterator, then this can end up with the reported warning if
> > we run out of input data.
> >
> > As a hack (see below), I tried rewinding the iterator in the error path
> > of skb_copy_datagram_from_iter() but I'm not sure whether other callers
> > would be happy with that. If not, then we could save/restore the
> > iterator state in virtio_transport_fill_skb() if the copy fails. Or we
> > could add a variant of skb_copy_datagram_from_iter(), say
> > skb_copy_datagram_from_iter_full(), which has the rewind behaviour.
> >
> > What do you think?
> >
> > Will
>
> It is, at least, self-contained. I don't much like hacking around
> it in virtio_transport_fill_skb. If your patch isn't acceptable,
> skb_copy_datagram_from_iter_full seem like a better approach, I think.
Thanks. I'll send something out shortly with you on cc.
Will
