* [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy
@ 2026-08-14 19:13 Mina Almasry
2026-08-14 19:13 ` [PATCH net v4 2/2] net: tcp: block mixing readable and unreadable frags Mina Almasry
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Mina Almasry @ 2026-08-14 19:13 UTC (permalink / raw)
To: Jakub Kicinski, Kaiyuan Zhang, Willem de Bruijn, Eric Dumazet,
Mina Almasry, Paolo Abeni, Stanislav Fomichev, netdev,
linux-kernel, dev
Cc: David S. Miller, Simon Horman, Neal Cardwell, Kuniyuki Iwashima,
Aaron Conole, Eelco Chaudron, Ilya Maximets, Jason Xing,
Pavel Begunkov, Bobby Eshleman, Florian Westphal
skb_zerocopy() fails to propagate the unreadable flag when copying
unreadable fragments, causing target skbs to appear as readable memory.
This patch fixes the flag propagation. Additionally, it returns -EFAULT
if readable fragments are mixed with unreadable 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>
Signed-off-by: Mina Almasry <almasrymina@google.com>
Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
---
v4:
- Changed commit references from payload types to readable/unreadable
fragments.
---
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: 24ef02f934eeb48830cff6b739abc3c62b1d107b
--
2.55.0.691.gc56d675ccc-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH net v4 2/2] net: tcp: block mixing readable and unreadable frags 2026-08-14 19:13 [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy Mina Almasry @ 2026-08-14 19:13 ` Mina Almasry 2026-08-18 10:26 ` Paolo Abeni 2026-08-18 10:25 ` [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy Paolo Abeni 2026-08-20 20:30 ` patchwork-bot+netdevbpf 2 siblings, 1 reply; 7+ messages in thread From: Mina Almasry @ 2026-08-14 19:13 UTC (permalink / raw) To: Jakub Kicinski, Kaiyuan Zhang, Willem de Bruijn, Eric Dumazet, Mina Almasry, Paolo Abeni, Stanislav Fomichev, netdev, linux-kernel, dev Cc: David S. Miller, Simon Horman, Neal Cardwell, Kuniyuki Iwashima, Aaron Conole, Eelco Chaudron, Ilya Maximets, Jason Xing, Pavel Begunkov, Bobby Eshleman, Florian Westphal Protect tcp_sendmsg_locked() from mistakenly mixing readable and unreadable page fragments in the same SKB. Check that the devmem binding matches the existing SKB's readability. If a mismatch is detected, avoid collapsing and create a new segment. Fixes: bd61848900bff ("net: devmem: Implement TX path") Suggested-by: Eric Dumazet <edumazet@google.com> Cc: Pavel Begunkov <asml.silence@gmail.com> Cc: Stanislav Fomichev <sdf@fomichev.me> Cc: Bobby Eshleman <bobbyeshleman@gmail.com> Signed-off-by: Mina Almasry <almasrymina@google.com> --- v4: - Adopt Eric's suggestion to check 'binding' against SKB readability alongside tcp_skb_can_collapse_to(). - Drop pre-existing Reviewed-by tags due to significant implementation changes. v3: https://lore.kernel.org/r/20260811195405.3979177-2-almasrymina@google.com v2: https://lore.kernel.org/r/20260810180956.2348280-2-almasrymina@google.com v1: https://lore.kernel.org/r/20260801125308.1342897-3-almasrymina@google.com --- net/ipv4/tcp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 455441f1b6949..b4237d0e994d6 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: -- 2.55.0.691.gc56d675ccc-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net v4 2/2] net: tcp: block mixing readable and unreadable frags 2026-08-14 19:13 ` [PATCH net v4 2/2] net: tcp: block mixing readable and unreadable frags Mina Almasry @ 2026-08-18 10:26 ` Paolo Abeni 2026-08-18 18:37 ` Mina Almasry 0 siblings, 1 reply; 7+ messages in thread From: Paolo Abeni @ 2026-08-18 10:26 UTC (permalink / raw) To: almasrymina Cc: kuba, kaiyuanz, willemb, edumazet, pabeni, sdf, netdev, linux-kernel, dev, davem, horms, ncardwell, kuniyu, aconole, echaudro, i.maximets, kerneljasonxing, asml.silence, bobbyeshleman, fw This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://netdev-ai.bots.linux.dev/sashiko/ --- net: tcp: block mixing readable and unreadable frags This adds a check in tcp_sendmsg_locked() so an existing tail SKB is not collapsed into when its readability does not match the presence of a devmem binding, forcing a new segment instead. > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 455441f1b6949..b4237d0e994d6 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: [Severity: Medium] Is binding alone the right proxy here for "this send will append unreadable frags"? What actually selects the devmem filler is the zc dispatch in the loop plus __zerocopy_sg_from_iter(): net/core/datagram.c:__zerocopy_sg_from_iter() { ... else if (binding) ret = zerocopy_fill_skb_from_devmem(skb, from, length, binding); else ret = zerocopy_fill_skb_from_iter(skb, from, length); } and that is only reached from the zc == MSG_ZEROCOPY arm of tcp_sendmsg_locked(). Can binding != NULL coexist with zc == 0? In tcp_sendmsg_locked() the binding is taken whenever the cmsg carries dmabuf_id: if (sk->sk_route_caps & NETIF_F_SG) zc = MSG_ZEROCOPY; else uarg_to_msgzc(uarg)->zerocopy = 0; if (!sockc_err && sockc.dmabuf_id) { binding = net_devmem_get_binding(sk, sockc.dmabuf_id); while zc is only set under NETIF_F_SG. The one dmabuf_id validation does not look at SG at all: if (!sockc_err && sockc.dmabuf_id && (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY))) { err = -EINVAL; And sk_setup_caps() in net/core/sock.c only force-adds NETIF_F_SG when sk_can_gso() holds: if (unlikely(sk->sk_gso_disabled)) sk->sk_route_caps &= ~NETIF_F_GSO_MASK; if (sk_can_gso(sk)) { if (dst->header_len && !xfrm_dst_offload_ok(dst)) { sk->sk_route_caps &= ~NETIF_F_GSO_MASK; } else { sk->sk_route_caps |= NETIF_F_SG | NETIF_F_HW_CSUM; so a TCP-MD5/TCP-AO socket (sk_gso_disable()) or an IPsec dst on a device with sg turned off ends up with binding != NULL and zc == 0. In that state, does the new predicate go wrong in both directions? If the tail is readable or newly allocated, skb_frags_readable(skb) != !binding evaluates to true != false, so every iteration takes goto new_segment and the write is split into one skb per iteration, even though the zc == 0 path only appends readable page frags via skb_fill_page_desc(). If the tail is already unreadable from an earlier devmem send, the test is false != false, collapsing is allowed, and skb_fill_page_desc() adds a readable page frag to an skb that __skb_fill_netmem_desc() already marked unreadable = 1 — the mixing this patch aims to prevent. Would validate_xmit_unreadable_skb() catch that? It only looks at the first frag: net/core/dev.c:validate_xmit_unreadable_skb() { if (shinfo->nr_frags > 0) { niov = netmem_to_net_iov(skb_frag_netmem(&shinfo->frags[0])); Would keying the new clause on binding && zc == MSG_ZEROCOPY, or rejecting dmabuf_id when the route cannot do SG, line the predicate up with the code that actually fills the frags? Note the mixing described in the second direction pre-dates this patch (there was no check at all before); the incorrect predicate and the forced re-segmentation are new. Reaching this also needs a local privileged setup (dmabuf TX binding plus MD5/AO or IPsec plus an ethtool feature change), and in that configuration a devmem send through the copy path would likely already fail in skb_copy_to_page_nocache() since the iovec holds dmabuf offsets rather than user addresses, so this is stated as an observation rather than a demonstrated corruption path. -- This is an AI-generated review. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v4 2/2] net: tcp: block mixing readable and unreadable frags 2026-08-18 10:26 ` Paolo Abeni @ 2026-08-18 18:37 ` Mina Almasry 0 siblings, 0 replies; 7+ messages in thread From: Mina Almasry @ 2026-08-18 18:37 UTC (permalink / raw) To: Paolo Abeni Cc: kuba, kaiyuanz, willemb, edumazet, sdf, netdev, linux-kernel, dev, davem, horms, ncardwell, kuniyu, aconole, echaudro, i.maximets, kerneljasonxing, asml.silence, bobbyeshleman, fw On Tue, Aug 18, 2026 at 3:26 AM Paolo Abeni <pabeni@redhat.com> wrote: > > This is an AI-generated review of your patch. The human sending this > email has considered the AI review valid, or at least plausible. > Full review at: https://netdev-ai.bots.linux.dev/sashiko/ > --- > net: tcp: block mixing readable and unreadable frags > > This adds a check in tcp_sendmsg_locked() so an existing tail SKB is not > collapsed into when its readability does not match the presence of a > devmem binding, forcing a new segment instead. > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > > index 455441f1b6949..b4237d0e994d6 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: > > [Severity: Medium] > Is binding alone the right proxy here for "this send will append > unreadable frags"? > > What actually selects the devmem filler is the zc dispatch in the loop > plus __zerocopy_sg_from_iter(): > > net/core/datagram.c:__zerocopy_sg_from_iter() { > ... > else if (binding) > ret = zerocopy_fill_skb_from_devmem(skb, from, length, binding); > else > ret = zerocopy_fill_skb_from_iter(skb, from, length); > } > > and that is only reached from the zc == MSG_ZEROCOPY arm of > tcp_sendmsg_locked(). > > Can binding != NULL coexist with zc == 0? In tcp_sendmsg_locked() the > binding is taken whenever the cmsg carries dmabuf_id: > > if (sk->sk_route_caps & NETIF_F_SG) > zc = MSG_ZEROCOPY; > else > uarg_to_msgzc(uarg)->zerocopy = 0; > > if (!sockc_err && sockc.dmabuf_id) { > binding = net_devmem_get_binding(sk, sockc.dmabuf_id); > > while zc is only set under NETIF_F_SG. The one dmabuf_id validation > does not look at SG at all: > > if (!sockc_err && sockc.dmabuf_id && > (!(flags & MSG_ZEROCOPY) || !sock_flag(sk, SOCK_ZEROCOPY))) { > err = -EINVAL; > > And sk_setup_caps() in net/core/sock.c only force-adds NETIF_F_SG when > sk_can_gso() holds: > > if (unlikely(sk->sk_gso_disabled)) > sk->sk_route_caps &= ~NETIF_F_GSO_MASK; > if (sk_can_gso(sk)) { > if (dst->header_len && !xfrm_dst_offload_ok(dst)) { > sk->sk_route_caps &= ~NETIF_F_GSO_MASK; > } else { > sk->sk_route_caps |= NETIF_F_SG | NETIF_F_HW_CSUM; > > so a TCP-MD5/TCP-AO socket (sk_gso_disable()) or an IPsec dst on a device > with sg turned off ends up with binding != NULL and zc == 0. > > In that state, does the new predicate go wrong in both directions? > > If the tail is readable or newly allocated, skb_frags_readable(skb) != > !binding evaluates to true != false, so every iteration takes > goto new_segment and the write is split into one skb per iteration, even > though the zc == 0 path only appends readable page frags via > skb_fill_page_desc(). > > If the tail is already unreadable from an earlier devmem send, the test > is false != false, collapsing is allowed, and skb_fill_page_desc() adds a > readable page frag to an skb that __skb_fill_netmem_desc() already marked > unreadable = 1 — the mixing this patch aims to prevent. Would > validate_xmit_unreadable_skb() catch that? It only looks at the first > frag: > > net/core/dev.c:validate_xmit_unreadable_skb() { > if (shinfo->nr_frags > 0) { > niov = netmem_to_net_iov(skb_frag_netmem(&shinfo->frags[0])); > > Would keying the new clause on binding && zc == MSG_ZEROCOPY, or > rejecting dmabuf_id when the route cannot do SG, line the predicate up > with the code that actually fills the frags? > > Note the mixing described in the second direction pre-dates this patch > (there was no check at all before); the incorrect predicate and the > forced re-segmentation are new. Reaching this also needs a local > privileged setup (dmabuf TX binding plus MD5/AO or IPsec plus an ethtool > feature change), and in that configuration a devmem send through the copy > path would likely already fail in skb_copy_to_page_nocache() since the > iovec holds dmabuf offsets rather than user addresses, so this is stated > as an observation rather than a demonstrated corruption path. Pre-existing issue I think. We indeed assume binding != NULL means the send is devmem and zc is set. We missed that we may not set zc if NETIF_F_SG is not enabled. Probably the code setting binding to non-NULL should check if zc is set. I think this is best reserved for its own follow up fix, but let me know if not Paolo. -- Thanks, Mina ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy 2026-08-14 19:13 [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy Mina Almasry 2026-08-14 19:13 ` [PATCH net v4 2/2] net: tcp: block mixing readable and unreadable frags Mina Almasry @ 2026-08-18 10:25 ` Paolo Abeni 2026-08-18 18:36 ` Mina Almasry 2026-08-20 20:30 ` patchwork-bot+netdevbpf 2 siblings, 1 reply; 7+ messages in thread From: Paolo Abeni @ 2026-08-18 10:25 UTC (permalink / raw) To: almasrymina Cc: kuba, kaiyuanz, willemb, edumazet, pabeni, sdf, netdev, linux-kernel, dev, davem, horms, ncardwell, kuniyu, aconole, echaudro, i.maximets, kerneljasonxing, asml.silence, bobbyeshleman, fw This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. Full review at: https://netdev-ai.bots.linux.dev/sashiko/ --- net: core: propagate unreadable flag in skb_zerocopy The patch makes skb_zerocopy() propagate the unreadable flag to the destination skb, returns -EFAULT when a readable head payload would be mixed with unreadable frags, and rejects unreadable skbs in openvswitch queue_userspace_packet(). > 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 [ ... ] > @@ -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; > } [Severity: Medium] On this -ENOMEM path, is it safe to re-derive the head page from from->head after skb_orphan_frags() has already run? skb_orphan_frags() -> skb_copy_ubufs() drops the clone first: net/core/skbuff.c:skb_copy_ubufs() { ... if (skb_shared(skb) || skb_unclone(skb, gfp_mask)) return -EINVAL; ... page = alloc_pages(gfp_mask | __GFP_COMP, order); if (!page) { ... return -ENOMEM; } } and skb_unclone() -> pskb_expand_head() replaces the buffer: net/core/skbuff.c:pskb_expand_head() { ... skb->head = data; skb->head_frag = 0; ... } So when the -ENOMEM comes from the alloc_pages() loop, from->head can already point at a different (freshly allocated) buffer than the page that was get_page()'d earlier in the hlen == 0 branch: page = virt_to_head_page(from->head); offset = from->data - (unsigned char *)page_address(page); __skb_fill_netmem_desc(to, 0, page_to_netmem(page), offset, plen); get_page(page); j = 1; Does that leak the reference on the original head-fragment page, and apply the put_page() to an unrelated buffer? Today the misdirected put_page() is quiet because pskb_expand_head() installs slab/kmalloc memory and put_page() early-returns for folio_test_slab()/folio_test_large_kmalloc() folios, but the reference on the page that was actually taken is never returned. Would it be more robust to keep the page pointer captured before get_page() in a local and use put_page(page) in both new error paths? I was not able to find an in-tree producer that combines head_frag == 1 with skb_zcopy() set, so this path may not be reachable today. [Severity: Low] This hunk looks like it also fixes an older, unrelated head-page reference leak. __skb_fill_netmem_desc() does not bump skb_shinfo(to)->nr_frags: include/linux/skbuff.h:__skb_fill_netmem_desc() { __skb_fill_netmem_desc_noacc(skb_shinfo(skb), i, netmem, off, size); ... } and skb_zerocopy() only sets it at the very end with skb_shinfo(to)->nr_frags = j;. So before this patch, returning -ENOMEM here left @to with nr_frags == 0 and the extra head-page reference was never dropped when the caller freed it (consume_skb(user_skb) in queue_userspace_packet(), kfree_skb() in nfqnl_build_packet_message()). That leak dates back to 36d5fe6a0007 ("core, nfqueue, openvswitch: Orphan frags in skb_zerocopy and handle errors") and is unrelated to unreadable frags, while the changelog only mentions unreadable-flag propagation and the new -EFAULT returns, and the Fixes: tag points at 65249feb6b3d. Could this hunk be split into its own patch with Fixes: 36d5fe6a0007, or at least described in the commit message, so that stable backports reach the pre-6.12 kernels that also contain the leak? > + if (i > 0 && from->unreadable) > + to->unreadable = 1; > + [ ... ] -- This is an AI-generated review. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy 2026-08-18 10:25 ` [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy Paolo Abeni @ 2026-08-18 18:36 ` Mina Almasry 0 siblings, 0 replies; 7+ messages in thread From: Mina Almasry @ 2026-08-18 18:36 UTC (permalink / raw) To: Paolo Abeni Cc: kuba, kaiyuanz, willemb, edumazet, sdf, netdev, linux-kernel, dev, davem, horms, ncardwell, kuniyu, aconole, echaudro, i.maximets, kerneljasonxing, asml.silence, bobbyeshleman, fw On Tue, Aug 18, 2026 at 3:26 AM Paolo Abeni <pabeni@redhat.com> wrote: > > This is an AI-generated review of your patch. The human sending this > email has considered the AI review valid, or at least plausible. > Full review at: https://netdev-ai.bots.linux.dev/sashiko/ Thanks Paolo. Responding to the specific feedback inline, but in general, clashiko is pointing out pre-existing issues. They should be fixed absolutely but I'd prefer to submit separate fixes for them. I'm a bit worried about going down this rabbit hole forever. Let me know what you think. > --- > net: core: propagate unreadable flag in skb_zerocopy > > The patch makes skb_zerocopy() propagate the unreadable flag to the > destination skb, returns -EFAULT when a readable head payload would be > mixed with unreadable frags, and rejects unreadable skbs in openvswitch > queue_userspace_packet(). > > > 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 > > [ ... ] > > > @@ -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; > > } > > [Severity: Medium] > On this -ENOMEM path, is it safe to re-derive the head page from > from->head after skb_orphan_frags() has already run? > Probably not. I think this is a pre-existing issue in skb_orphan_frags though. I think skb_orphan_frags should check !skb_frags_readable() before doing anything so that it doens't modify the skb and then return -EFAULT on a modified skb. > skb_orphan_frags() -> skb_copy_ubufs() drops the clone first: > > net/core/skbuff.c:skb_copy_ubufs() { > ... > if (skb_shared(skb) || skb_unclone(skb, gfp_mask)) > return -EINVAL; > ... > page = alloc_pages(gfp_mask | __GFP_COMP, order); > if (!page) { > ... > return -ENOMEM; > } > } > > and skb_unclone() -> pskb_expand_head() replaces the buffer: > > net/core/skbuff.c:pskb_expand_head() { > ... > skb->head = data; > skb->head_frag = 0; > ... > } > > So when the -ENOMEM comes from the alloc_pages() loop, from->head can > already point at a different (freshly allocated) buffer than the page that > was get_page()'d earlier in the hlen == 0 branch: > > page = virt_to_head_page(from->head); > offset = from->data - (unsigned char *)page_address(page); > __skb_fill_netmem_desc(to, 0, page_to_netmem(page), offset, plen); > get_page(page); > j = 1; > > Does that leak the reference on the original head-fragment page, and apply > the put_page() to an unrelated buffer? Today the misdirected put_page() is > quiet because pskb_expand_head() installs slab/kmalloc memory and put_page() > early-returns for folio_test_slab()/folio_test_large_kmalloc() folios, but > the reference on the page that was actually taken is never returned. > > Would it be more robust to keep the page pointer captured before get_page() > in a local and use put_page(page) in both new error paths? > > I was not able to find an in-tree producer that combines head_frag == 1 with > skb_zcopy() set, so this path may not be reachable today. > > [Severity: Low] > This hunk looks like it also fixes an older, unrelated head-page reference > leak. __skb_fill_netmem_desc() does not bump skb_shinfo(to)->nr_frags: > > include/linux/skbuff.h:__skb_fill_netmem_desc() { > __skb_fill_netmem_desc_noacc(skb_shinfo(skb), i, netmem, off, size); > ... > } > > and skb_zerocopy() only sets it at the very end with > skb_shinfo(to)->nr_frags = j;. So before this patch, returning -ENOMEM here > left @to with nr_frags == 0 and the extra head-page reference was never > dropped when the caller freed it (consume_skb(user_skb) in > queue_userspace_packet(), kfree_skb() in nfqnl_build_packet_message()). > > That leak dates back to 36d5fe6a0007 ("core, nfqueue, openvswitch: Orphan > frags in skb_zerocopy and handle errors") and is unrelated to unreadable > frags, while the changelog only mentions unreadable-flag propagation and the > new -EFAULT returns, and the Fixes: tag points at 65249feb6b3d. > > Could this hunk be split into its own patch with Fixes: 36d5fe6a0007, or at > least described in the commit message, so that stable backports reach the > pre-6.12 kernels that also contain the leak? > Right, I added this hunk because sashkio reported this pre-existing issue in a previous iteration. I could fork it into its own fix, but for the follow up pre-existing issues it's reporting now, it maybe makes sense to also fix those in follow up fixes. -- Thanks, Mina ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy 2026-08-14 19:13 [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy Mina Almasry 2026-08-14 19:13 ` [PATCH net v4 2/2] net: tcp: block mixing readable and unreadable frags Mina Almasry 2026-08-18 10:25 ` [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy Paolo Abeni @ 2026-08-20 20:30 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 7+ messages in thread From: patchwork-bot+netdevbpf @ 2026-08-20 20:30 UTC (permalink / raw) To: Mina Almasry Cc: kuba, kaiyuanz, willemb, edumazet, pabeni, sdf, netdev, linux-kernel, dev, davem, horms, ncardwell, kuniyu, aconole, echaudro, i.maximets, kerneljasonxing, asml.silence, bobbyeshleman, fw Hello: This series was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 14 Aug 2026 19:13:30 +0000 you wrote: > skb_zerocopy() fails to propagate the unreadable flag when copying > unreadable fragments, causing target skbs to appear as readable memory. > > This patch fixes the flag propagation. Additionally, it returns -EFAULT > if readable fragments are mixed with unreadable fragments during > extraction, and returns -EFAULT in openvswitch queue_userspace_packet(). > > [...] Here is the summary with links: - [net,v4,1/2] net: core: propagate unreadable flag in skb_zerocopy https://git.kernel.org/netdev/net/c/68d8c6532659 - [net,v4,2/2] net: tcp: block mixing readable and unreadable frags https://git.kernel.org/netdev/net/c/d9c56501c72f You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-08-20 20:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-14 19:13 [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy Mina Almasry 2026-08-14 19:13 ` [PATCH net v4 2/2] net: tcp: block mixing readable and unreadable frags Mina Almasry 2026-08-18 10:26 ` Paolo Abeni 2026-08-18 18:37 ` Mina Almasry 2026-08-18 10:25 ` [PATCH net v4 1/2] net: core: propagate unreadable flag in skb_zerocopy Paolo Abeni 2026-08-18 18:36 ` Mina Almasry 2026-08-20 20:30 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox