On Tue, Aug 18, 2026 at 3:26 AM Paolo Abeni <[email protected]> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
