netdev - Re: [PATCH net-next 1/4] net: allow alloc_skb_with

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iJwP_Ar57Te0EG2fAjM=JNL+N0mYwnEZDrJME4nhe4WTg@mail.gmail.com>
Date: Tue, 1 Aug 2023 18:33:13 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Willem de Bruijn <willemb@...gle.com>, 
	Tahsin Erdogan <trdgn@...zon.com>, netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net-next 1/4] net: allow alloc_skb_with_frags() to
 allocate bigger packets

On Tue, Aug 1, 2023 at 5:44 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Eric Dumazet wrote:
> > Refactor alloc_skb_with_frags() to allow bigger packets allocations.
> >
> > Instead of assuming that only order-0 allocations will be attempted,
> > use the caller supplied max order.
> >
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > Cc: Tahsin Erdogan <trdgn@...zon.com>
> > ---
> >  net/core/skbuff.c | 56 +++++++++++++++++++++--------------------------
> >  1 file changed, 25 insertions(+), 31 deletions(-)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index a298992060e6efdecb87c7ffc8290eafe330583f..0ac70a0144a7c1f4e7824ddc19980aee73e4c121 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -6204,7 +6204,7 @@ EXPORT_SYMBOL_GPL(skb_mpls_dec_ttl);
> >   *
> >   * @header_len: size of linear part
> >   * @data_len: needed length in frags
> > - * @max_page_order: max page order desired.
> > + * @order: max page order desired.
> >   * @errcode: pointer to error code if any
> >   * @gfp_mask: allocation mask
> >   *
> > @@ -6212,21 +6212,17 @@ EXPORT_SYMBOL_GPL(skb_mpls_dec_ttl);
> >   */
> >  struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
> >                                    unsigned long data_len,
> > -                                  int max_page_order,
> > +                                  int order,
> >                                    int *errcode,
> >                                    gfp_t gfp_mask)
> >  {
> > -     int npages = (data_len + (PAGE_SIZE - 1)) >> PAGE_SHIFT;
> >       unsigned long chunk;
> >       struct sk_buff *skb;
> >       struct page *page;
> > -     int i;
> > +     int nr_frags = 0;
> >
> >       *errcode = -EMSGSIZE;
> > -     /* Note this test could be relaxed, if we succeed to allocate
> > -      * high order pages...
> > -      */
> > -     if (npages > MAX_SKB_FRAGS)
> > +     if (unlikely(data_len > MAX_SKB_FRAGS * (PAGE_SIZE << order)))
> >               return NULL;
> >
> >       *errcode = -ENOBUFS;
> > @@ -6234,34 +6230,32 @@ struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
> >       if (!skb)
> >               return NULL;
> >
> > -     skb->truesize += npages << PAGE_SHIFT;
> > -
> > -     for (i = 0; npages > 0; i++) {
> > -             int order = max_page_order;
> > -
> > -             while (order) {
> > -                     if (npages >= 1 << order) {
> > -                             page = alloc_pages((gfp_mask & ~__GFP_DIRECT_RECLAIM) |
> > -                                                __GFP_COMP |
> > -                                                __GFP_NOWARN,
> > -                                                order);
> > -                             if (page)
> > -                                     goto fill_page;
> > -                             /* Do not retry other high order allocations */
>
> Is this heuristic to only try one type of compound pages and else
> fall back onto regular pages still relevant? I don't know the story
> behind it.

I keep doing high-order attempts without direct reclaim,
they should be fine and we eventually fallback to order-2 pages
if we have plenty of them.

Immediate fallback to order-0 seems pessimistic.

>
> > -                             order = 1;
> > -                             max_page_order = 0;
> > -                     }
> > +     while (data_len) {
> > +             if (nr_frags == MAX_SKB_FRAGS - 1)
> > +                     goto failure;
> > +             while (order && data_len < (PAGE_SIZE << order))
> >                       order--;
>
> Why decrement order on every iteration through the loop, not just when
> alloc_pages fails?

Say we enter the function with initial @data_len == 4000, and @order==3

We do not want to allocate/waste an order-3 page (32768 bytes on x86)
while an order-0 one should be good enough to fit the expected
payload.

Same story if initial data_len = 33000:
- We should allocate one order-3 page, and one order-0 one, instead of
two order-3 pages.