* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-17 2:45 ` Matthew Wilcox
@ 2021-04-17 18:32 ` Ilias Apalodimas
2021-04-17 20:22 ` Matthew Wilcox
2021-04-17 21:18 ` David Laight
` (2 subsequent siblings)
3 siblings, 1 reply; 26+ messages in thread
From: Ilias Apalodimas @ 2021-04-17 18:32 UTC (permalink / raw)
To: Matthew Wilcox
Cc: brouer, linux-kernel, linux-mm, netdev, linuxppc-dev,
linux-arm-kernel, linux-mips, mcroce, grygorii.strashko, arnd,
hch, linux-snps-arc, mhocko, mgorman
Hi Matthew,
On Sat, Apr 17, 2021 at 03:45:22AM +0100, Matthew Wilcox wrote:
>
> Replacement patch to fix compiler warning.
>
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> Date: Fri, 16 Apr 2021 16:34:55 -0400
> Subject: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
> To: brouer@redhat.com
> Cc: linux-kernel@vger.kernel.org,
> linux-mm@kvack.org,
> netdev@vger.kernel.org,
> linuxppc-dev@lists.ozlabs.org,
> linux-arm-kernel@lists.infradead.org,
> linux-mips@vger.kernel.org,
> ilias.apalodimas@linaro.org,
> mcroce@linux.microsoft.com,
> grygorii.strashko@ti.com,
> arnd@kernel.org,
> hch@lst.de,
> linux-snps-arc@lists.infradead.org,
> mhocko@kernel.org,
> mgorman@suse.de
>
> 32-bit architectures which expect 8-byte alignment for 8-byte integers
> and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> page inadvertently expanded in 2019. When the dma_addr_t was added,
> it forced the alignment of the union to 8 bytes, which inserted a 4 byte
> gap between 'flags' and the union.
>
> Fix this by storing the dma_addr_t in one or two adjacent unsigned longs.
> This restores the alignment to that of an unsigned long, and also fixes a
> potential problem where (on a big endian platform), the bit used to denote
> PageTail could inadvertently get set, and a racing get_user_pages_fast()
> could dereference a bogus compound_head().
>
> Fixes: c25fff7171be ("mm: add dma_addr_t to struct page")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> include/linux/mm_types.h | 4 ++--
> include/net/page_pool.h | 12 +++++++++++-
> net/core/page_pool.c | 12 +++++++-----
> 3 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 6613b26a8894..5aacc1c10a45 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -97,10 +97,10 @@ struct page {
> };
> struct { /* page_pool used by netstack */
> /**
> - * @dma_addr: might require a 64-bit value even on
> + * @dma_addr: might require a 64-bit value on
> * 32-bit architectures.
> */
> - dma_addr_t dma_addr;
> + unsigned long dma_addr[2];
> };
> struct { /* slab, slob and slub */
> union {
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index b5b195305346..ad6154dc206c 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -198,7 +198,17 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
>
> static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
> {
> - return page->dma_addr;
> + dma_addr_t ret = page->dma_addr[0];
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16;
> + return ret;
> +}
> +
> +static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr)
> +{
> + page->dma_addr[0] = addr;
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + page->dma_addr[1] = addr >> 16 >> 16;
The 'error' that was reported will never trigger right?
I assume this was compiled with dma_addr_t as 32bits (so it triggered the
compilation error), but the if check will never allow this codepath to run.
If so can we add a comment explaining this, since none of us will remember why
in 6 months from now?
> }
>
> static inline bool is_page_pool_compiled_in(void)
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index ad8b0707af04..f014fd8c19a6 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -174,8 +174,10 @@ static void page_pool_dma_sync_for_device(struct page_pool *pool,
> struct page *page,
> unsigned int dma_sync_size)
> {
> + dma_addr_t dma_addr = page_pool_get_dma_addr(page);
> +
> dma_sync_size = min(dma_sync_size, pool->p.max_len);
> - dma_sync_single_range_for_device(pool->p.dev, page->dma_addr,
> + dma_sync_single_range_for_device(pool->p.dev, dma_addr,
> pool->p.offset, dma_sync_size,
> pool->p.dma_dir);
> }
> @@ -226,7 +228,7 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
> put_page(page);
> return NULL;
> }
> - page->dma_addr = dma;
> + page_pool_set_dma_addr(page, dma);
>
> if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
> page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
> @@ -294,13 +296,13 @@ void page_pool_release_page(struct page_pool *pool, struct page *page)
> */
> goto skip_dma_unmap;
>
> - dma = page->dma_addr;
> + dma = page_pool_get_dma_addr(page);
>
> - /* When page is unmapped, it cannot be returned our pool */
> + /* When page is unmapped, it cannot be returned to our pool */
> dma_unmap_page_attrs(pool->p.dev, dma,
> PAGE_SIZE << pool->p.order, pool->p.dma_dir,
> DMA_ATTR_SKIP_CPU_SYNC);
> - page->dma_addr = 0;
> + page_pool_set_dma_addr(page, 0);
> skip_dma_unmap:
> /* This may be the last page returned, releasing the pool, so
> * it is not safe to reference pool afterwards.
> --
> 2.30.2
>
Thanks
/Ilias
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-17 18:32 ` Ilias Apalodimas
@ 2021-04-17 20:22 ` Matthew Wilcox
2021-04-18 11:21 ` Ilias Apalodimas
0 siblings, 1 reply; 26+ messages in thread
From: Matthew Wilcox @ 2021-04-17 20:22 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: brouer, linux-kernel, linux-mm, netdev, linuxppc-dev,
linux-arm-kernel, linux-mips, mcroce, grygorii.strashko, arnd,
hch, linux-snps-arc, mhocko, mgorman
On Sat, Apr 17, 2021 at 09:32:06PM +0300, Ilias Apalodimas wrote:
> > +static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr)
> > +{
> > + page->dma_addr[0] = addr;
> > + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> > + page->dma_addr[1] = addr >> 16 >> 16;
>
> The 'error' that was reported will never trigger right?
> I assume this was compiled with dma_addr_t as 32bits (so it triggered the
> compilation error), but the if check will never allow this codepath to run.
> If so can we add a comment explaining this, since none of us will remember why
> in 6 months from now?
That's right. I compiled it all three ways -- 32-bit, 64-bit dma, 32-bit long
and 64-bit. The 32/64 bit case turn into:
if (0)
page->dma_addr[1] = addr >> 16 >> 16;
which gets elided. So the only case that has to work is 64-bit dma and
32-bit long.
I can replace this with upper_32_bits().
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-17 20:22 ` Matthew Wilcox
@ 2021-04-18 11:21 ` Ilias Apalodimas
0 siblings, 0 replies; 26+ messages in thread
From: Ilias Apalodimas @ 2021-04-18 11:21 UTC (permalink / raw)
To: Matthew Wilcox
Cc: brouer, linux-kernel, linux-mm, netdev, linuxppc-dev,
linux-arm-kernel, linux-mips, mcroce, grygorii.strashko, arnd,
hch, linux-snps-arc, mhocko, mgorman
On Sat, Apr 17, 2021 at 09:22:40PM +0100, Matthew Wilcox wrote:
> On Sat, Apr 17, 2021 at 09:32:06PM +0300, Ilias Apalodimas wrote:
> > > +static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr)
> > > +{
> > > + page->dma_addr[0] = addr;
> > > + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> > > + page->dma_addr[1] = addr >> 16 >> 16;
> >
> > The 'error' that was reported will never trigger right?
> > I assume this was compiled with dma_addr_t as 32bits (so it triggered the
> > compilation error), but the if check will never allow this codepath to run.
> > If so can we add a comment explaining this, since none of us will remember why
> > in 6 months from now?
>
> That's right. I compiled it all three ways -- 32-bit, 64-bit dma, 32-bit long
> and 64-bit. The 32/64 bit case turn into:
>
> if (0)
> page->dma_addr[1] = addr >> 16 >> 16;
>
> which gets elided. So the only case that has to work is 64-bit dma and
> 32-bit long.
>
> I can replace this with upper_32_bits().
>
Ok up to you, I don't mind either way and thanks for solving this!
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-17 2:45 ` Matthew Wilcox
2021-04-17 18:32 ` Ilias Apalodimas
@ 2021-04-17 21:18 ` David Laight
2021-04-17 22:45 ` Matthew Wilcox
2021-04-20 2:48 ` Vineet Gupta
2021-04-20 7:39 ` Geert Uytterhoeven
3 siblings, 1 reply; 26+ messages in thread
From: David Laight @ 2021-04-17 21:18 UTC (permalink / raw)
To: 'Matthew Wilcox', brouer
Cc: linux-kernel, linux-mm, netdev, linuxppc-dev, linux-arm-kernel,
linux-mips, ilias.apalodimas, mcroce, grygorii.strashko, arnd,
hch, linux-snps-arc, mhocko, mgorman
From: Matthew Wilcox <willy@infradead.org>
> Sent: 17 April 2021 03:45
>
> Replacement patch to fix compiler warning.
...
> static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
> {
> - return page->dma_addr;
> + dma_addr_t ret = page->dma_addr[0];
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16;
Ugly as well.
Why not just replace the (dma_addr_t) cast with a (u64) one?
Looks better than the double shift.
Same could be done for the '>> 32'.
Is there an upper_32_bits() that could be used??
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-17 21:18 ` David Laight
@ 2021-04-17 22:45 ` Matthew Wilcox
0 siblings, 0 replies; 26+ messages in thread
From: Matthew Wilcox @ 2021-04-17 22:45 UTC (permalink / raw)
To: David Laight
Cc: brouer, linux-kernel, linux-mm, netdev, linuxppc-dev,
linux-arm-kernel, linux-mips, ilias.apalodimas, mcroce,
grygorii.strashko, arnd, hch, linux-snps-arc, mhocko, mgorman
On Sat, Apr 17, 2021 at 09:18:57PM +0000, David Laight wrote:
> Ugly as well.
Thank you for expressing your opinion. Again.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-17 2:45 ` Matthew Wilcox
2021-04-17 18:32 ` Ilias Apalodimas
2021-04-17 21:18 ` David Laight
@ 2021-04-20 2:48 ` Vineet Gupta
2021-04-20 3:10 ` Matthew Wilcox
2021-04-20 7:39 ` Geert Uytterhoeven
3 siblings, 1 reply; 26+ messages in thread
From: Vineet Gupta @ 2021-04-20 2:48 UTC (permalink / raw)
To: Matthew Wilcox, brouer
Cc: linux-kernel, linux-mm, netdev, linuxppc-dev, linux-arm-kernel,
linux-mips, ilias.apalodimas, mcroce, grygorii.strashko, arnd,
hch, linux-snps-arc, mhocko, mgorman
Hi Matthew,
On 4/16/21 7:45 PM, Matthew Wilcox wrote:
> Replacement patch to fix compiler warning.
>
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> Date: Fri, 16 Apr 2021 16:34:55 -0400
> Subject: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
> To: brouer@redhat.com
> Cc: linux-kernel@vger.kernel.org,
> linux-mm@kvack.org,
> netdev@vger.kernel.org,
> linuxppc-dev@lists.ozlabs.org,
> linux-arm-kernel@lists.infradead.org,
> linux-mips@vger.kernel.org,
> ilias.apalodimas@linaro.org,
> mcroce@linux.microsoft.com,
> grygorii.strashko@ti.com,
> arnd@kernel.org,
> hch@lst.de,
> linux-snps-arc@lists.infradead.org,
> mhocko@kernel.org,
> mgorman@suse.de
>
> 32-bit architectures which expect 8-byte alignment for 8-byte integers
> and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> page inadvertently expanded in 2019.
FWIW, ARC doesn't require 8 byte alignment for 8 byte integers. This is
only needed for 8-byte atomics due to the requirements of LLOCKD/SCOND
instructions.
> When the dma_addr_t was added,
> it forced the alignment of the union to 8 bytes, which inserted a 4 byte
> gap between 'flags' and the union.
>
> Fix this by storing the dma_addr_t in one or two adjacent unsigned longs.
> This restores the alignment to that of an unsigned long, and also fixes a
> potential problem where (on a big endian platform), the bit used to denote
> PageTail could inadvertently get set, and a racing get_user_pages_fast()
> could dereference a bogus compound_head().
>
> Fixes: c25fff7171be ("mm: add dma_addr_t to struct page")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> include/linux/mm_types.h | 4 ++--
> include/net/page_pool.h | 12 +++++++++++-
> net/core/page_pool.c | 12 +++++++-----
> 3 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 6613b26a8894..5aacc1c10a45 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -97,10 +97,10 @@ struct page {
> };
> struct { /* page_pool used by netstack */
> /**
> - * @dma_addr: might require a 64-bit value even on
> + * @dma_addr: might require a 64-bit value on
> * 32-bit architectures.
> */
> - dma_addr_t dma_addr;
> + unsigned long dma_addr[2];
> };
> struct { /* slab, slob and slub */
> union {
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index b5b195305346..ad6154dc206c 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -198,7 +198,17 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
>
> static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
> {
> - return page->dma_addr;
> + dma_addr_t ret = page->dma_addr[0];
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16;
> + return ret;
> +}
> +
> +static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr)
> +{
> + page->dma_addr[0] = addr;
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + page->dma_addr[1] = addr >> 16 >> 16;
> }
>
> static inline bool is_page_pool_compiled_in(void)
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index ad8b0707af04..f014fd8c19a6 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -174,8 +174,10 @@ static void page_pool_dma_sync_for_device(struct page_pool *pool,
> struct page *page,
> unsigned int dma_sync_size)
> {
> + dma_addr_t dma_addr = page_pool_get_dma_addr(page);
> +
> dma_sync_size = min(dma_sync_size, pool->p.max_len);
> - dma_sync_single_range_for_device(pool->p.dev, page->dma_addr,
> + dma_sync_single_range_for_device(pool->p.dev, dma_addr,
> pool->p.offset, dma_sync_size,
> pool->p.dma_dir);
> }
> @@ -226,7 +228,7 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
> put_page(page);
> return NULL;
> }
> - page->dma_addr = dma;
> + page_pool_set_dma_addr(page, dma);
>
> if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
> page_pool_dma_sync_for_device(pool, page, pool->p.max_len);
> @@ -294,13 +296,13 @@ void page_pool_release_page(struct page_pool *pool, struct page *page)
> */
> goto skip_dma_unmap;
>
> - dma = page->dma_addr;
> + dma = page_pool_get_dma_addr(page);
>
> - /* When page is unmapped, it cannot be returned our pool */
> + /* When page is unmapped, it cannot be returned to our pool */
> dma_unmap_page_attrs(pool->p.dev, dma,
> PAGE_SIZE << pool->p.order, pool->p.dma_dir,
> DMA_ATTR_SKIP_CPU_SYNC);
> - page->dma_addr = 0;
> + page_pool_set_dma_addr(page, 0);
> skip_dma_unmap:
> /* This may be the last page returned, releasing the pool, so
> * it is not safe to reference pool afterwards.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-20 2:48 ` Vineet Gupta
@ 2021-04-20 3:10 ` Matthew Wilcox
2021-04-20 7:07 ` Arnd Bergmann
0 siblings, 1 reply; 26+ messages in thread
From: Matthew Wilcox @ 2021-04-20 3:10 UTC (permalink / raw)
To: Vineet Gupta
Cc: brouer, linux-kernel, linux-mm, netdev, linuxppc-dev,
linux-arm-kernel, linux-mips, ilias.apalodimas, mcroce,
grygorii.strashko, arnd, hch, linux-snps-arc, mhocko, mgorman
On Tue, Apr 20, 2021 at 02:48:17AM +0000, Vineet Gupta wrote:
> > 32-bit architectures which expect 8-byte alignment for 8-byte integers
> > and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> > page inadvertently expanded in 2019.
>
> FWIW, ARC doesn't require 8 byte alignment for 8 byte integers. This is
> only needed for 8-byte atomics due to the requirements of LLOCKD/SCOND
> instructions.
Ah, like x86? OK, great, I'll drop your arch from the list of
affected. Thanks!
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-20 3:10 ` Matthew Wilcox
@ 2021-04-20 7:07 ` Arnd Bergmann
2021-04-20 21:14 ` Vineet Gupta
0 siblings, 1 reply; 26+ messages in thread
From: Arnd Bergmann @ 2021-04-20 7:07 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Vineet Gupta, grygorii.strashko, netdev, ilias.apalodimas,
linux-kernel, linux-mips, mhocko, linux-mm, mgorman, brouer,
mcroce, linux-snps-arc, linuxppc-dev, hch, linux-arm-kernel
On Tue, Apr 20, 2021 at 5:10 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Apr 20, 2021 at 02:48:17AM +0000, Vineet Gupta wrote:
> > > 32-bit architectures which expect 8-byte alignment for 8-byte integers
> > > and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> > > page inadvertently expanded in 2019.
> >
> > FWIW, ARC doesn't require 8 byte alignment for 8 byte integers. This is
> > only needed for 8-byte atomics due to the requirements of LLOCKD/SCOND
> > instructions.
>
> Ah, like x86? OK, great, I'll drop your arch from the list of
> affected. Thanks!
I mistakenly assumed that i386 and m68k were the only supported
architectures with 32-bit alignment on u64. I checked it now and found
$ for i in /home/arnd/cross/x86_64/gcc-10.1.0-nolibc/*/bin/*-gcc ; do
echo `echo 'int a = __alignof__(long long);' | $i -xc - -Wall -S -o- |
grep -A1 a: | tail -n 1 | cut -f 3 -d\ `
${i#/home/arnd/cross/x86_64/gcc-10.1.0-nolibc/*/bin/} ; done
8 aarch64-linux-gcc
8 alpha-linux-gcc
4 arc-linux-gcc
8 arm-linux-gnueabi-gcc
8 c6x-elf-gcc
4 csky-linux-gcc
4 h8300-linux-gcc
8 hppa-linux-gcc
8 hppa64-linux-gcc
8 i386-linux-gcc
8 ia64-linux-gcc
2 m68k-linux-gcc
4 microblaze-linux-gcc
8 mips-linux-gcc
8 mips64-linux-gcc
8 nds32le-linux-gcc
4 nios2-linux-gcc
4 or1k-linux-gcc
8 powerpc-linux-gcc
8 powerpc64-linux-gcc
8 riscv32-linux-gcc
8 riscv64-linux-gcc
8 s390-linux-gcc
4 sh2-linux-gcc
4 sh4-linux-gcc
8 sparc-linux-gcc
8 sparc64-linux-gcc
8 x86_64-linux-gcc
8 xtensa-linux-gcc
which means that half the 32-bit architectures do this. This may
cause more problems when arc and/or microblaze want to support
64-bit kernels and compat mode in the future on their latest hardware,
as that means duplicating the x86 specific hacks we have for compat.
What is alignof(u64) on 64-bit arc?
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-20 7:07 ` Arnd Bergmann
@ 2021-04-20 21:14 ` Vineet Gupta
2021-04-20 21:20 ` Arnd Bergmann
0 siblings, 1 reply; 26+ messages in thread
From: Vineet Gupta @ 2021-04-20 21:14 UTC (permalink / raw)
To: Arnd Bergmann, Matthew Wilcox
Cc: Vineet Gupta, grygorii.strashko, netdev, ilias.apalodimas,
linux-kernel, linux-mips, mhocko, linux-mm, mgorman, brouer,
mcroce, linux-snps-arc, linuxppc-dev, hch, linux-arm-kernel
On 4/20/21 12:07 AM, Arnd Bergmann wrote:
> On Tue, Apr 20, 2021 at 5:10 AM Matthew Wilcox <willy@infradead.org> wrote:
>> On Tue, Apr 20, 2021 at 02:48:17AM +0000, Vineet Gupta wrote:
>>>> 32-bit architectures which expect 8-byte alignment for 8-byte integers
>>>> and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
>>>> page inadvertently expanded in 2019.
>>> FWIW, ARC doesn't require 8 byte alignment for 8 byte integers. This is
>>> only needed for 8-byte atomics due to the requirements of LLOCKD/SCOND
>>> instructions.
>> Ah, like x86? OK, great, I'll drop your arch from the list of
>> affected. Thanks!
> I mistakenly assumed that i386 and m68k were the only supported
> architectures with 32-bit alignment on u64. I checked it now and found
>
> $ for i in /home/arnd/cross/x86_64/gcc-10.1.0-nolibc/*/bin/*-gcc ; do
> echo `echo 'int a = __alignof__(long long);' | $i -xc - -Wall -S -o- |
> grep -A1 a: | tail -n 1 | cut -f 3 -d\ `
> ${i#/home/arnd/cross/x86_64/gcc-10.1.0-nolibc/*/bin/} ; done
> 8 aarch64-linux-gcc
> 8 alpha-linux-gcc
> 4 arc-linux-gcc
> 8 arm-linux-gnueabi-gcc
> 8 c6x-elf-gcc
> 4 csky-linux-gcc
> 4 h8300-linux-gcc
> 8 hppa-linux-gcc
> 8 hppa64-linux-gcc
> 8 i386-linux-gcc
> 8 ia64-linux-gcc
> 2 m68k-linux-gcc
> 4 microblaze-linux-gcc
> 8 mips-linux-gcc
> 8 mips64-linux-gcc
> 8 nds32le-linux-gcc
> 4 nios2-linux-gcc
> 4 or1k-linux-gcc
> 8 powerpc-linux-gcc
> 8 powerpc64-linux-gcc
> 8 riscv32-linux-gcc
> 8 riscv64-linux-gcc
> 8 s390-linux-gcc
> 4 sh2-linux-gcc
> 4 sh4-linux-gcc
> 8 sparc-linux-gcc
> 8 sparc64-linux-gcc
> 8 x86_64-linux-gcc
> 8 xtensa-linux-gcc
>
> which means that half the 32-bit architectures do this. This may
> cause more problems when arc and/or microblaze want to support
> 64-bit kernels and compat mode in the future on their latest hardware,
> as that means duplicating the x86 specific hacks we have for compat.
>
> What is alignof(u64) on 64-bit arc?
$ echo 'int a = __alignof__(long long);' | arc64-linux-gnu-gcc -xc -
-Wall -S -o - | grep -A1 a: | tail -n 1 | cut -f 3
8
Yeah ARCv2 alignment of 4 for 64-bit data was a bit of surprise finding
for me as well. When 64-bit load/stores were initially targeted by the
internal Metaware compiler (llvm based) they decided to keep alignment
to 4 still (granted hardware allowed this) and then gcc guys decided to
follow the same ABI. I only found this by accident :-)
Can you point me to some specifics on the compat issue. For better of
worse, arc64 does''t have a compat 32-bit mode, so everything is
64-on-64 or 32-on-32 (ARC32 flavor of ARCv3)
Thx,
-Vineet
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-20 21:14 ` Vineet Gupta
@ 2021-04-20 21:20 ` Arnd Bergmann
2021-04-21 5:50 ` hch
2021-04-21 8:43 ` David Laight
0 siblings, 2 replies; 26+ messages in thread
From: Arnd Bergmann @ 2021-04-20 21:20 UTC (permalink / raw)
To: Vineet Gupta
Cc: Matthew Wilcox, grygorii.strashko, netdev, ilias.apalodimas,
linux-kernel, linux-mips, mhocko, linux-mm, mgorman, brouer,
mcroce, linux-snps-arc, linuxppc-dev, hch, linux-arm-kernel
On Tue, Apr 20, 2021 at 11:14 PM Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
> On 4/20/21 12:07 AM, Arnd Bergmann wrote:
> >
> > which means that half the 32-bit architectures do this. This may
> > cause more problems when arc and/or microblaze want to support
> > 64-bit kernels and compat mode in the future on their latest hardware,
> > as that means duplicating the x86 specific hacks we have for compat.
> >
> > What is alignof(u64) on 64-bit arc?
>
> $ echo 'int a = __alignof__(long long);' | arc64-linux-gnu-gcc -xc -
> -Wall -S -o - | grep -A1 a: | tail -n 1 | cut -f 3
> 8
Ok, good.
> Yeah ARCv2 alignment of 4 for 64-bit data was a bit of surprise finding
> for me as well. When 64-bit load/stores were initially targeted by the
> internal Metaware compiler (llvm based) they decided to keep alignment
> to 4 still (granted hardware allowed this) and then gcc guys decided to
> follow the same ABI. I only found this by accident :-)
>
> Can you point me to some specifics on the compat issue. For better of
> worse, arc64 does''t have a compat 32-bit mode, so everything is
> 64-on-64 or 32-on-32 (ARC32 flavor of ARCv3)
In that case, there should be no problem for you.
The main issue is with system calls and ioctls that contain a misaligned
struct member like
struct s {
u32 a;
u64 b;
};
Passing this structure by reference from a 32-bit user space application
to a 64-bit kernel with different alignment constraints means that the
kernel has to convert the structure layout. See
compat_ioctl_preallocate() in fs/ioctl.c for one such example.
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-20 21:20 ` Arnd Bergmann
@ 2021-04-21 5:50 ` hch
2021-04-21 8:43 ` David Laight
1 sibling, 0 replies; 26+ messages in thread
From: hch @ 2021-04-21 5:50 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Vineet Gupta, Matthew Wilcox, grygorii.strashko, netdev,
ilias.apalodimas, linux-kernel, linux-mips, mhocko, linux-mm,
mgorman, brouer, mcroce, linux-snps-arc, linuxppc-dev, hch,
linux-arm-kernel
On Tue, Apr 20, 2021 at 11:20:19PM +0200, Arnd Bergmann wrote:
> In that case, there should be no problem for you.
>
> The main issue is with system calls and ioctls that contain a misaligned
> struct member like
>
> struct s {
> u32 a;
> u64 b;
> };
>
> Passing this structure by reference from a 32-bit user space application
> to a 64-bit kernel with different alignment constraints means that the
> kernel has to convert the structure layout. See
> compat_ioctl_preallocate() in fs/ioctl.c for one such example.
We've also had this problem with some on-disk structures in the past,
but hopefully people desining those have learnt the lesson by now.
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-20 21:20 ` Arnd Bergmann
2021-04-21 5:50 ` hch
@ 2021-04-21 8:43 ` David Laight
2021-04-21 8:58 ` Arnd Bergmann
1 sibling, 1 reply; 26+ messages in thread
From: David Laight @ 2021-04-21 8:43 UTC (permalink / raw)
To: 'Arnd Bergmann', Vineet Gupta
Cc: Matthew Wilcox, grygorii.strashko, netdev, ilias.apalodimas,
linux-kernel, linux-mips, mhocko, linux-mm, mgorman, brouer,
mcroce, linux-snps-arc, linuxppc-dev, hch, linux-arm-kernel
From: Arnd Bergmann
> Sent: 20 April 2021 22:20
>
> On Tue, Apr 20, 2021 at 11:14 PM Vineet Gupta
> <Vineet.Gupta1@synopsys.com> wrote:
> > On 4/20/21 12:07 AM, Arnd Bergmann wrote:
>
> > >
> > > which means that half the 32-bit architectures do this. This may
> > > cause more problems when arc and/or microblaze want to support
> > > 64-bit kernels and compat mode in the future on their latest hardware,
> > > as that means duplicating the x86 specific hacks we have for compat.
> > >
> > > What is alignof(u64) on 64-bit arc?
> >
> > $ echo 'int a = __alignof__(long long);' | arc64-linux-gnu-gcc -xc -
> > -Wall -S -o - | grep -A1 a: | tail -n 1 | cut -f 3
> > 8
>
> Ok, good.
That test doesn't prove anything.
Try running on x86:
$ echo 'int a = __alignof__(long long);' | gcc -xc - -Wall -S -o - -m32
.file ""
.globl a
.data
.align 4
.type a, @object
.size a, 4
a:
.long 8
.ident "GCC: (Ubuntu 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609"
.section .note.GNU-stack,"",@progbits
Using '__alignof__(struct {long long x;})' does give the expected 4.
__alignof__() returns the preferred alignment, not the enforced
alignmnet - go figure.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-21 8:43 ` David Laight
@ 2021-04-21 8:58 ` Arnd Bergmann
0 siblings, 0 replies; 26+ messages in thread
From: Arnd Bergmann @ 2021-04-21 8:58 UTC (permalink / raw)
To: David Laight
Cc: Vineet Gupta, Matthew Wilcox, grygorii.strashko, netdev,
ilias.apalodimas, linux-kernel, linux-mips, mhocko, linux-mm,
mgorman, brouer, mcroce, linux-snps-arc, linuxppc-dev, hch,
linux-arm-kernel
On Wed, Apr 21, 2021 at 10:43 AM David Laight <David.Laight@aculab.com> wrote:
> From: Arnd Bergmann Sent: 20 April 2021 22:20
> > On Tue, Apr 20, 2021 at 11:14 PM Vineet Gupta <Vineet.Gupta1@synopsys.com> wrote:
> > > On 4/20/21 12:07 AM, Arnd Bergmann wrote:
> >
> > > >
> > > > which means that half the 32-bit architectures do this. This may
> > > > cause more problems when arc and/or microblaze want to support
> > > > 64-bit kernels and compat mode in the future on their latest hardware,
> > > > as that means duplicating the x86 specific hacks we have for compat.
> > > >
> > > > What is alignof(u64) on 64-bit arc?
> > >
> > > $ echo 'int a = __alignof__(long long);' | arc64-linux-gnu-gcc -xc -
> > > -Wall -S -o - | grep -A1 a: | tail -n 1 | cut -f 3
> > > 8
> >
> > Ok, good.
>
> That test doesn't prove anything.
> Try running on x86:
> $ echo 'int a = __alignof__(long long);' | gcc -xc - -Wall -S -o - -m32
> a:
> .long 8
Right, I had wondered about that one after I sent the email.
> Using '__alignof__(struct {long long x;})' does give the expected 4.
>
> __alignof__() returns the preferred alignment, not the enforced
> alignmnet - go figure.
I checked the others as well now, and i386 is the only one that
changed here: m68k still has '2', while arc/csky/h8300/microblaze/
nios2/or1k/sh/i386 all have '4' and the rest have '8'.
Arnd
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-17 2:45 ` Matthew Wilcox
` (2 preceding siblings ...)
2021-04-20 2:48 ` Vineet Gupta
@ 2021-04-20 7:39 ` Geert Uytterhoeven
2021-04-20 8:39 ` David Laight
2021-04-20 11:32 ` Matthew Wilcox
3 siblings, 2 replies; 26+ messages in thread
From: Geert Uytterhoeven @ 2021-04-20 7:39 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Jesper Dangaard Brouer, Linux Kernel Mailing List, Linux MM,
netdev, linuxppc-dev, Linux ARM, open list:BROADCOM NVRAM DRIVER,
Ilias Apalodimas, mcroce, Grygorii Strashko, Arnd Bergmann,
Christoph Hellwig, arcml, Michal Hocko, Mel Gorman
Hi Willy,
On Sat, Apr 17, 2021 at 4:49 AM Matthew Wilcox <willy@infradead.org> wrote:
> Replacement patch to fix compiler warning.
>
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> Date: Fri, 16 Apr 2021 16:34:55 -0400
> Subject: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
> To: brouer@redhat.com
> Cc: linux-kernel@vger.kernel.org,
> linux-mm@kvack.org,
> netdev@vger.kernel.org,
> linuxppc-dev@lists.ozlabs.org,
> linux-arm-kernel@lists.infradead.org,
> linux-mips@vger.kernel.org,
> ilias.apalodimas@linaro.org,
> mcroce@linux.microsoft.com,
> grygorii.strashko@ti.com,
> arnd@kernel.org,
> hch@lst.de,
> linux-snps-arc@lists.infradead.org,
> mhocko@kernel.org,
> mgorman@suse.de
>
> 32-bit architectures which expect 8-byte alignment for 8-byte integers
> and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> page inadvertently expanded in 2019. When the dma_addr_t was added,
> it forced the alignment of the union to 8 bytes, which inserted a 4 byte
> gap between 'flags' and the union.
>
> Fix this by storing the dma_addr_t in one or two adjacent unsigned longs.
> This restores the alignment to that of an unsigned long, and also fixes a
> potential problem where (on a big endian platform), the bit used to denote
> PageTail could inadvertently get set, and a racing get_user_pages_fast()
> could dereference a bogus compound_head().
>
> Fixes: c25fff7171be ("mm: add dma_addr_t to struct page")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Thanks for your patch!
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -97,10 +97,10 @@ struct page {
> };
> struct { /* page_pool used by netstack */
> /**
> - * @dma_addr: might require a 64-bit value even on
> + * @dma_addr: might require a 64-bit value on
> * 32-bit architectures.
> */
> - dma_addr_t dma_addr;
> + unsigned long dma_addr[2];
So we get two 64-bit words on 64-bit platforms, while only one is
needed?
Would
unsigned long _dma_addr[sizeof(dma_addr_t) / sizeof(unsigned long)];
work?
Or will the compiler become too overzealous, and warn about the use
of ...[1] below, even when unreachable?
I wouldn't mind an #ifdef instead of an if () in the code below, though.
> };
> struct { /* slab, slob and slub */
> union {
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index b5b195305346..ad6154dc206c 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -198,7 +198,17 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
>
> static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
> {
> - return page->dma_addr;
> + dma_addr_t ret = page->dma_addr[0];
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16;
We don't seem to have a handy macro for a 32-bit left shift yet...
But you can also avoid the warning using
ret |= (u64)page->dma_addr[1] << 32;
> + return ret;
> +}
> +
> +static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr)
> +{
> + page->dma_addr[0] = addr;
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + page->dma_addr[1] = addr >> 16 >> 16;
... but we do have upper_32_bits() for a 32-bit right shift.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-20 7:39 ` Geert Uytterhoeven
@ 2021-04-20 8:39 ` David Laight
2021-04-20 11:32 ` Matthew Wilcox
1 sibling, 0 replies; 26+ messages in thread
From: David Laight @ 2021-04-20 8:39 UTC (permalink / raw)
To: 'Geert Uytterhoeven', Matthew Wilcox
Cc: Jesper Dangaard Brouer, Linux Kernel Mailing List, Linux MM,
netdev, linuxppc-dev, Linux ARM, open list:BROADCOM NVRAM DRIVER,
Ilias Apalodimas, mcroce, Grygorii Strashko, Arnd Bergmann,
Christoph Hellwig, arcml, Michal Hocko, Mel Gorman
From: Geert Uytterhoeven
> Sent: 20 April 2021 08:40
>
> Hi Willy,
>
> On Sat, Apr 17, 2021 at 4:49 AM Matthew Wilcox <willy@infradead.org> wrote:
> > Replacement patch to fix compiler warning.
> >
> > 32-bit architectures which expect 8-byte alignment for 8-byte integers
> > and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> > page inadvertently expanded in 2019. When the dma_addr_t was added,
> > it forced the alignment of the union to 8 bytes, which inserted a 4 byte
> > gap between 'flags' and the union.
> >
> > Fix this by storing the dma_addr_t in one or two adjacent unsigned longs.
> > This restores the alignment to that of an unsigned long, and also fixes a
> > potential problem where (on a big endian platform), the bit used to denote
> > PageTail could inadvertently get set, and a racing get_user_pages_fast()
> > could dereference a bogus compound_head().
> >
> > Fixes: c25fff7171be ("mm: add dma_addr_t to struct page")
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>
> Thanks for your patch!
>
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -97,10 +97,10 @@ struct page {
> > };
> > struct { /* page_pool used by netstack */
> > /**
> > - * @dma_addr: might require a 64-bit value even on
> > + * @dma_addr: might require a 64-bit value on
> > * 32-bit architectures.
> > */
> > - dma_addr_t dma_addr;
> > + unsigned long dma_addr[2];
>
> So we get two 64-bit words on 64-bit platforms, while only one is
> needed?
>
> Would
>
> unsigned long _dma_addr[sizeof(dma_addr_t) / sizeof(unsigned long)];
>
> work?
>
> Or will the compiler become too overzealous, and warn about the use
> of ...[1] below, even when unreachable?
> I wouldn't mind an #ifdef instead of an if () in the code below, though.
You could use [ARRAY_SIZE()-1] instead of [1].
Or, since IIRC it is the last member of that specific struct, define as:
unsigned long dma_addr[];
...
> > - return page->dma_addr;
> > + dma_addr_t ret = page->dma_addr[0];
> > + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> > + ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16;
>
> We don't seem to have a handy macro for a 32-bit left shift yet...
>
> But you can also avoid the warning using
>
> ret |= (u64)page->dma_addr[1] << 32;
Or:
ret |= page->dma_addr[1] + 0ull << 32;
Which relies in integer promotion rather than a cast.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
2021-04-20 7:39 ` Geert Uytterhoeven
2021-04-20 8:39 ` David Laight
@ 2021-04-20 11:32 ` Matthew Wilcox
1 sibling, 0 replies; 26+ messages in thread
From: Matthew Wilcox @ 2021-04-20 11:32 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jesper Dangaard Brouer, Linux Kernel Mailing List, Linux MM,
netdev, linuxppc-dev, Linux ARM, open list:BROADCOM NVRAM DRIVER,
Ilias Apalodimas, mcroce, Grygorii Strashko, Arnd Bergmann,
Christoph Hellwig, arcml, Michal Hocko, Mel Gorman
On Tue, Apr 20, 2021 at 09:39:54AM +0200, Geert Uytterhoeven wrote:
> > +++ b/include/linux/mm_types.h
> > @@ -97,10 +97,10 @@ struct page {
> > };
> > struct { /* page_pool used by netstack */
> > /**
> > - * @dma_addr: might require a 64-bit value even on
> > + * @dma_addr: might require a 64-bit value on
> > * 32-bit architectures.
> > */
> > - dma_addr_t dma_addr;
> > + unsigned long dma_addr[2];
>
> So we get two 64-bit words on 64-bit platforms, while only one is
> needed?
Not really. This is part of the 5-word union in struct page, so the space
ends up being reserved anyway, event if it's not "assigned" to dma_addr.
> > + dma_addr_t ret = page->dma_addr[0];
> > + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> > + ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16;
>
> We don't seem to have a handy macro for a 32-bit left shift yet...
>
> But you can also avoid the warning using
>
> ret |= (u64)page->dma_addr[1] << 32;
Sure. It doesn't really matter which way we eliminate the warning;
the code is unreachable.
> > +{
> > + page->dma_addr[0] = addr;
> > + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> > + page->dma_addr[1] = addr >> 16 >> 16;
>
> ... but we do have upper_32_bits() for a 32-bit right shift.
Yep, that's what my current tree looks like.
^ permalink raw reply [flat|nested] 26+ messages in thread