linux-kernel - Re: [PATCH net-next V3] net: restore the iterator to its original state when an error occurs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <tencent_000364896CA3A544481764D76486C0336005@qq.com>
Date: Wed, 10 Dec 2025 18:03:14 +0800
From: Edward Adam Davis <eadavis@...com>
To: kuba@...nel.org
Cc: davem@...emloft.net,
	eadavis@...com,
	edumazet@...gle.com,
	eperezma@...hat.com,
	horms@...nel.org,
	jasowang@...hat.com,
	kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	mst@...hat.com,
	netdev@...r.kernel.org,
	pabeni@...hat.com,
	sgarzare@...hat.com,
	stefanha@...hat.com,
	syzbot+ci3edb9412aeb2e703@...kaller.appspotmail.com,
	syzbot@...ts.linux.dev,
	syzbot@...kaller.appspotmail.com,
	syzkaller-bugs@...glegroups.com,
	virtualization@...ts.linux.dev,
	xuanzhuo@...ux.alibaba.com
Subject: Re: [PATCH net-next V3] net: restore the iterator to its original state when an error occurs

On Wed, 10 Dec 2025 17:31:25 +0900, Jakub Kicinski wrote:
> > In zerocopy_fill_skb_from_iter(), if two copy operations are performed
> > and the first one succeeds while the second one fails, it returns a
> > failure but the count in iterator has already been decremented due to
> > the first successful copy. This ultimately affects the local variable
> > rest_len in virtio_transport_send_pkt_info(), causing the remaining
> > count in rest_len to be greater than the actual iterator count. As a
> > result, packet sending operations continue even when the iterator count
> > is zero, which further leads to skb->len being 0 and triggers the warning
> > reported by syzbot [1].
> >
> > Therefore, if the zerocopy operation fails, we should revert the iterator
> > to its original state.
> >
> > The iov_iter_revert() in skb_zerocopy_iter_stream() is no longer needed
> > and has been removed.
> >
> > [1]
> > 'send_pkt()' returns 0, but 4096 expected
> > WARNING: net/vmw_vsock/virtio_transport_common.c:430 at virtio_transport_send_pkt_info+0xd1e/0xef0 net/vmw_vsock/virtio_transport_common.c:428, CPU#1: syz.0.17/5986
> > Call Trace:
> >  virtio_transport_stream_enqueue net/vmw_vsock/virtio_transport_common.c:1113 [inline]
> >  virtio_transport_seqpacket_enqueue+0x143/0x1c0 net/vmw_vsock/virtio_transport_common.c:841
> >  vsock_connectible_sendmsg+0xabf/0x1040 net/vmw_vsock/af_vsock.c:2158
> >  sock_sendmsg_nosec net/socket.c:727 [inline]
> >  __sock_sendmsg+0x21c/0x270 net/socket.c:746
> >
> > Reported-by: syzbot+28e5f3d207b14bae122a@...kaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=28e5f3d207b14bae122a
> > Signed-off-by: Edward Adam Davis <eadavis@...com>
> > ---
> > v3:
> >   - fix test tcp_zerocopy_maxfrags timeout
> > v2: https://lore.kernel.org/all/tencent_BA768766163C533724966E36344AAE754709@qq.com/
> >   - Remove iov_iter_revert() in skb_zerocopy_iter_stream()
> > v1: https://lore.kernel.org/all/tencent_387517772566B03DBD365896C036264AA809@qq.com/
> 
> Have you investigated the other callers? Given problems with previous
> version of this patch I'm worried you have not. If you did please extend
> the commit message with the appropriate explanation.
Are you asking if I investigated other zerocopy tests? NO.
The test results [T2] for this version of the patch do not show any
failures related to zerocopy.

[T2] https://patchwork.kernel.org/project/netdevbpf/patch/tencent_3C86DFD37A0374496263BE24483777D76305@qq.com
> 
> Alternative would be to add a _full() flavor of this API, but not sure
> if other callers actually care.
> 
> > diff --git a/net/core/datagram.c b/net/core/datagram.c
> > index c285c6465923..3a612ebbbe80 100644
> > --- a/net/core/datagram.c
> > +++ b/net/core/datagram.c
> > @@ -748,9 +748,13 @@ int __zerocopy_sg_from_iter(struct msghdr *msg, struct sock *sk,
> >  			    size_t length,
> >  			    struct net_devmem_dmabuf_binding *binding)
> >  {
> > +	struct iov_iter_state state;
> 
> nit: if you respin move this one line down
OK.
> 
> >  	unsigned long orig_size = skb->truesize;
> >  	unsigned long truesize;
> > -	int ret;
> > +	int ret, orig_len;
> > +
> > +	iov_iter_save_state(from, &state);
> > +	orig_len = skb->len;
> >
> >  	if (msg && msg->msg_ubuf && msg->sg_from_iter)
> >  		ret = msg->sg_from_iter(skb, from, length);
> > @@ -759,6 +763,9 @@ int __zerocopy_sg_from_iter(struct msghdr *msg, struct sock *sk,
> >  	else
> >  		ret = zerocopy_fill_skb_from_iter(skb, from, length);
> >
> > +	if (ret == -EFAULT || (ret == -EMSGSIZE && skb->len == orig_len))
> 
> I'd think that for the purpose of reverting iter the second part of
> this condition is completely moot. If skb len didn't change there should
> be nothing to revert?
Regarding the second judgment condition, I aligned it with the condition
in skb_zerocopy_iter_stream(). During my testing, I only encountered
-EFAULT failures, not -EMSGSIZE.
> 
> > +		iov_iter_restore(from, &state);
> > +
> >  	truesize = skb->truesize - orig_size;
> >  	if (sk && sk->sk_type == SOCK_STREAM) {
> >  		sk_wmem_queued_add(sk, truesize);
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index a00808f7be6a..7b8836f668b7 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -1908,7 +1908,6 @@ int skb_zerocopy_iter_stream(struct sock *sk, struct sk_buff *skb,
> >  		struct sock *save_sk = skb->sk;
> >
> >  		/* Streams do not free skb on error. Reset to prev state. */
> > -		iov_iter_revert(&msg->msg_iter, skb->len - orig_len);
> >  		skb->sk = sk;
> >  		___pskb_trim(skb, orig_len);
> >  		skb->sk = save_sk;