[PATCH net v3 1/2] net: core: propagate unreadable flag in skb_zerocopy

[PATCH net v3 1/2] net: core: propagate unreadable flag in skb_zerocopy

[Mina Almasry]

skb_zerocopy() fails to propagate the unreadable flag when copying
devmem fragments, causing target skbs to appear as readable memory.

This patch fixes the flag propagation. Additionally, it returns -EFAULT
if standard payload is mixed with unreadable devmem fragments during
extraction, and returns -EFAULT in openvswitch
queue_userspace_packet().

Fixes: 65249feb6b3d ("net: add support for skbs with unreadable frags")
Cc: Pavel Begunkov <asml.silence@gmail.com>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Bobby Eshleman <bobbyeshleman@gmail.com>
Cc: Florian Westphal <fw@strlen.de>
Cc: Aaron Conole <aconole@redhat.com>
Cc: Eelco Chaudron <echaudro@redhat.com>
Cc: Ilya Maximets <i.maximets@ovn.org>
Cc: Willem de Bruijn <willemb@google.com>
Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
Signed-off-by: Mina Almasry <almasrymina@google.com>
---
 net/core/skbuff.c          | 13 ++++++++++++-
 net/openvswitch/datapath.c |  3 +++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index ba3dbac80fb49..d21af68156950 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3870,7 +3870,8 @@ EXPORT_SYMBOL_GPL(skb_zerocopy_headlen);
  *	Return value:
  *	0: everything is OK
  *	-ENOMEM: couldn't orphan frags of @from due to lack of memory
- *	-EFAULT: skb_copy_bits() found some problem with skb geometry
+ *	-EFAULT: skb_copy_bits() found some problem with skb geometry, or readable head
+ *      payload would be mixed with unreadable frags.
  */
 int
 skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
@@ -3905,10 +3906,17 @@ skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
 		}
 	}
 
+	if (!skb_frags_readable(from) && j > 0 && len) {
+		put_page(virt_to_head_page(from->head));
+		return -EFAULT;
+	}
+
 	skb_len_add(to, len + plen);
 
 	if (unlikely(skb_orphan_frags(from, GFP_ATOMIC))) {
 		skb_tx_error(from);
+		if (j > 0)
+			put_page(virt_to_head_page(from->head));
 		return -ENOMEM;
 	}
 	skb_zerocopy_clone(to, from, GFP_ATOMIC);
@@ -3928,6 +3936,9 @@ skb_zerocopy(struct sk_buff *to, struct sk_buff *from, int len, int hlen)
 	}
 	skb_shinfo(to)->nr_frags = j;
 
+	if (i > 0 && from->unreadable)
+		to->unreadable = 1;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(skb_zerocopy);
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index ae69b2cabab9e..482893a5f67dc 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -467,6 +467,9 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
 	if (!dp_ifindex)
 		return -ENODEV;
 
+	if (!skb_frags_readable(skb))
+		return -EFAULT;
+
 	if (skb_vlan_tag_present(skb)) {
 		nskb = skb_clone(skb, GFP_ATOMIC);
 		if (!nskb)

base-commit: cba9ccb47e9fa4cc77692fb896cc5ab57a667882
-- 
2.55.0.679.g6767b8d81c-goog

[PATCH net v3 2/2] net: tcp: block standard payload injection into devmem skbs

[Mina Almasry]

Protect tcp_sendmsg_locked() from mistakenly appending non-zerocopy
page fragments to unreadable devmem skbs. Create a new segment instead.

Fixes: bd61848900bff ("net: devmem: Implement TX path")
Cc: Pavel Begunkov <asml.silence@gmail.com>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Bobby Eshleman <bobbyeshleman@gmail.com>
Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
Signed-off-by: Mina Almasry <almasrymina@google.com>
---
 net/ipv4/tcp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 455441f1b6949..186a36c698798 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1278,6 +1278,11 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
 		if (copy > msg_data_left(msg))
 			copy = msg_data_left(msg);
 
+		if (zc != MSG_ZEROCOPY && unlikely(!skb_frags_readable(skb))) {
+			tcp_mark_push(tp, skb);
+			goto new_segment;
+		}
+
 		if (zc == 0) {
 			bool merge = true;
 			int i = skb_shinfo(skb)->nr_frags;
-- 
2.55.0.679.g6767b8d81c-goog

Re: [PATCH net v3 2/2] net: tcp: block standard payload injection into devmem skbs

[Eric Dumazet]

On Tue, Aug 11, 2026 at 9:54 PM Mina Almasry <almasrymina@google.com> wrote:
>
> Protect tcp_sendmsg_locked() from mistakenly appending non-zerocopy
> page fragments to unreadable devmem skbs. Create a new segment instead.
>
> Fixes: bd61848900bff ("net: devmem: Implement TX path")
> Cc: Pavel Begunkov <asml.silence@gmail.com>
> Cc: Stanislav Fomichev <sdf@fomichev.me>
> Cc: Bobby Eshleman <bobbyeshleman@gmail.com>
> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
> Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> ---
>  net/ipv4/tcp.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 455441f1b6949..186a36c698798 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1278,6 +1278,11 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>                 if (copy > msg_data_left(msg))
>                         copy = msg_data_left(msg);
>
> +               if (zc != MSG_ZEROCOPY && unlikely(!skb_frags_readable(skb))) {

This seems wrong, as @binding could be NULL or not ?
Also testing the condition right after a fresh skb was allocated is
adding unecessary cost.

> +                       tcp_mark_push(tp, skb);
> +                       goto new_segment;
> +               }
> +
>                 if (zc == 0) {
>                         bool merge = true;
>                         int i = skb_shinfo(skb)->nr_frags;
> --
> 2.55.0.679.g6767b8d81c-goog
>

What about instead:

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 455441f1b694904172cfa1d8e7bac7076b60cb24..b4237d0e994d6f9d754d2167023e3981a40b58f4
100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1240,7 +1240,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct
msghdr *msg, size_t size)

                trace_tcp_sendmsg_locked(sk, msg, skb, size_goal);

-               if (copy <= 0 || !tcp_skb_can_collapse_to(skb)) {
+               if (copy <= 0 || !tcp_skb_can_collapse_to(skb) ||
+                   unlikely(skb_frags_readable(skb) != !binding)) {
                        bool first_skb;

 new_segment:

Re: [PATCH net v3 2/2] net: tcp: block standard payload injection into devmem skbs

[Pavel Begunkov]

On 8/12/26 13:01, Eric Dumazet wrote:
> On Tue, Aug 11, 2026 at 9:54 PM Mina Almasry <almasrymina@google.com> wrote:
>>
>> Protect tcp_sendmsg_locked() from mistakenly appending non-zerocopy
>> page fragments to unreadable devmem skbs. Create a new segment instead.
>>
>> Fixes: bd61848900bff ("net: devmem: Implement TX path")
>> Cc: Pavel Begunkov <asml.silence@gmail.com>
>> Cc: Stanislav Fomichev <sdf@fomichev.me>
>> Cc: Bobby Eshleman <bobbyeshleman@gmail.com>
>> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
>> Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
>> Signed-off-by: Mina Almasry <almasrymina@google.com>
>> ---
>>   net/ipv4/tcp.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>> index 455441f1b6949..186a36c698798 100644
>> --- a/net/ipv4/tcp.c
>> +++ b/net/ipv4/tcp.c
>> @@ -1278,6 +1278,11 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>>                  if (copy > msg_data_left(msg))
>>                          copy = msg_data_left(msg);
>>
>> +               if (zc != MSG_ZEROCOPY && unlikely(!skb_frags_readable(skb))) {
> 
> This seems wrong, as @binding could be NULL or not ?

I'd say it is *supposed* to be null as device memory without zero
copy doesn't make sense, but it looks like that can happen if there
is no NETIF_F_SG. How about rejecting it? It'd EFAULT somewhere in
skb_copy_to_page_nocache() anyway.

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 455441f1b694..f403830af65f 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1162,6 +1162,10 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
  					binding = NULL;
  					goto out_err;
  				}
+				if (zc != MSG_ZEROCOPY) {
+					err = -EOPNOTSUPP;
+					goto out_err;
+				}
  			}
  		}
  	} else if (unlikely(msg->msg_flags & MSG_SPLICE_PAGES) && size) {


> Also testing the condition right after a fresh skb was allocated is
> adding unecessary cost.

FWIW, we can even remove all extra overhead with a new SKBFL flag and
checking it together with likes of skb_zcopy_pure(), but IMHO it's
better to be done on top if needed.

-- 
Pavel Begunkov

Re: [PATCH net v3 2/2] net: tcp: block standard payload injection into devmem skbs

[Mina Almasry]

On Wed, Aug 12, 2026 at 5:01 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Aug 11, 2026 at 9:54 PM Mina Almasry <almasrymina@google.com> wrote:
> >
> > Protect tcp_sendmsg_locked() from mistakenly appending non-zerocopy
> > page fragments to unreadable devmem skbs. Create a new segment instead.
> >
> > Fixes: bd61848900bff ("net: devmem: Implement TX path")
> > Cc: Pavel Begunkov <asml.silence@gmail.com>
> > Cc: Stanislav Fomichev <sdf@fomichev.me>
> > Cc: Bobby Eshleman <bobbyeshleman@gmail.com>
> > Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
> > Reviewed-by: Bobby Eshleman <bobbyeshleman@meta.com>
> > Signed-off-by: Mina Almasry <almasrymina@google.com>
> > ---
> >  net/ipv4/tcp.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 455441f1b6949..186a36c698798 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -1278,6 +1278,11 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
> >                 if (copy > msg_data_left(msg))
> >                         copy = msg_data_left(msg);
> >
> > +               if (zc != MSG_ZEROCOPY && unlikely(!skb_frags_readable(skb))) {
>
> This seems wrong, as @binding could be NULL or not ?
> Also testing the condition right after a fresh skb was allocated is
> adding unecessary cost.
>

Ah, my bad. We were indeed checking zc only and assuming if zc ==
MSG_ZEROCOPY, then also binding is non-NULL and the send is devmem.
That is not correct.

> > +                       tcp_mark_push(tp, skb);
> > +                       goto new_segment;
> > +               }
> > +
> >                 if (zc == 0) {
> >                         bool merge = true;
> >                         int i = skb_shinfo(skb)->nr_frags;
> > --
> > 2.55.0.679.g6767b8d81c-goog
> >
>
> What about instead:
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 455441f1b694904172cfa1d8e7bac7076b60cb24..b4237d0e994d6f9d754d2167023e3981a40b58f4
> 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1240,7 +1240,8 @@ int tcp_sendmsg_locked(struct sock *sk, struct
> msghdr *msg, size_t size)
>
>                 trace_tcp_sendmsg_locked(sk, msg, skb, size_goal);
>
> -               if (copy <= 0 || !tcp_skb_can_collapse_to(skb)) {
> +               if (copy <= 0 || !tcp_skb_can_collapse_to(skb) ||
> +                   unlikely(skb_frags_readable(skb) != !binding)) {
>                         bool first_skb;
>
>  new_segment:


This fix is much better and looks like it will work. FWIW I also
checked other call sites of tcp_skb_can_collapse_to like
tcp_should_autocork and mptcp_skb_can_collapse_to and it seems both
work as-is and don't need any changes. I'll rev v4 after the cooldown.
Thanks!

-- 
Thanks,
Mina

Re: [PATCH net v3 1/2] net: core: propagate unreadable flag in skb_zerocopy

[Ilya Maximets]

On 8/11/26 9:53 PM, Mina Almasry wrote:
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index ae69b2cabab9e..482893a5f67dc 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -467,6 +467,9 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>  	if (!dp_ifindex)
>  		return -ENODEV;
>  
> +	if (!skb_frags_readable(skb))
> +		return -EFAULT;
> +
>  	if (skb_vlan_tag_present(skb)) {
>  		nskb = skb_clone(skb, GFP_ATOMIC);
>  		if (!nskb)
FWIW, the devmem integration doesn't seem well-designed.  I understand
that it is for performance, but IMO there should be a way to copy the
data on a slow path to avoid dropping the packets.  Clamping without
notifying the users that the packet is truncated is not a good solution.
Not for OVS, not for other parts of the kernel networking stack.  It's
a uAPI breakage.

As it is, there is not much we can do here without extensive changes
in userspace applications, so for this OVS block:

Reviewed-by: Ilya Maximets <i.maximets@ovn.org>