Re: [PATCH net-next v3 2/2] docs: netmem: document netmem and memory provider design principles

[netdev-bot+sashiko@kernel.org]

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

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

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] Item 5 of the new Design Principles section in
  Documentation/networking/netmem.rst says every frag in an skb must be…
- [Low] Items 2 and 5 of the new Design Principles section contradict
  each other once a page-backed memory provider exists.
- [Low] Item 3 of netmem.rst, and the matching net_iov comment in
  include/net/netmem.h added by the commit "net: netmem: document netmem…

--

docs: netmem: document netmem and memory provider design principles

This commit adds a Design Principles section to
Documentation/networking/netmem.rst. The section covers the netmem_ref
abstraction, the rule against downcasting, decoupling memory providers
from net_iov and net_iov from unreadability, and delegating type-specific
logic to the lower layers. It also adds a rule against mixing skb fragment
memory sources.

> diff --git a/Documentation/networking/netmem.rst b/Documentation/networking/netmem.rst
> index 217869d1108dd..6871dcdd700cc 100644
> --- a/Documentation/networking/netmem.rst
> +++ b/Documentation/networking/netmem.rst
> @@ -19,6 +19,59 @@ Benefits of Netmem :

[ ... ]

> +3. **Decouple net_iov from unreadability**: ``struct net_iov`` is flexible and
> +   has no inherent restrictions; it may represent either CPU-readable or
> +   unreadable memory. Today, in-tree ``net_iov`` implementations are unreadable
> +   by the CPU (``netmem_address()`` returns ``NULL``) and some existing code
> +   still reflects that limitation, but new code must not assume ``net_iov``
> +   implies unreadable memory (check readability via ``netmem_address()`` or
> +   ``skb_frags_readable()`` instead) and should, as much as possible, generalize
> +   existing limitations to match the design principles.

[Severity: Low]
Can netmem_address() reliably test readability for page-backed netmem?
For pages, it ends up calling page_address():

include/net/netmem.h:netmem_address() {
	if (netmem_is_net_iov(netmem))
		return NULL;

	return __netmem_address(netmem);
}

With CONFIG_HIGHMEM, page_address() in mm/highmem.c returns NULL for a
highmem page that has no permanent mapping. The CPU can still read that
page through kmap_local_page(), which item 1 uses as its example.

Highmem page frags are normal in skbs on 32-bit kernels. They come from
sendfile/splice or from MSG_ZEROCOPY user pages.

Could new code that follows this advice and treats a NULL return from
netmem_address() as "unreadable" wrongly reject or skip valid highmem page
frags?

The same advice appears in the struct net_iov comment in
include/net/netmem.h. That comment was added by "net: netmem: document
netmem and memory provider design in comments".

Would it be more accurate to say that a NULL netmem_address() means "no
permanent kernel mapping"? Another option is to point only to
skb_frags_readable() or to a dedicated netmem readability helper.

[ ... ]

> +5. **Do not mix skb fragment memory sources**: All ``frags[]`` in an
> +   ``sk_buff`` must either be ``struct page``-backed or belong to the same
> +   memory provider instance (for example, the same ``devmem`` binding or
> +   ``io_uring`` area). Mixing ``struct page`` and memory-provider fragments,
> +   or mixing fragments from different memory providers within a single
> +   ``sk_buff``, is not allowed (including when coalescing ``sk_buff``\ s).

[Severity: Medium]
Is this invariant enforced anywhere today? Items 2 and 3 note that some
existing code does not follow them yet, but this item has no such note.

The companion commit "net: netmem: document netmem and memory provider
design in comments" adds this comment in skb_try_coalesce():

net/core/skbuff.c:skb_try_coalesce() {
    ...
	/* 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;
    ...
}

skb_frags_readable() only returns !skb->unreadable. Two unreadable skbs
can carry net_iovs from different devmem bindings, or from a devmem
binding and an io_uring zcrx area. Both would pass this check, and their
frags would then be merged.

Other places look similar:

- tcp_skb_can_collapse() in include/net/tcp.h compares only readability.
- skb_gro_receive() in net/core/gro.c checks pp_recycle and skb_zcopy(),
  but not readability or provider identity.

On TX, tcp_sendmsg_locked() looks up the binding again on every sendmsg
call with net_devmem_get_binding(sk, sockc.dmabuf_id). Before it appends
to the tail skb, it only checks:

	if (copy <= 0 || !tcp_skb_can_collapse_to(skb) ||
	    unlikely(skb_frags_readable(skb) != !binding)) {

msg_zerocopy_realloc() then extends the tail skb's uarg. Because of that,
the -EEXIST check in skb_zerocopy_iter_stream() never triggers.

bind-tx does not require privileges. Could a user bind two dmabufs to one
netdev, then send two back-to-back MSG_ZEROCOPY sendmsg calls with
different dmabuf_ids? That would leave net_iovs from two bindings in one
skb.

validate_xmit_unreadable_skb() in net/core/dev.c already relies on this
invariant, because it only checks frags[0]:

	niov = netmem_to_net_iov(skb_frag_netmem(&shinfo->frags[0]));
	if (net_is_devmem_iov(niov) &&
	    READ_ONCE(net_devmem_iov_binding(niov)->dev) != dev)
		goto out_free;

Should these places get a provider-identity check? Otherwise, could this
item and the skb_try_coalesce() comment say that only readability is
enforced today, the way items 2 and 3 describe their own limitations?

[Severity: Low]
Do items 2 and 5 conflict once a page-backed memory provider exists?

Item 2 says "a memory provider that returns struct page-backed
netmem_refs to upper layers is allowed". A frag from such a provider
fits both categories in the first sentence here: "struct page-backed"
and "same memory provider instance". Read that way, it could share an
skb with ordinary page frags, or with page frags from a different
page-backed provider. skb_try_coalesce() would allow that today.

The second sentence, however, forbids "Mixing struct page and
memory-provider fragments, or mixing fragments from different memory
providers".

The new skb_frag_t comment in include/linux/skbuff.h uses the same
wording.

Could the two categories be stated so they don't overlap? For example,
"non-provider struct pages" versus "frags from a single provider
instance, whatever their backing".

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