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

[Mina Almasry]

On Thu, Nov 5, 2026 at 8:59 AM Stanislav Fomichev <sdf.kernel@xxxxxxxxx> 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.

> > +
> > +     niov = skb_frag_net_iov(&skb_shinfo(skb)->frags[0]);
> > +     if (!niov)
> > +             return NULL;
> > +
> > +     if (net_is_devmem_iov(niov))
> > +             return net_iov_owner(niov);
> > +
> > +     return niov->desc.pp;
> > +}
> > +
> > +/* Return true if the memory providers of @skb1 and @skb2 are compatible
> > + * for coalescing. For any struct page-backed provider (the buddy allocator
> > + * or a future provider handing out struct pages), coalescing is assumed
> > + * feasible. For net_iov-backed providers, the provider instance pointer
> > + * must match.
> > + */
> > +static inline bool skb_mem_provider_compat(const struct sk_buff *skb1,
> > +                                        const struct sk_buff *skb2)
> > +{
> > +     void *prov1, *prov2;
> > +
>
> [..]
>
> > +     if ((!skb1->len && !skb_shinfo(skb1)->nr_frags &&
> > +          !skb_has_net_iovs(skb1)) ||
> > +         (!skb2->len && !skb_shinfo(skb2)->nr_frags &&
> > +          !skb_has_net_iovs(skb2)))
> > +             return true;
>
> This deserves an explanation or a comment?
>

This... doesn't make sense. It guards against coalescing empty skbs.
Too defensive. I'll remove it. Thanks for catching!

> > +
> > +     if (skb_has_net_iovs(skb1) != skb_has_net_iovs(skb2))
> > +             return false;
> > +
>
> [..]
>
> > +     if (!skb_has_net_iovs(skb1))
> > +             return true;
>
> This also seems extra? The bottom prov1 == prov2 covers this?
>

This is a ninja optimization to reduce the number of cachelines being
loaded.  getting prov2 requires loading
skb_shinfo(skb2)->frags[0]->blablah, which is not loaded yet. So we
check whether skb1 has a mem-provider that we want to check against
first. I'll add a comment and/or see if I can make this prettier.


> > +static inline struct net_devmem_dmabuf_binding *
> > +net_devmem_skb_binding(const struct sk_buff *skb)
> > +{
> > +     struct net_iov *niov;
> > +
>
> [..]
>
> > +     if (!skb_has_net_iovs(skb) || !skb_shinfo(skb)->nr_frags)
> > +             return NULL;
>
> Same question here about 'why !nr_frags' is needed. Can we simplify
> (or cover this in !skb_has_net_iovs)

Yep, I think so. Thanks for the review!

-- 
Thanks,
Mina