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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
