linux-kernel - Re: linux-next: build failure after merge of the net-next tree

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]

Message-ID: <20240913132438.4235abe4@kernel.org>
Date: Fri, 13 Sep 2024 13:24:38 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Matthew Wilcox <willy@...radead.org>, Stephen Rothwell
 <sfr@...b.auug.org.au>, christophe.leroy2@...soprasteria.com, David Miller
 <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>, Networking
 <netdev@...r.kernel.org>, Linux Kernel Mailing List
 <linux-kernel@...r.kernel.org>, Linux Next Mailing List
 <linux-next@...r.kernel.org>, Arnd Bergmann <arnd@...db.de>,
 "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: linux-next: build failure after merge of the net-next tree

On Fri, 13 Sep 2024 13:05:32 -0700 Mina Almasry wrote:
> Change, got NAKed:
> https://lore.kernel.org/netdev/ZuSQ9BT9Vg7O2kXv@casper.infradead.org/

Humpf.

> But AFAICT we don't really need to do this inside of mm, affecting
> things like compound_head. This equivalent change also makes the build
> pass. Does this look good?
> 
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index 8a6e20be4b9d..58f2120cd392 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -100,7 +100,15 @@ static inline netmem_ref net_iov_to_netmem(struct
> net_iov *niov)
> 
>  static inline netmem_ref page_to_netmem(struct page *page)
>  {
> -       return (__force netmem_ref)page;
> +       /* page* exported from the mm stack would not have the LSB set, but the
> +        * GCC 14 powerpc compiler will optimize reads into this pointer into
> +        * unaligned reads as it sees address arthemetic in _compound_head().
> +        *
> +        * Explicitly clear the LSB until what looks like a GCC compiler issue
> +        * is resolved.
> +        */
> +       DEBUG_NET_WARN_ON_ONCE((unsigned long)page & 1UL);
> +       return (__force netmem_ref)page & ~1UL;
>  }

Hmm. Not really, the math this is doing is a bit of a cargo cult,
AFAIU the operation itself is meaningless. It works because it
achieves breaking the optimization/register chain in the compiler.
But the exact ALU op doesn't matter. So pretending LSB is meaningful 
could be confusing to the reader.

I think this will achieve the same effect without the spurious ALU
operations (apologies for broken whitespace):

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index a813d30d2135..b7e0acaed933 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -864,7 +864,11 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
        allow_direct = page_pool_napi_local(pool);
 
        for (i = 0; i < count; i++) {
-               netmem_ref netmem = page_to_netmem(virt_to_head_page(data[i]));
+               struct page *page = virt_to_head_page(data[i]);
+               netmem_ref netmem;
+
+               /* $explanation */
+               netmem = page_to_netmem(READ_ONCE(page));
 
                /* It is not the last user for the page frag case */
                if (!page_pool_is_last_ref(netmem))

If it makes sense could you polish it up and submit?