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:

quoted

On Fri, Aug 15, 2025 at 11:09:24AM +0100, Will Deacon wrote:

quoted

On Tue, Aug 12, 2025 at 06:15:46AM -0400, Michael S. Tsirkin wrote:

quoted

On Tue, Aug 12, 2025 at 03:03:02AM -0700, syzbot wrote:

quoted

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 [off-list ref]
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 [off-list ref]
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

--->8

diff --git a/net/core/datagram.c b/net/core/datagram.c
index 94cc4705e91d..62e44ab136b7 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c

@@ -551,7 +551,7 @@ int skb_copy_datagram_from_iter(struct sk_buff *skb, int offset,
 				 int len)
 {
 	int start = skb_headlen(skb);
-	int i, copy = start - offset;
+	int i, copy = start - offset, start_off = offset;
 	struct sk_buff *frag_iter;
 
 	/* Copy header. */

@@ -614,6 +614,7 @@ int skb_copy_datagram_from_iter(struct sk_buff *skb, int offset,
 		return 0;
 
 fault:
+	iov_iter_revert(from, offset - start_off);
 	return -EFAULT;
 }
 EXPORT_SYMBOL(skb_copy_datagram_from_iter);