x86/mm: Check if PTRS_PER_PMD is defined before use

[tip: x86/mm] x86/mm: Check if PTRS_PER_PMD is defined before use

Posted by tip-bot2 for Andy Shevchenko 11 months, 1 week ago

The following commit has been merged into the x86/mm branch of tip:

Commit-ID:     74a6c5103f8c27df056227e9e3bfe1ffeb7fc3f3
Gitweb:        https://git.kernel.org/tip/74a6c5103f8c27df056227e9e3bfe1ffeb7fc3f3
Author:        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
AuthorDate:    Thu, 06 Mar 2025 11:25:41 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Thu, 06 Mar 2025 11:03:59 +01:00

x86/mm: Check if PTRS_PER_PMD is defined before use

Compiler (GCC) is not happy about PTRS_PER_PMD being undefined
(note, clang also issues the quite similar warning):

  In file included from arch/x86/kernel/head_32.S:29:
  arch/x86/include/asm/pgtable_32.h:59:5: error: "PTRS_PER_PMD" is not defined, evaluates to 0 [-Werror=undef]
     59 | #if PTRS_PER_PMD > 1

Add a check to make sure PTRS_PER_PMD is defined before use.

The documentation for GCC 7.5.0+ says that:

	if defined BUFSIZE && BUFSIZE >= 1024

    can generally be simplified to just #if BUFSIZE >= 1024, since if BUFSIZE
    is not defined, it will be interpreted as having the value zero.

But in the same time the last paragraph points out that

    It will warn wherever your code uses this feature.

which is what we met here.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/r/20250306092658.378837-1-andriy.shevchenko@linux.intel.com

Closes: https://lore.kernel.org/r/202412152358.l9RJiVaH-lkp@intel.com/
---
 arch/x86/include/asm/pgtable_32.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/pgtable_32.h b/arch/x86/include/asm/pgtable_32.h
index 7d4ad89..3c05235 100644
--- a/arch/x86/include/asm/pgtable_32.h
+++ b/arch/x86/include/asm/pgtable_32.h
@@ -56,7 +56,7 @@ do {						\
  * With PAE paging (PTRS_PER_PMD > 1), we allocate PTRS_PER_PGD == 4 pages for
  * the PMD's in addition to the pages required for the last level pagetables.
  */
-#if PTRS_PER_PMD > 1
+#if defined(PTRS_PER_PMD) && (PTRS_PER_PMD > 1)
 #define PAGE_TABLE_SIZE(pages) (((pages) / PTRS_PER_PMD) + PTRS_PER_PGD)
 #else
 #define PAGE_TABLE_SIZE(pages) ((pages) / PTRS_PER_PGD)

Re: [tip: x86/mm] x86/mm: Check if PTRS_PER_PMD is defined before use

Posted by Linus Torvalds 11 months, 1 week ago

On Thu, 6 Mar 2025 at 00:13, tip-bot2 for Andy Shevchenko
<tip-bot2@linutronix.de> wrote:
>
> x86/mm: Check if PTRS_PER_PMD is defined before use

I'm not at all happy with this one.

> -#if PTRS_PER_PMD > 1
> +#if defined(PTRS_PER_PMD) && (PTRS_PER_PMD > 1)

Honestly, I feel that if PTRS_PER_PMD isn't defined, we've missed some
include, and now the code is making random decisions based on lack of
information.

That's not correct. You can't say "I don't know the size, so I'm just
assuming it's 1".

It should always be defined, because it's normally used
unconditionally (just grep for it in mm code, eg mm/pagewalk.c:
real_depth()).

So the undefined case really is broken.

It should be defined either by the architecture pgtable_types.h
header, or if the PMD is folded away, the architecture should have
included <asm-generic/pgtable-nopmd.h>.

So I'm *really* thinking this patch is completely bogus and is hiding
a serious problem, and making PAGE_TABLE_SIZE() have random values.

                Linus

Re: [tip: x86/mm] x86/mm: Check if PTRS_PER_PMD is defined before use

Posted by Ingo Molnar 11 months, 1 week ago

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 6 Mar 2025 at 00:13, tip-bot2 for Andy Shevchenko
> <tip-bot2@linutronix.de> wrote:
> >
> > x86/mm: Check if PTRS_PER_PMD is defined before use
> 
> I'm not at all happy with this one.
> 
> > -#if PTRS_PER_PMD > 1
> > +#if defined(PTRS_PER_PMD) && (PTRS_PER_PMD > 1)
> 
> Honestly, I feel that if PTRS_PER_PMD isn't defined, we've missed some
> include, and now the code is making random decisions based on lack of
> information.

Yeah, so <asm/pgtable-2level_types.h> hasn't defined it historically, 
because 2-level paging only has PGDs and PTE tables - and it relies on 
<asm-generic/pgtable-nopmd.h> doing it:

 #define PTRS_PER_PMD    1

<asm/pgtable_types.h> includes <asm-generic/pgtable-nopmd.h>, and with 
it most of the MM headers.

But:

> It should be defined either by the architecture pgtable_types.h
> header, or if the PMD is folded away, the architecture should have
> included <asm-generic/pgtable-nopmd.h>.
> 
> So I'm *really* thinking this patch is completely bogus and is hiding
> a serious problem, and making PAGE_TABLE_SIZE() have random values.

Yeah, so the MM headers cover the C case - but the bugreport was about 
the assembly side (head_32.S):

  In file included from arch/x86/kernel/head_32.S:29:
  arch/x86/include/asm/pgtable_32.h:59:5: error: "PTRS_PER_PMD" is not defined, evaluates to 0 [-Werror=undef]
     59 | #if PTRS_PER_PMD > 1

and AFAICS the assembly version of these headers doesn't define 
PTRS_PER_PMD.

Separating out the assembler-compatible defines from the types headers 
appears to be a bigger patch, since it's all mixed in with C syntax:

<=-----------------------------------===============================
typedef struct { pud_t pud; } pmd_t;

#define PMD_SHIFT       PUD_SHIFT
#define PTRS_PER_PMD    1
#define PMD_SIZE        (1UL << PMD_SHIFT)
#define PMD_MASK        (~(PMD_SIZE-1))

/*
 * The "pud_xxx()" functions here are trivial for a folded two-level
 * setup: the pmd is never bad, and a pmd always exists (as it's folded
 * into the pud entry)
 */
static inline int pud_none(pud_t pud)           { return 0; }
static inline int pud_bad(pud_t pud)            { return 0; }
static inline int pud_present(pud_t pud)        { return 1; }
================================================================>

In any case I've removed the commit for the time being until this all 
is cleared up.

Thanks,

	Ingo

[PATCH] x86/mm: Define PTRS_PER_PMD for assembly code too

Posted by Ingo Molnar 11 months, 1 week ago


* Ingo Molnar <mingo@kernel.org> wrote:

> Separating out the assembler-compatible defines from the types 
> headers appears to be a bigger patch, since it's all mixed in with C 
> syntax:
> 
> <=-----------------------------------===============================
> typedef struct { pud_t pud; } pmd_t;
> 
> #define PMD_SHIFT       PUD_SHIFT
> #define PTRS_PER_PMD    1
> #define PMD_SIZE        (1UL << PMD_SHIFT)
> #define PMD_MASK        (~(PMD_SIZE-1))
> 
> /*
>  * The "pud_xxx()" functions here are trivial for a folded two-level
>  * setup: the pmd is never bad, and a pmd always exists (as it's folded
>  * into the pud entry)
>  */
> static inline int pud_none(pud_t pud)           { return 0; }
> static inline int pud_bad(pud_t pud)            { return 0; }
> static inline int pud_present(pud_t pud)        { return 1; }
> ================================================================>
> 
> In any case I've removed the commit for the time being until this all 
> is cleared up.

So there's a simple solution: define it on i386 too, via the patch 
below. It appears the double-definition doesn't create any warnings, on 
GCC at least.

But if it's an issue, we could do something like this in 
<asm-generic/pgtable-nopmd.h>:

 #if defined(PTRS_PER_PMD) && (PTRS_PER_PMD != 1)
 # error "mm: Wait a minute, that's a super confusing pagetable setup ..."
 #endif

?

Thanks,

	Ingo

=========================>
From: Ingo Molnar <mingo@kernel.org>
Date: Thu, 6 Mar 2025 22:53:49 +0100
Subject: [PATCH] x86/mm: Define PTRS_PER_PMD for assembly code too

Andy reported the following build warning from head_32.S:

  In file included from arch/x86/kernel/head_32.S:29:
  arch/x86/include/asm/pgtable_32.h:59:5: error: "PTRS_PER_PMD" is not defined, evaluates to 0 [-Werror=undef]
       59 | #if PTRS_PER_PMD > 1

The reason is that on 2-level i386 paging the folded in PMD's
PTRS_PER_PMD constant is not defined in assembly headers,
only in generic MM C headers.

Instead of trying to fish out the definition from the generic
headers, just define it - it even has a comment for it already...

Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/pgtable-2level_types.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/pgtable-2level_types.h b/arch/x86/include/asm/pgtable-2level_types.h
index 7f6ccff0ba72..4a12c276b181 100644
--- a/arch/x86/include/asm/pgtable-2level_types.h
+++ b/arch/x86/include/asm/pgtable-2level_types.h
@@ -23,17 +23,17 @@ typedef union {
 #define ARCH_PAGE_TABLE_SYNC_MASK	PGTBL_PMD_MODIFIED
 
 /*
- * traditional i386 two-level paging structure:
+ * Traditional i386 two-level paging structure:
  */
 
 #define PGDIR_SHIFT	22
 #define PTRS_PER_PGD	1024
 
-
 /*
- * the i386 is two-level, so we don't really have any
- * PMD directory physically.
+ * The i386 is two-level, so we don't really have any
+ * PMD directory physically:
  */
+#define PTRS_PER_PMD	1
 
 #define PTRS_PER_PTE	1024

Re: [PATCH] x86/mm: Define PTRS_PER_PMD for assembly code too

Posted by Andy Shevchenko 11 months, 1 week ago

On Thu, Mar 06, 2025 at 11:00:16PM +0100, Ingo Molnar wrote:
> * Ingo Molnar <mingo@kernel.org> wrote:
> 
> > Separating out the assembler-compatible defines from the types 
> > headers appears to be a bigger patch, since it's all mixed in with C 
> > syntax:
> > 
> > <=-----------------------------------===============================
> > typedef struct { pud_t pud; } pmd_t;
> > 
> > #define PMD_SHIFT       PUD_SHIFT
> > #define PTRS_PER_PMD    1
> > #define PMD_SIZE        (1UL << PMD_SHIFT)
> > #define PMD_MASK        (~(PMD_SIZE-1))
> > 
> > /*
> >  * The "pud_xxx()" functions here are trivial for a folded two-level
> >  * setup: the pmd is never bad, and a pmd always exists (as it's folded
> >  * into the pud entry)
> >  */
> > static inline int pud_none(pud_t pud)           { return 0; }
> > static inline int pud_bad(pud_t pud)            { return 0; }
> > static inline int pud_present(pud_t pud)        { return 1; }
> > ================================================================>
> > 
> > In any case I've removed the commit for the time being until this all 
> > is cleared up.
> 
> So there's a simple solution: define it on i386 too, via the patch 
> below. It appears the double-definition doesn't create any warnings, on 
> GCC at least.

Fine by me as long as it gets fixed. Currently it prevents the WERROR=y
to be used along with `make W=1` for x86_32 by both compilers.

> But if it's an issue, we could do something like this in 
> <asm-generic/pgtable-nopmd.h>:
> 
>  #if defined(PTRS_PER_PMD) && (PTRS_PER_PMD != 1)
>  # error "mm: Wait a minute, that's a super confusing pagetable setup ..."
>  #endif
> 
> ?
> 
> Thanks,
> 
> 	Ingo
> 
> =========================>
> From: Ingo Molnar <mingo@kernel.org>
> Date: Thu, 6 Mar 2025 22:53:49 +0100
> Subject: [PATCH] x86/mm: Define PTRS_PER_PMD for assembly code too
> 
> Andy reported the following build warning from head_32.S:
> 
>   In file included from arch/x86/kernel/head_32.S:29:
>   arch/x86/include/asm/pgtable_32.h:59:5: error: "PTRS_PER_PMD" is not defined, evaluates to 0 [-Werror=undef]
>        59 | #if PTRS_PER_PMD > 1
> 
> The reason is that on 2-level i386 paging the folded in PMD's
> PTRS_PER_PMD constant is not defined in assembly headers,
> only in generic MM C headers.
> 
> Instead of trying to fish out the definition from the generic
> headers, just define it - it even has a comment for it already...
> 
> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/include/asm/pgtable-2level_types.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pgtable-2level_types.h b/arch/x86/include/asm/pgtable-2level_types.h
> index 7f6ccff0ba72..4a12c276b181 100644
> --- a/arch/x86/include/asm/pgtable-2level_types.h
> +++ b/arch/x86/include/asm/pgtable-2level_types.h
> @@ -23,17 +23,17 @@ typedef union {
>  #define ARCH_PAGE_TABLE_SYNC_MASK	PGTBL_PMD_MODIFIED
>  
>  /*
> - * traditional i386 two-level paging structure:
> + * Traditional i386 two-level paging structure:
>   */
>  
>  #define PGDIR_SHIFT	22
>  #define PTRS_PER_PGD	1024
>  
> -
>  /*
> - * the i386 is two-level, so we don't really have any
> - * PMD directory physically.
> + * The i386 is two-level, so we don't really have any
> + * PMD directory physically:
>   */
> +#define PTRS_PER_PMD	1

Should I give a try?

Okay, just

Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

for x86_32 with Clang 19.1.7 and GCC 14.2.0.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] x86/mm: Define PTRS_PER_PMD for assembly code too

Posted by Ingo Molnar 11 months, 1 week ago

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Okay, just
> 
> Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> for x86_32 with Clang 19.1.7 and GCC 14.2.0.

Thanks!

	Ingo

[tip: x86/urgent] x86/mm: Define PTRS_PER_PMD for assembly code too

Posted by tip-bot2 for Ingo Molnar 11 months, 1 week ago

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     6914f7e2e25fac9d1d2b62c208eaa5f2bf810fe9
Gitweb:        https://git.kernel.org/tip/6914f7e2e25fac9d1d2b62c208eaa5f2bf810fe9
Author:        Ingo Molnar <mingo@kernel.org>
AuthorDate:    Thu, 06 Mar 2025 23:00:16 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Sat, 08 Mar 2025 00:09:09 +01:00

x86/mm: Define PTRS_PER_PMD for assembly code too

Andy reported the following build warning from head_32.S:

  In file included from arch/x86/kernel/head_32.S:29:
  arch/x86/include/asm/pgtable_32.h:59:5: error: "PTRS_PER_PMD" is not defined, evaluates to 0 [-Werror=undef]
       59 | #if PTRS_PER_PMD > 1

The reason is that on 2-level i386 paging the folded in PMD's
PTRS_PER_PMD constant is not defined in assembly headers,
only in generic MM C headers.

Instead of trying to fish out the definition from the generic
headers, just define it - it even has a comment for it already...

Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/r/Z8oa8AUVyi2HWfo9@gmail.com
---
 arch/x86/include/asm/pgtable-2level_types.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/pgtable-2level_types.h b/arch/x86/include/asm/pgtable-2level_types.h
index 7f6ccff..4a12c27 100644
--- a/arch/x86/include/asm/pgtable-2level_types.h
+++ b/arch/x86/include/asm/pgtable-2level_types.h
@@ -23,17 +23,17 @@ typedef union {
 #define ARCH_PAGE_TABLE_SYNC_MASK	PGTBL_PMD_MODIFIED
 
 /*
- * traditional i386 two-level paging structure:
+ * Traditional i386 two-level paging structure:
  */
 
 #define PGDIR_SHIFT	22
 #define PTRS_PER_PGD	1024
 
-
 /*
- * the i386 is two-level, so we don't really have any
- * PMD directory physically.
+ * The i386 is two-level, so we don't really have any
+ * PMD directory physically:
  */
+#define PTRS_PER_PMD	1
 
 #define PTRS_PER_PTE	1024