1. Patchew
  2. linux
  3. View series

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

Edward Adam Davis posted 1 patch 2 weeks, 2 days ago
There is a newer version of this series
net/core/datagram.c | 9 ++++++++-
net/core/skbuff.c   | 1 -
2 files changed, 8 insertions(+), 2 deletions(-)
[PATCH net-next V3] net: restore the iterator to its original state when an error occurs
Posted by Edward Adam Davis 2 weeks, 2 days ago 
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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=28e5f3d207b14bae122a
Signed-off-by: Edward Adam Davis <eadavis@qq.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/

 net/core/datagram.c | 9 ++++++++-
 net/core/skbuff.c   | 1 -
 2 files changed, 8 insertions(+), 2 deletions(-)

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;
 	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))
+		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;
-- 
2.43.0
Re: [PATCH net-next V3] net: restore the iterator to its original state when an error occurs
Posted by Jakub Kicinski 1 week, 2 days ago 
CC: Will, as this looks like ZC side of 7fb1291257ea1e27

On Thu,  4 Dec 2025 00:03:39 +0800 Edward Adam Davis 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=28e5f3d207b14bae122a
> Signed-off-by: Edward Adam Davis <eadavis@qq.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.

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

>  	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?

> +		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;
-- 
pw-bot: cr
Re: [PATCH net-next V3] net: restore the iterator to its original state when an error occurs
Posted by Edward Adam Davis 1 week, 2 days ago 
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@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=28e5f3d207b14bae122a
> > Signed-off-by: Edward Adam Davis <eadavis@qq.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;
Re: [PATCH net-next V3] net: restore the iterator to its original state when an error occurs
Posted by Jakub Kicinski 1 week, 1 day ago 
On Wed, 10 Dec 2025 18:03:14 +0800 Edward Adam Davis wrote:
> > 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.

I said callers. You're changing behavior of a function, is it going 
to break any of the callers.

> The test results [T2] for this version of the patch do not show any
> failures related to zerocopy.
Re: [PATCH net-next V3] net: restore the iterator to its original state when an error occurs
Posted by Edward Adam Davis 1 week, 1 day ago 
On Thu, 11 Dec 2025 14:21:42 +0900, Jakub Kicinski wrote:
> > > 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.
> 
> I said callers. You're changing behavior of a function, is it going
> to break any of the callers.
I investigated the relevant callers and found no similar restore/revert
operations. Furthermore, if such a restore action existed, the ZC
related test programs would have detected it, just as the tests
detected the revert operation in skb_zerocopy_iter_stream().

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.