Re: [PATCH net-next v3 2/2] net: prevent mixing frags from different memory providers or bindings

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 11/07, Mina Almasry wrote:
> On Fri, Nov 6, 2026 at 2:22 PM Stanislav Fomichev <sdf.kernel@gmail.com> wrote:
> >
> > On 11/06, Mina Almasry wrote:
> > > On Thu, Nov 5, 2026 at 8:59 AM Stanislav Fomichev <sdf.kernel@gmail.com> wrote:
> > > > > +static inline void *skb_net_iov_provider(const struct sk_buff *skb)
> > > > > +{
> > > > > +     struct net_iov *niov;
> > > > > +
> > > >
> > > > [..]
> > > >
> > > > > +     if (!skb_has_net_iovs(skb) || !skb_shinfo(skb)->nr_frags)
> > > > > +             return NULL;
> > > >
> > > > (a few nits/questions)
> > > >
> > > > Why !skb_has_net_iovs is not enough? (and if !nr_frags is required,
> > > > maybe fold it into skb_has_net_iovs?)
> > > >
> > >
> > > Well skb_has_net_iovs only checks skb->unreadable, which is not
> > > actually enough. The skb doesn't necessarily have net_iovs if it's
> > > unreadable, it technically could be that it has no frags, right?
> > >
> > > Technically we should never have the edge case where
> > > skb->unreadable==true && skb_shinfo->nr_frags==0, but I don't think
> > > the code is careful about that today, and I don't know that I want to
> > > do a global refactor everywhere we remove frags, which skb-> flags
> > > need to be updated.
> > >
> > > but you're right that it can be folded.
> >
> > Not sure I'm buying that, but happy to look again if you disagree. I think
> > the only way to make an skb unreadable right now is to add at least
> > one niov frag? Don't think this is (ever?) gonna change? What does
> > and unreadable skb without frags even means?
> 
> I have no doubt that we set skb->unreadable correctly when we add an
> unreadable frag. I have doubt that we unset it correctly when we
> remove the last unreadable frag (or we do it correctly for other skb
> flags like pfmemalloc). If you can trust it, my LLM confirmed my
> suspicion:
> 
> ```
>   1. skb->pfmemalloc is NEVER cleared (or recomputed) when nr_frags
> becomes 0 on an sk_buff—and isn't even propagated in skb_split() /
> skb_shift():
>       • None of ___pskb_trim(), __pskb_pull_tail(), skb_split(),
> skb_shift(), skb_condense(), gro_pull_from_frag0(), or
> __pskb_trim_head() touch skb->pfmemalloc.
>       • Why it's fundamentally broken to just clear skb->pfmemalloc =
> 0 on nr_frags == 0: Unlike struct xdp_buff (which has a separate
> XDP_FLAGS_FRAGS_PF_MEMALLOC flag that BPF helpers clear via
>       xdp_buff_clear_frag_pfmemalloc() in filter.c:4441-4444 and
> filter.c:12740-12743 when !sinfo->nr_frags), struct sk_buff has a
> single sticky skb->pfmemalloc:1 bit shared by both linear skb->head
>       (skbuff.c:515-885) and frags[] (skbuff.h:2631-2632). Clearing
> skb->pfmemalloc when nr_frags == 0 would wrongly clear pfmemalloc when
> skb->head itself was allocated from PFMEMALLOC reserves.
>   2. skb->unreadable is cleared in some nr_frags -> 0 paths, but still
> missed in a few others:
>       • Unlike pfmemalloc, skb->unreadable only applies to frags[] /
> frag_list (linear skb->head is always CPU-readable).
>       • Where we DO clear skb->unreadable when nr_frags == 0:
>           • ___pskb_trim() (skbuff.c:2810-2811): if
> (!skb_shinfo(skb)->nr_frags && !skb_has_frag_list(skb))
> skb->unreadable = 0;
>           • skb_split_inside_header(), skb_split_no_header(), and
> skb_shift() (skbuff.c:4228-4454): cleared by Patch 1/2 in this series.
>       • Where nr_frags can drop to 0 and skb->unreadable is NOT cleared:
>           • pskb_carve_inside_nonlinear() (skbuff.c:7139-7167): if
> pskb_extract() carves past all frags[] (k == 0, e.g., into frag_list),
> shinfo->nr_frags = 0 on the clone without clearing skb->unreadable (if
>           !skb_has_frag_list(skb) or if the remaining frag_list is readable).
>           • __pskb_trim_head() (tcp_output.c:2020): sets
> shinfo->nr_frags = k without checking !k (in practice tcp_trim_head()
> is only called when len < skb->len and skb_headlen(skb) == 0, so k > 0
> unless a
>           frag_list is present).
>           • skb_try_coalesce() (skbuff.c:6365) and skb_gro_receive()
> (gro.c:131): zero from_shinfo->nr_frags = 0 when stealing frags
> without clearing from->unreadable (benign because from is immediately
> freed
>           via stolen-head free).
>           • __pskb_pull_tail() (skbuff.c:2894) and
> gro_pull_from_frag0() (gro.c:416): cannot be entered with
> skb->unreadable == 1 (__pskb_pull_tail() bails on
> !skb_frags_readable(skb), and
>           skb_gro_reset_offset() requires skb_frag_page(frag0) != NULL).
> ```
> 
> I already have 1 ambitious refactor on list to try to remove
> pp_recycle. I think maybe I want to do the nr_frags checks in this
> patch rather than do a global cleanup of setting skb flags correctly
> so that skb->unread is 100% accurate. WDTY?

Hard to say how real the above are and when they happen. But if they
are, let's fix them separately instead of defensively sprinkling nr_frags
checks everywhere?