Re: [PATCH net-next v3 1/2] net: netmem: document netmem and memory provider design in comments

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch adds a comment in skb_try_coalesce()
  (net/core/skbuff.c, lines 6213-6216).
- [Low] The new skb_frag_t comment (include/linux/skbuff.h:361-363) gives
  'the same io_uring area' as an example of 'the same memory provider…

--

net: netmem: document netmem and memory provider design in comments

This commit adds comments describing the design principles for netmem,
memory providers, page_pool and skb frags. It documents that all frags in
an skb must be page-backed or come from the same memory provider instance.
It also adds a comment to that effect in skb_try_coalesce().

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 27ec1e38c8283..af1cbc2799b77 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -358,6 +358,12 @@ struct sk_buff;
>   */
>  #define GSO_BY_FRAGS	0xFFFF
>  
> +/* All frags in an skb must either all be struct page-backed or all belong to
> + * the same memory provider instance (e.g. the same devmem binding or io_uring
> + * area). Mixing page and memory-provider frags, or mixing frags from different

[Severity: Low]
Is "io_uring area" the right example here? For io_uring zcrx, the provider
instance is the ifq, not the area:

io_uring/zcrx.c:io_pp_to_ifq() {
	return pp->mp_priv;
}

io_zcrx_recv_frag() also checks identity per ifq:

	if (!pp || pp->mp_ops != &io_uring_pp_zc_ops || io_pp_to_ifq(pp) != ifq)
		return -EFAULT;

One ifq can own several areas. io_zcrx_append_area() grows ifq->areas[] up
to ZCRX_MAX_AREAS, and userspace can add areas to an existing ifq with
ZCRX_CTRL_ADD_AREA. Frags from two areas of the same ifq share one provider
instance and one page_pool, and nothing stops them from sharing an skb.

Read literally, the example says those frags must not share an skb. Would
"the same io_uring zcrx ifq" be more accurate?

The later commit "docs: netmem: document netmem and memory provider design
principles" repeats this example in Documentation/networking/netmem.rst.

> + * memory providers within a single skb (including via coalescing), is not
> + * allowed.
> + */
>  typedef struct skb_frag {
>  	netmem_ref netmem;
>  	unsigned int len;

[ ... ]

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index d56f4f0102f75..69d3d27e29d97 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -6210,6 +6210,10 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
>  	if (to->pp_recycle != from->pp_recycle)
>  		return false;
>  
> +	/* All frags in an skb must either all be struct page-backed or all
> +	 * belong to the same memory provider instance; do not coalesce skbs
> +	 * that would mix them.
> +	 */
>  	if (skb_frags_readable(from) != skb_frags_readable(to))
>  		return false;

[Severity: Medium]
Does this check enforce the rule stated in the comment above it?
skb_frags_readable() only looks at a single bit:

include/linux/skbuff.h:skb_frags_readable() {
	return !skb->unreadable;
}

Nothing in skb_try_coalesce() compares pp->mp_priv, the devmem binding or
the zcrx ifq of to and from.

Two unreadable skbs from different provider instances would pass this
check, and their frag arrays would be memcpy'd together. For example, two
devmem bindings, or a devmem binding and a zcrx ifq, could reach one TCP
socket through different RX queues via
tcp_try_coalesce()->skb_try_coalesce().

Also, when an unreadable from skb has a non-zero, stealable linear head,
the skb_headlen(from) != 0 branch adds the head page as a page frag next to
the net_iov frags:

	page = virt_to_head_page(from->head);
	...
	skb_fill_page_desc(to, to_shinfo->nr_frags,
			   page, offset, skb_headlen(from));

skb_gro_receive() in net/core/gro.c is another coalescing path. It checks
only pp_recycle and skb_zcopy, with no readability or provider check.

Existing consumers already handle mixed skbs one frag at a time:
io_zcrx_recv_frag() rejects frags from another ifq, and
tcp_recvmsg_dmabuf() re-checks each frag with skb_frag_net_iov().

The same patch also says providers may supply page-backed netmems and that
net_iovs may be readable. Under those principles, readability alone can't
tell page frags from provider frags.

The new skb_frag_t comment and the commit message state this as a hard
invariant ("is not allowed", "including via coalescing"). Could the wording
describe what the code currently guarantees instead? Otherwise the per-frag
checks above might later be removed because of it.

Or should skb_try_coalesce() and skb_gro_receive() actually compare
provider identity?

The later commit "docs: netmem: document netmem and memory provider design
principles" repeats the same claim in Documentation/networking/netmem.rst.
The comment here is not corrected anywhere in the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010033630.1171692-1-almasrymina%40google.com