linux-kernel - Re: [PATCH net-next v6 2/2] net: add netmem to skb_frag

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Thu, 1 Feb 2024 12:45:37 -0800
From: Mina Almasry <almasrymina@...gle.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: linux-kernel@...r.kernel.org, netdev@...r.kernel.org, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Jason Gunthorpe <jgg@...dia.com>, 
	Christian König <christian.koenig@....com>, 
	Shakeel Butt <shakeelb@...gle.com>, Yunsheng Lin <linyunsheng@...wei.com>, 
	Willem de Bruijn <willemdebruijn.kernel@...il.com>
Subject: Re: [PATCH net-next v6 2/2] net: add netmem to skb_frag_t

On Tue, Jan 30, 2024 at 1:34 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> Hi,
>
> I'm sorry for the late feedback.
>

Thanks for looking.

> On Tue, 2024-01-23 at 14:17 -0800, Mina Almasry wrote:
> > @@ -845,16 +863,24 @@ struct sk_buff *__napi_alloc_skb(struct napi_struct *napi, unsigned int len,
> >  }
> >  EXPORT_SYMBOL(__napi_alloc_skb);
> >
> > -void skb_add_rx_frag(struct sk_buff *skb, int i, struct page *page, int off,
> > -                  int size, unsigned int truesize)
> > +void skb_add_rx_frag_netmem(struct sk_buff *skb, int i, netmem_ref netmem,
> > +                         int off, int size, unsigned int truesize)
> >  {
> >       DEBUG_NET_WARN_ON_ONCE(size > truesize);
> >
> > -     skb_fill_page_desc(skb, i, page, off, size);
> > +     skb_fill_netmem_desc(skb, i, netmem, off, size);
> >       skb->len += size;
> >       skb->data_len += size;
> >       skb->truesize += truesize;
> >  }
> > +EXPORT_SYMBOL(skb_add_rx_frag_netmem);
> > +
> > +void skb_add_rx_frag(struct sk_buff *skb, int i, struct page *page, int off,
> > +                  int size, unsigned int truesize)
> > +{
> > +     skb_add_rx_frag_netmem(skb, i, page_to_netmem(page), off, size,
> > +                            truesize);
> > +}
> >  EXPORT_SYMBOL(skb_add_rx_frag);
>
> Out of sheer ignorance, I'm unsure if the compiler will always inline
> the above skb_add_rx_frag_netmem() call. What about moving this helper
> to the header file?
>

Will do.

> > diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
> > index 1184d40167b8..145ef22b2b35 100644
> > --- a/net/kcm/kcmsock.c
> > +++ b/net/kcm/kcmsock.c
> > @@ -636,9 +636,14 @@ static int kcm_write_msgs(struct kcm_sock *kcm)
> >               for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> >                       msize += skb_frag_size(&skb_shinfo(skb)->frags[i]);
> >
> > +             if (WARN_ON_ONCE(!skb_frag_page(&skb_shinfo(skb)->frags[0]))) {
> > +                     ret = -EINVAL;
> > +                     goto out;
> > +             }
>
> I feel like the following has been already discussed, but I could not
> find the relevant reference... Are all frags constrained to carry the
> same memref type? If not it would be better to move this check inside
> the previous loop, it's already traversing all the skb frags, it should
> not add measurable overhead.
>

Yes, this was discussed before. I believe the agreement is that, yes,
all the frags in a single skb will be constrained to a single type. It
was discussed on one of the many RFCs I believe.

Supporting skbs with mixed netmem types is certainly possible, but
requires per-frag checking and per-frag handling. Constraining all
skbs to the same netmem type just simplifies things greatly because
frag0 can be checked to determine the type of all the frags in the
skb, and all the frags in the skb can be processed the same as they're
the same type. There are no interesting use cases I can think of right
now that require mixed types, and the code can always be extended to
that if someone has a use case in the future.

I plan to add a WARN_ON_ONCE or DEBUG_NET_WARN_ON_ONCE in
skb_add_frag_rx_netmem that detects if the driver is trying to mix
types in the devmem series which adds non-page netmem.

If OK with you, I'll keep the check for only frag 0, but combine it
with the nr_frags check above like this:

diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index 145ef22b2b35..73c200c5c8e4 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -627,7 +627,8 @@ static int kcm_write_msgs(struct kcm_sock *kcm)
                        skb = txm->frag_skb;
                }

-               if (WARN_ON(!skb_shinfo(skb)->nr_frags)) {
+               if (WARN_ON(!skb_shinfo(skb)->nr_frags) ||
+                   WARN_ON_ONCE(!skb_frag_page(&skb_shinfo(skb)->frags[0]))) {
                        ret = -EINVAL;
                        goto out;
                }
@@ -636,11 +637,6 @@ static int kcm_write_msgs(struct kcm_sock *kcm)
                for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
                        msize += skb_frag_size(&skb_shinfo(skb)->frags[i]);

-               if (WARN_ON_ONCE(!skb_frag_page(&skb_shinfo(skb)->frags[0]))) {
-                       ret = -EINVAL;
-                       goto out;
-               }
-

But I'm happy implementing the check exactly as you described if you
strongly prefer that instead, I don't think it's a big deal from my
end either way. Thanks!

-- 
Thanks,
Mina