Re: [syzbot] [kvm?] [net?] [virt?] WARNING in virtio_transport_send_pkt_info

[Hillf Danton <hdanton@sina.com>]

On Fri, 15 Aug 2025 16:48:00 +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 <will@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 <will@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
>
>--->8

#syz test

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);
--