linux-kernel - Re: [PATCH net-next v4 1/4] mm: add a signature in struct page

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YJyQYCj3UUk5Sp4Z@casper.infradead.org>
Date:   Thu, 13 May 2021 03:35:12 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Yunsheng Lin <linyunsheng@...wei.com>
Cc:     Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Matteo Croce <mcroce@...ux.microsoft.com>,
        Networking <netdev@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Ayush Sawal <ayush.sawal@...lsio.com>,
        Vinay Kumar Yadav <vinay.yadav@...lsio.com>,
        Rohit Maheshwari <rohitm@...lsio.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        Russell King <linux@...linux.org.uk>,
        Mirko Lindner <mlindner@...vell.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Tariq Toukan <tariqt@...dia.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        Boris Pismenny <borisp@...dia.com>,
        Arnd Bergmann <arnd@...db.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Vlastimil Babka <vbabka@...e.cz>, Yu Zhao <yuzhao@...gle.com>,
        Will Deacon <will@...nel.org>,
        Michel Lespinasse <walken@...gle.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Roman Gushchin <guro@...com>, Hugh Dickins <hughd@...gle.com>,
        Peter Xu <peterx@...hat.com>, Jason Gunthorpe <jgg@...pe.ca>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Alexander Lobakin <alobakin@...me>,
        Cong Wang <cong.wang@...edance.com>, wenxu <wenxu@...oud.cn>,
        Kevin Hao <haokexin@...il.com>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Marco Elver <elver@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Miaohe Lin <linmiaohe@...wei.com>,
        Guillaume Nault <gnault@...hat.com>,
        open list <linux-kernel@...r.kernel.org>,
        linux-rdma@...r.kernel.org, bpf <bpf@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        David Ahern <dsahern@...il.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Saeed Mahameed <saeedm@...dia.com>,
        Andrew Lunn <andrew@...n.ch>, Paolo Abeni <pabeni@...hat.com>,
        Sven Auhagen <sven.auhagen@...eatech.de>
Subject: Re: [PATCH net-next v4 1/4] mm: add a signature in struct page

On Thu, May 13, 2021 at 10:15:26AM +0800, Yunsheng Lin wrote:
> On 2021/5/12 23:57, Matthew Wilcox wrote:
> > You'll need something like this because of the current use of
> > page->index to mean "pfmemalloc".
> >
> > @@ -1682,12 +1684,12 @@ static inline bool page_is_pfmemalloc(const struct page *page)
> >   */
> >  static inline void set_page_pfmemalloc(struct page *page)
> >  {
> > -	page->index = -1UL;
> > +	page->compound_head = 2;
> 
> Is there any reason why not use "page->compound_head |= 2"? as
> corresponding to the "page->compound_head & 2" in the above
> page_is_pfmemalloc()?
> 
> Also, this may mean we need to make sure to pass head page or
> base page to set_page_pfmemalloc() if using
> "page->compound_head = 2", because it clears the bit 0 and head
> page ptr for tail page too, right?

I think what you're missing here is that this page is freshly allocated.
This is information being passed from the page allocator to any user
who cares to look at it.  By definition, it's set on the head/base page, and
there is nothing else present in the page->compound_head.  Doing an OR
is more expensive than just setting it to 2.

I'm not really sure why set/clear page_pfmemalloc are defined in mm.h.
They should probably be in mm/page_alloc.c where nobody else would ever
think that they could or should be calling them.

> >  		struct {	/* page_pool used by netstack */
> > -			/**
> > -			 * @dma_addr: might require a 64-bit value on
> > -			 * 32-bit architectures.
> > -			 */
> > +			unsigned long pp_magic;
> > +			struct page_pool *pp;
> > +			unsigned long _pp_mapping_pad;
> >  			unsigned long dma_addr[2];
> 
> It seems the dma_addr[1] aliases with page->private, and
> page_private() is used in skb_copy_ubufs()?
> 
> It seems we can avoid using page_private() in skb_copy_ubufs()
> by using a dynamic allocated array to store the page ptr?

This is why I hate it when people use page_private() instead of
documenting what they're doing in struct page.  There is no way to know
(as an outsider to networking) whether the page in skb_copy_ubufs()
comes from page_pool.  I looked at it, and thought it didn't:

                page = alloc_page(gfp_mask);

but if you say those pages can come from page_pool, I believe you.