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

On Tue, Aug 18, 2026 at 3:26 AM Paolo Abeni [off-list ref] 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().

quoted

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

[ ... ]

quoted

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