Re: [PATCH net-next 11/15] net: skbuff: refuse to segment a nested frag_list

[netdev-bot+sashiko@xxxxxxxxxx]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "net: skbuff: refuse to segment a nested
  frag_list" says "Nothing in the tree is known to build such an skb;…

--

net: skbuff: refuse to segment a nested frag_list

This adds a check to skb_segment() that rejects an skb whose frag_list
members have frag_lists of their own. It fails with -EINVAL and calls
DEBUG_NET_WARN_ON_ONCE(), since the segmentation walk would otherwise run
out of members and dereference a NULL list_skb.

> Nothing in the tree is known to build such an skb; GRO flushes instead
> of chaining an skb that already has a frag_list.  Check for it up front

[Severity: Low]

Is the first half of this sentence accurate as a general statement?

IP fragment reassembly appears to build exactly this layout. When the
first fragment already has a frag_list, inet_frag_reasm_prepare() moves
that frag_list under a new zero-length skb:

net/ipv4/inet_fragment.c:inet_frag_reasm_prepare() {
    ...
        if (skb_has_frag_list(head)) {
                struct sk_buff *clone;
                ...
                clone = alloc_skb(0, GFP_ATOMIC);
                ...
                skb_shinfo(clone)->frag_list = skb_shinfo(head)->frag_list;
                skb_frag_list_init(head);
                ...
                skb_shinfo(head)->frag_list = clone;
                nextp = &clone->next;
    ...
}

The result is head -> frag_list[clone -> frag_list[...]]. That is the
nested layout the new check in skb_segment() rejects.

The GRO part is correct. dev_gro_receive() sets:

        NAPI_GRO_CB(skb)->flush = skb_has_frag_list(skb);

Reassembled skbs are not GSO, so they don't normally reach skb_segment().
The narrower claim seems to be what's meant: no in-tree path builds a GSO
skb with a nested frag_list.

Could the commit message say that instead, and mention IP defrag as a
known non-GSO source of nested frag_lists?

-- 
Sashiko AI review · 
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019-b4-skbuff-bug-on-followups-v1-0-41dbef50b9f7%40toxicpanda.com