netdev - Re: [PATCH net-next 3/7] pool_pool: avoid calling compound

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]

Date:   Thu, 23 Sep 2021 14:47:00 +0300
From:   Ilias Apalodimas <ilias.apalodimas@...aro.org>
To:     Yunsheng Lin <linyunsheng@...wei.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Networking <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        linuxarm@...neuler.org, Jesper Dangaard Brouer <hawk@...nel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Alexander Lobakin <alobakin@...me>,
        Willem de Bruijn <willemb@...gle.com>,
        Cong Wang <cong.wang@...edance.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Kevin Hao <haokexin@...il.com>,
        Aleksandr Nogikh <nogikh@...gle.com>,
        Marco Elver <elver@...gle.com>, memxor@...il.com,
        Eric Dumazet <edumazet@...gle.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH net-next 3/7] pool_pool: avoid calling compound_head() for
 skb frag page

On Thu, 23 Sept 2021 at 14:24, Yunsheng Lin <linyunsheng@...wei.com> wrote:
>
> On 2021/9/23 16:33, Ilias Apalodimas wrote:
> > On Wed, Sep 22, 2021 at 05:41:27PM +0800, Yunsheng Lin wrote:
> >> As the pp page for a skb frag is always a head page, so make
> >> sure skb_pp_recycle() passes a head page to avoid calling
> >> compound_head() for skb frag page case.
> >
> > Doesn't that rely on the driver mostly (i.e what's passed in skb_frag_set_page() ?
> > None of the current netstack code assumes bv_page is the head page of a
> > compound page.  Since our page_pool allocator can will allocate compound
> > pages for order > 0,  why should we rely on it ?
>
> As the page pool alloc function return 'struct page *' to the caller, which
> is the head page of a compound pages for order > 0, so I assume the caller
> will pass that to skb_frag_set_page().

Yea that's exactly the assumption I was afraid of.
Sure not passing the head page might seem weird atm and the assumption
stands, but the point is we shouldn't blow up the entire network stack
if someone does that eventually.

>
> For non-pp page, I assume it is ok whether the page is a head page or tail
> page, as the pp_magic for both of them are not set with PP_SIGNATURE.

Yea that's true, although we removed the checking for coalescing
recyclable and non-recyclable SKBs,   the next patch first checks the
signature before trying to do anything with the skb.

>
> Or should we play safe here, and do the trick as skb_free_head() does in
> patch 6?

I don't think the &1 will even be measurable,  so I'd suggest just
dropping this and play safe?

Cheers
/Ilias
>
> >
> > Thanks
> > /Ilias
> >>
> >> Signed-off-by: Yunsheng Lin <linyunsheng@...wei.com>
> >> ---
> >>  include/linux/skbuff.h | 2 +-
> >>  net/core/page_pool.c   | 2 --
> >>  2 files changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> >> index 6bdb0db3e825..35eebc2310a5 100644
> >> --- a/include/linux/skbuff.h
> >> +++ b/include/linux/skbuff.h
> >> @@ -4722,7 +4722,7 @@ static inline bool skb_pp_recycle(struct sk_buff *skb, void *data)
> >>  {
> >>      if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
> >>              return false;
> >> -    return page_pool_return_skb_page(virt_to_page(data));
> >> +    return page_pool_return_skb_page(virt_to_head_page(data));
> >>  }
> >>
> >>  #endif      /* __KERNEL__ */
> >> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> >> index f7e71dcb6a2e..357fb53343a0 100644
> >> --- a/net/core/page_pool.c
> >> +++ b/net/core/page_pool.c
> >> @@ -742,8 +742,6 @@ bool page_pool_return_skb_page(struct page *page)
> >>  {
> >>      struct page_pool *pp;
> >>
> >> -    page = compound_head(page);
> >> -
> >>      /* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation
> >>       * in order to preserve any existing bits, such as bit 0 for the
> >>       * head page of compound page and bit 1 for pfmemalloc page, so
> >> --
> >> 2.33.0
> >>
> > .
> >