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

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

[Mina Almasry]

On Wed, Aug 12, 2026 at 8:52 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> 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.

As is most of what I touch :P

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

FWIW, it happens that the fallback-to-copy in the context of the
devmem TCP seems to be useless to the userspace. As a matter of fact
there is one current path where we fallback to copy/CPU memory
(SCM_DEVMEM_LINEAR), and my users decided to write userspace to
completely barf on that condition. We ended up rooting all the reasons
SCM_DEVMEM_LINEAR could happen and preventing that (mostly flow
steering failures in our case).

Broadcomm also added a devmem kselftest test case that fails on any
SCM_DEVMEM_LINEAR as well, so I think they may have independently
reached the same conclusion with their users.

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

But, a future user could find such a fallback-to-cpu-mem useful. If
anyone is reading this wondering how to implement that, these are my
rough ideas:

+ some dmabufs will support the dma_buf_vmap op which will map the
dmabuf to the kernel space. if the dmabuf supports it, we could use
that op to map the dmabuf memory to the kernel space, then copy the
memory to a normal page allocated from a non-devmem page_pool, and
create a readable skb based on that.
+ If the dmabuf does not support dma_buf_vmap, then I don't think
there is any copy fallback we can do, sorry.
+ We'd need to design a uapi that tells the user that this particular
chunk is in cpu memory. My guess is that recvmsg() needs to return if
it notices a devmem/non-devmem skb boundary, and return early. Then
the non-devmem skb can be given to the userspace with
SCM_DEVMEM_LINEAR on the next recvmsg() call. Then the next recvmsg()
call gets the next devmem skb without SCM_DEVMEM_LINEAR, etc. I think
that would work.
+ We'd need the driver to maintain multiple page_pools per rx-queue
then, I guess. One for the devmem, and one for the possible non-devmem
fallback.

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

Thank you! I'll submit another revision addressing the comments on the
other patch.

-- 
Thanks,
Mina

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

[Ilya Maximets]

On 8/14/26 8:48 PM, Mina Almasry wrote:
> On Wed, Aug 12, 2026 at 8:52 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>>
>> 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.
> 
> As is most of what I touch :P
> 
>>  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.
>>
> 
> FWIW, it happens that the fallback-to-copy in the context of the
> devmem TCP seems to be useless to the userspace. As a matter of fact
> there is one current path where we fallback to copy/CPU memory
> (SCM_DEVMEM_LINEAR), and my users decided to write userspace to
> completely barf on that condition. We ended up rooting all the reasons
> SCM_DEVMEM_LINEAR could happen and preventing that (mostly flow
> steering failures in our case).
> 
> Broadcomm also added a devmem kselftest test case that fails on any
> SCM_DEVMEM_LINEAR as well, so I think they may have independently
> reached the same conclusion with their users.

Just for the context on how OVS works: the very first packet, e.g.
SYN, goes to userspace via this upcall mechanism, then ovs-vswitchd
runs it through the OpenFlow pipeline and figures out what actions
to take and where to forward.  Next it injects the packet back via
netlink request to execute those actions and in parallel it installs
a datapath flow into the kernel.  The next packet that matches the
installed datapath flow does not go to userspace and gets forwarded
inside the kernel.

So, in theory, very few packets go to userspace and the rest stay
in the kernel going through the fast path.  In this situation it
doesn't matter too much that the first packet takes the performance
hit as long as the rest are not.

Depending on the OpenFlow pipeline the syn+ack and the ack+psh may
need to go to userspace, out of which, I suppose the ack+psh is the
most problematic as it carries a large payload that will end up in
the unreadable memory.

In this situation, It seems to me that being able to copy the data
directly to the userspace would be useful as it would not have any
performance impact on the fast path and will allow openvswitch to
work normally for the most part without any changes to ovs-vswitchd
in userspace.

Best regards, Ilya Maximets.

> 
>> As it is, there is not much we can do here without extensive changes
>> in userspace applications, so for this OVS block:
>>
> 
> But, a future user could find such a fallback-to-cpu-mem useful. If
> anyone is reading this wondering how to implement that, these are my
> rough ideas:
> 
> + some dmabufs will support the dma_buf_vmap op which will map the
> dmabuf to the kernel space. if the dmabuf supports it, we could use
> that op to map the dmabuf memory to the kernel space, then copy the
> memory to a normal page allocated from a non-devmem page_pool, and
> create a readable skb based on that.
> + If the dmabuf does not support dma_buf_vmap, then I don't think
> there is any copy fallback we can do, sorry.
> + We'd need to design a uapi that tells the user that this particular
> chunk is in cpu memory. My guess is that recvmsg() needs to return if
> it notices a devmem/non-devmem skb boundary, and return early. Then
> the non-devmem skb can be given to the userspace with
> SCM_DEVMEM_LINEAR on the next recvmsg() call. Then the next recvmsg()
> call gets the next devmem skb without SCM_DEVMEM_LINEAR, etc. I think
> that would work.
> + We'd need the driver to maintain multiple page_pools per rx-queue
> then, I guess. One for the devmem, and one for the possible non-devmem
> fallback.
> 
>> Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
> 
> Thank you! I'll submit another revision addressing the comments on the
> other patch.
>