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

[Paolo Abeni <pabeni@redhat.com>]

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.