Re: Regression with v5.18-rc1 tag on STM32F7 and STM32H7 based boards

Re: Regression with v5.18-rc1 tag on STM32F7 and STM32H7 based boards

[Hugh Dickins]

Asking Arnd and others below: should noMMU arches have a good ZERO_PAGE?

On Tue, 5 Apr 2022, Hugh Dickins wrote:
> On Tue, 5 Apr 2022, Patrice CHOTARD wrote:
> > 
> > We found an issue with last kernel tag v5.18-rc1 on stm32f746-disco and 
> > stm32h743-disco boards (ARMV7-M SoCs).
> > 
> > Kernel hangs when executing SetPageUptodate(ZERO_PAGE(0)); in mm/filemap.c.
> > 
> > By reverting commit 56a8c8eb1eaf ("tmpfs: do not allocate pages on read"), 
> > kernel boots without any issue.
> 
> Sorry about that, thanks a lot for finding.
> 
> I see that arch/arm/configs/stm32_defconfig says CONFIG_MMU is not set:
> please confirm that is the case here.
> 
> Yes, it looks as if NOMMU platforms are liable to have a bogus (that's my
> reading, but it may be unfair) definition for ZERO_PAGE(vaddr), and I was
> walking on ice to touch it without regard for !CONFIG_MMU.
> 
> CONFIG_SHMEM depends on CONFIG_MMU, so that PageUptodate is only needed
> when CONFIG_MMU.
> 
> Easily fixed by an #ifdef CONFIG_MMU there in mm/filemap.c, but I'll hunt
> around (again) for a better place to do it - though I won't want to touch
> all the architectures for it.  I'll post later today.

I could put #ifdef CONFIG_MMU around the SetPageUptodate(ZERO_PAGE(0))
added to pagecache_init(); or if that's considered distasteful, I could
skip making it potentially useful to other filesystems, revert the change
to pagecache_init(), and just do it in mm/shmem.c's CONFIG_SHMEM (hence
CONFIG_MMU) instance of shmem_init().

But I wonder if it's safe for noMMU architectures to go on without a
working ZERO_PAGE(0).  It has uses scattered throughout the tree, in
drivers, fs, crypto and more, and it's not at all obvious (to me) that
they all depend on CONFIG_MMU.  Some might cause (unreported) crashes,
some might use an unzeroed page in place of a pageful of zeroes.

arm noMMU and h8300 noMMU and m68k noMMU each has
#define ZERO_PAGE(vaddr)	(virt_to_page(0))
which seems riskily wrong to me.

h8300 and m68k actually go to the trouble of allocating an empty_zero_page
for this, but then forget to link it up to the ZERO_PAGE(vaddr) definition,
which is what all the common code uses.

arm noMMU does not presently allocate such a page; and I do not feel
entitled to steal a page from arm noMMU platforms, for a hypothetical
case, without agreement.

But here's an unbuilt and untested patch for consideration - which of
course should be split in three if agreed (and perhaps the h8300 part
quietly forgotten if h8300 is already on its way out).

(Yes, arm uses empty_zero_page in a different way from all the other
architectures; but that's okay, and I think arm's way, with virt_to_page()
already baked in, is better than the others; but I've no wish to get into
changing them.)

Patrice, does this patch build and run for you? I have no appreciation
of arm early startup issues, and may have got it horribly wrong.

Thanks,
Hugh

 arch/arm/include/asm/pgtable-nommu.h |    3 ++-
 arch/arm/mm/nommu.c                  |   16 ++++++++++++++++
 arch/h8300/include/asm/pgtable.h     |    6 +++++-
 arch/h8300/mm/init.c                 |    5 +++--
 arch/m68k/include/asm/pgtable_no.h   |    5 ++++-
 5 files changed, 30 insertions(+), 5 deletions(-)

--- a/arch/arm/include/asm/pgtable-nommu.h
+++ b/arch/arm/include/asm/pgtable-nommu.h
@@ -48,7 +48,8 @@ typedef pte_t *pte_addr_t;
  * ZERO_PAGE is a global shared page that is always zero: used
  * for zero-mapped memory areas etc..
  */
-#define ZERO_PAGE(vaddr)	(virt_to_page(0))
+extern struct page *empty_zero_page;
+#define ZERO_PAGE(vaddr)	(empty_zero_page)
 
 /*
  * Mark the prot value as uncacheable and unbufferable.
--- a/arch/arm/mm/nommu.c
+++ b/arch/arm/mm/nommu.c
@@ -24,6 +24,13 @@
 
 #include "mm.h"
 
+/*
+ * empty_zero_page is a special page that is used for
+ * zero-initialized data and COW.
+ */
+struct page *empty_zero_page;
+EXPORT_SYMBOL(empty_zero_page);
+
 unsigned long vectors_base;
 
 #ifdef CONFIG_ARM_MPU
@@ -148,9 +155,18 @@ void __init adjust_lowmem_bounds(void)
  */
 void __init paging_init(const struct machine_desc *mdesc)
 {
+	void *zero_page;
+
 	early_trap_init((void *)vectors_base);
 	mpu_setup();
 	bootmem_init();
+
+	zero_page = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
+	if (!zero_page)
+		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
+		      __func__, PAGE_SIZE, PAGE_SIZE);
+	empty_zero_page = virt_to_page(zero_page);
+	flush_dcache_page(empty_zero_page);
 }
 
 /*
--- a/arch/h8300/include/asm/pgtable.h
+++ b/arch/h8300/include/asm/pgtable.h
@@ -19,11 +19,15 @@ extern void paging_init(void);
 
 static inline int pte_file(pte_t pte) { return 0; }
 #define swapper_pg_dir ((pgd_t *) 0)
+
+/* zero page used for uninitialized stuff */
+extern void *empty_zero_page;
+
 /*
  * ZERO_PAGE is a global shared page that is always zero: used
  * for zero-mapped memory areas etc..
  */
-#define ZERO_PAGE(vaddr)	(virt_to_page(0))
+#define ZERO_PAGE(vaddr)	(virt_to_page(empty_zero_page))
 
 /*
  * These would be in other places but having them here reduces the diffs.
--- a/arch/h8300/mm/init.c
+++ b/arch/h8300/mm/init.c
@@ -41,7 +41,8 @@
  * ZERO_PAGE is a special page that is used for zero-initialized
  * data and COW.
  */
-unsigned long empty_zero_page;
+void *empty_zero_page;
+EXPORT_SYMBOL(empty_zero_page);
 
 /*
  * paging_init() continues the virtual memory environment setup which
@@ -65,7 +66,7 @@ void __init paging_init(void)
 	 * Initialize the bad page table and bad page to point
 	 * to a couple of allocated pages.
 	 */
-	empty_zero_page = (unsigned long)memblock_alloc(PAGE_SIZE, PAGE_SIZE);
+	empty_zero_page = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
 	if (!empty_zero_page)
 		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
 		      __func__, PAGE_SIZE, PAGE_SIZE);
--- a/arch/m68k/include/asm/pgtable_no.h
+++ b/arch/m68k/include/asm/pgtable_no.h
@@ -38,11 +38,14 @@ extern void paging_init(void);
 #define __pte_to_swp_entry(pte)	((swp_entry_t) { pte_val(pte) })
 #define __swp_entry_to_pte(x)	((pte_t) { (x).val })
 
+/* zero page used for uninitialized stuff */
+extern void *empty_zero_page;
+
 /*
  * ZERO_PAGE is a global shared page that is always zero: used
  * for zero-mapped memory areas etc..
  */
-#define ZERO_PAGE(vaddr)	(virt_to_page(0))
+#define ZERO_PAGE(vaddr)	(virt_to_page(empty_zero_page))
 
 /*
  * All 32bit addresses are effectively valid for vmalloc...

Re: Regression with v5.18-rc1 tag on STM32F7 and STM32H7 based boards

[Patrice CHOTARD]

Hugh, 

On 4/6/22 08:22, Hugh Dickins wrote:
> Asking Arnd and others below: should noMMU arches have a good ZERO_PAGE?
> 
> On Tue, 5 Apr 2022, Hugh Dickins wrote:
>> On Tue, 5 Apr 2022, Patrice CHOTARD wrote:
>>>
>>> We found an issue with last kernel tag v5.18-rc1 on stm32f746-disco and 
>>> stm32h743-disco boards (ARMV7-M SoCs).
>>>
>>> Kernel hangs when executing SetPageUptodate(ZERO_PAGE(0)); in mm/filemap.c.
>>>
>>> By reverting commit 56a8c8eb1eaf ("tmpfs: do not allocate pages on read"), 
>>> kernel boots without any issue.
>>
>> Sorry about that, thanks a lot for finding.
>>
>> I see that arch/arm/configs/stm32_defconfig says CONFIG_MMU is not set:
>> please confirm that is the case here.
>>
>> Yes, it looks as if NOMMU platforms are liable to have a bogus (that's my
>> reading, but it may be unfair) definition for ZERO_PAGE(vaddr), and I was
>> walking on ice to touch it without regard for !CONFIG_MMU.
>>
>> CONFIG_SHMEM depends on CONFIG_MMU, so that PageUptodate is only needed
>> when CONFIG_MMU.
>>
>> Easily fixed by an #ifdef CONFIG_MMU there in mm/filemap.c, but I'll hunt
>> around (again) for a better place to do it - though I won't want to touch
>> all the architectures for it.  I'll post later today.
> 
> I could put #ifdef CONFIG_MMU around the SetPageUptodate(ZERO_PAGE(0))
> added to pagecache_init(); or if that's considered distasteful, I could
> skip making it potentially useful to other filesystems, revert the change
> to pagecache_init(), and just do it in mm/shmem.c's CONFIG_SHMEM (hence
> CONFIG_MMU) instance of shmem_init().
> 
> But I wonder if it's safe for noMMU architectures to go on without a
> working ZERO_PAGE(0).  It has uses scattered throughout the tree, in
> drivers, fs, crypto and more, and it's not at all obvious (to me) that
> they all depend on CONFIG_MMU.  Some might cause (unreported) crashes,
> some might use an unzeroed page in place of a pageful of zeroes.
> 
> arm noMMU and h8300 noMMU and m68k noMMU each has
> #define ZERO_PAGE(vaddr)	(virt_to_page(0))
> which seems riskily wrong to me.
> 
> h8300 and m68k actually go to the trouble of allocating an empty_zero_page
> for this, but then forget to link it up to the ZERO_PAGE(vaddr) definition,
> which is what all the common code uses.
> 
> arm noMMU does not presently allocate such a page; and I do not feel
> entitled to steal a page from arm noMMU platforms, for a hypothetical
> case, without agreement.
> 
> But here's an unbuilt and untested patch for consideration - which of
> course should be split in three if agreed (and perhaps the h8300 part
> quietly forgotten if h8300 is already on its way out).
> 
> (Yes, arm uses empty_zero_page in a different way from all the other
> architectures; but that's okay, and I think arm's way, with virt_to_page()
> already baked in, is better than the others; but I've no wish to get into
> changing them.)
> 
> Patrice, does this patch build and run for you? I have no appreciation
> of arm early startup issues, and may have got it horribly wrong.

This patch is okay on my side on both boards (STM32F7 and STM32H7), boot are OK.

Thanks for your reactivity ;-)
Patrice

> 
> Thanks,
> Hugh
> 
>  arch/arm/include/asm/pgtable-nommu.h |    3 ++-
>  arch/arm/mm/nommu.c                  |   16 ++++++++++++++++
>  arch/h8300/include/asm/pgtable.h     |    6 +++++-
>  arch/h8300/mm/init.c                 |    5 +++--
>  arch/m68k/include/asm/pgtable_no.h   |    5 ++++-
>  5 files changed, 30 insertions(+), 5 deletions(-)
> 
> --- a/arch/arm/include/asm/pgtable-nommu.h
> +++ b/arch/arm/include/asm/pgtable-nommu.h
> @@ -48,7 +48,8 @@ typedef pte_t *pte_addr_t;
>   * ZERO_PAGE is a global shared page that is always zero: used
>   * for zero-mapped memory areas etc..
>   */
> -#define ZERO_PAGE(vaddr)	(virt_to_page(0))
> +extern struct page *empty_zero_page;
> +#define ZERO_PAGE(vaddr)	(empty_zero_page)
>  
>  /*
>   * Mark the prot value as uncacheable and unbufferable.
> --- a/arch/arm/mm/nommu.c
> +++ b/arch/arm/mm/nommu.c
> @@ -24,6 +24,13 @@
>  
>  #include "mm.h"
>  
> +/*
> + * empty_zero_page is a special page that is used for
> + * zero-initialized data and COW.
> + */
> +struct page *empty_zero_page;
> +EXPORT_SYMBOL(empty_zero_page);
> +
>  unsigned long vectors_base;
>  
>  #ifdef CONFIG_ARM_MPU
> @@ -148,9 +155,18 @@ void __init adjust_lowmem_bounds(void)
>   */
>  void __init paging_init(const struct machine_desc *mdesc)
>  {
> +	void *zero_page;
> +
>  	early_trap_init((void *)vectors_base);
>  	mpu_setup();
>  	bootmem_init();
> +
> +	zero_page = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> +	if (!zero_page)
> +		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> +		      __func__, PAGE_SIZE, PAGE_SIZE);
> +	empty_zero_page = virt_to_page(zero_page);
> +	flush_dcache_page(empty_zero_page);
>  }
>  
>  /*
> --- a/arch/h8300/include/asm/pgtable.h
> +++ b/arch/h8300/include/asm/pgtable.h
> @@ -19,11 +19,15 @@ extern void paging_init(void);
>  
>  static inline int pte_file(pte_t pte) { return 0; }
>  #define swapper_pg_dir ((pgd_t *) 0)
> +
> +/* zero page used for uninitialized stuff */
> +extern void *empty_zero_page;
> +
>  /*
>   * ZERO_PAGE is a global shared page that is always zero: used
>   * for zero-mapped memory areas etc..
>   */
> -#define ZERO_PAGE(vaddr)	(virt_to_page(0))
> +#define ZERO_PAGE(vaddr)	(virt_to_page(empty_zero_page))
>  
>  /*
>   * These would be in other places but having them here reduces the diffs.
> --- a/arch/h8300/mm/init.c
> +++ b/arch/h8300/mm/init.c
> @@ -41,7 +41,8 @@
>   * ZERO_PAGE is a special page that is used for zero-initialized
>   * data and COW.
>   */
> -unsigned long empty_zero_page;
> +void *empty_zero_page;
> +EXPORT_SYMBOL(empty_zero_page);
>  
>  /*
>   * paging_init() continues the virtual memory environment setup which
> @@ -65,7 +66,7 @@ void __init paging_init(void)
>  	 * Initialize the bad page table and bad page to point
>  	 * to a couple of allocated pages.
>  	 */
> -	empty_zero_page = (unsigned long)memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> +	empty_zero_page = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
>  	if (!empty_zero_page)
>  		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
>  		      __func__, PAGE_SIZE, PAGE_SIZE);
> --- a/arch/m68k/include/asm/pgtable_no.h
> +++ b/arch/m68k/include/asm/pgtable_no.h
> @@ -38,11 +38,14 @@ extern void paging_init(void);
>  #define __pte_to_swp_entry(pte)	((swp_entry_t) { pte_val(pte) })
>  #define __swp_entry_to_pte(x)	((pte_t) { (x).val })
>  
> +/* zero page used for uninitialized stuff */
> +extern void *empty_zero_page;
> +
>  /*
>   * ZERO_PAGE is a global shared page that is always zero: used
>   * for zero-mapped memory areas etc..
>   */
> -#define ZERO_PAGE(vaddr)	(virt_to_page(0))
> +#define ZERO_PAGE(vaddr)	(virt_to_page(empty_zero_page))
>  
>  /*
>   * All 32bit addresses are effectively valid for vmalloc...

Re: Regression with v5.18-rc1 tag on STM32F7 and STM32H7 based boards

[Hugh Dickins]

On Wed, 6 Apr 2022, Patrice CHOTARD wrote:
> On 4/6/22 08:22, Hugh Dickins wrote:
> > Asking Arnd and others below: should noMMU arches have a good ZERO_PAGE?
> > 
> > On Tue, 5 Apr 2022, Hugh Dickins wrote:
> >> On Tue, 5 Apr 2022, Patrice CHOTARD wrote:
> >>>
> >>> We found an issue with last kernel tag v5.18-rc1 on stm32f746-disco and 
> >>> stm32h743-disco boards (ARMV7-M SoCs).
> >>>
> >>> Kernel hangs when executing SetPageUptodate(ZERO_PAGE(0)); in mm/filemap.c.
> >>>
> >>> By reverting commit 56a8c8eb1eaf ("tmpfs: do not allocate pages on read"), 
> >>> kernel boots without any issue.
> >>
> >> Sorry about that, thanks a lot for finding.
> >>
> >> I see that arch/arm/configs/stm32_defconfig says CONFIG_MMU is not set:
> >> please confirm that is the case here.
> >>
> >> Yes, it looks as if NOMMU platforms are liable to have a bogus (that's my
> >> reading, but it may be unfair) definition for ZERO_PAGE(vaddr), and I was
> >> walking on ice to touch it without regard for !CONFIG_MMU.
> >>
> >> CONFIG_SHMEM depends on CONFIG_MMU, so that PageUptodate is only needed
> >> when CONFIG_MMU.
> >>
> >> Easily fixed by an #ifdef CONFIG_MMU there in mm/filemap.c, but I'll hunt
> >> around (again) for a better place to do it - though I won't want to touch
> >> all the architectures for it.  I'll post later today.
> > 
> > I could put #ifdef CONFIG_MMU around the SetPageUptodate(ZERO_PAGE(0))
> > added to pagecache_init(); or if that's considered distasteful, I could
> > skip making it potentially useful to other filesystems, revert the change
> > to pagecache_init(), and just do it in mm/shmem.c's CONFIG_SHMEM (hence
> > CONFIG_MMU) instance of shmem_init().
> > 
> > But I wonder if it's safe for noMMU architectures to go on without a
> > working ZERO_PAGE(0).  It has uses scattered throughout the tree, in
> > drivers, fs, crypto and more, and it's not at all obvious (to me) that
> > they all depend on CONFIG_MMU.  Some might cause (unreported) crashes,
> > some might use an unzeroed page in place of a pageful of zeroes.
> > 
> > arm noMMU and h8300 noMMU and m68k noMMU each has
> > #define ZERO_PAGE(vaddr)	(virt_to_page(0))
> > which seems riskily wrong to me.
> > 
> > h8300 and m68k actually go to the trouble of allocating an empty_zero_page
> > for this, but then forget to link it up to the ZERO_PAGE(vaddr) definition,
> > which is what all the common code uses.
> > 
> > arm noMMU does not presently allocate such a page; and I do not feel
> > entitled to steal a page from arm noMMU platforms, for a hypothetical
> > case, without agreement.
> > 
> > But here's an unbuilt and untested patch for consideration - which of
> > course should be split in three if agreed (and perhaps the h8300 part
> > quietly forgotten if h8300 is already on its way out).
> > 
> > (Yes, arm uses empty_zero_page in a different way from all the other
> > architectures; but that's okay, and I think arm's way, with virt_to_page()
> > already baked in, is better than the others; but I've no wish to get into
> > changing them.)
> > 
> > Patrice, does this patch build and run for you? I have no appreciation
> > of arm early startup issues, and may have got it horribly wrong.
> 
> This patch is okay on my side on both boards (STM32F7 and STM32H7), boot are OK.
> 
> Thanks for your reactivity ;-)
> Patrice

Just to wrap up this thread: the tentative arch/ patches below did not
go into 5.18-rc2, but 5.18-rc3 will contain
1bdec44b1eee ("tmpfs: fix regressions from wider use of ZERO_PAGE")
which fixes a further issue, and deletes the line which gave you trouble.

With arch/h8300 removed from linux-next, and arch/arm losing a page by
the patch below, I don't think it's worth my arguing for those changes.
I'd still prefer arch/m68k to expose its empty_zero_page in ZERO_PAGE(),
or else not allocate it; but I won't be pursuing this further.

Thanks for reporting!
Hugh

> > 
> >  arch/arm/include/asm/pgtable-nommu.h |    3 ++-
> >  arch/arm/mm/nommu.c                  |   16 ++++++++++++++++
> >  arch/h8300/include/asm/pgtable.h     |    6 +++++-
> >  arch/h8300/mm/init.c                 |    5 +++--
> >  arch/m68k/include/asm/pgtable_no.h   |    5 ++++-
> >  5 files changed, 30 insertions(+), 5 deletions(-)
> > 
> > --- a/arch/arm/include/asm/pgtable-nommu.h
> > +++ b/arch/arm/include/asm/pgtable-nommu.h
> > @@ -48,7 +48,8 @@ typedef pte_t *pte_addr_t;
> >   * ZERO_PAGE is a global shared page that is always zero: used
> >   * for zero-mapped memory areas etc..
> >   */
> > -#define ZERO_PAGE(vaddr)	(virt_to_page(0))
> > +extern struct page *empty_zero_page;
> > +#define ZERO_PAGE(vaddr)	(empty_zero_page)
> >  
> >  /*
> >   * Mark the prot value as uncacheable and unbufferable.
> > --- a/arch/arm/mm/nommu.c
> > +++ b/arch/arm/mm/nommu.c
> > @@ -24,6 +24,13 @@
> >  
> >  #include "mm.h"
> >  
> > +/*
> > + * empty_zero_page is a special page that is used for
> > + * zero-initialized data and COW.
> > + */
> > +struct page *empty_zero_page;
> > +EXPORT_SYMBOL(empty_zero_page);
> > +
> >  unsigned long vectors_base;
> >  
> >  #ifdef CONFIG_ARM_MPU
> > @@ -148,9 +155,18 @@ void __init adjust_lowmem_bounds(void)
> >   */
> >  void __init paging_init(const struct machine_desc *mdesc)
> >  {
> > +	void *zero_page;
> > +
> >  	early_trap_init((void *)vectors_base);
> >  	mpu_setup();
> >  	bootmem_init();
> > +
> > +	zero_page = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> > +	if (!zero_page)
> > +		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> > +		      __func__, PAGE_SIZE, PAGE_SIZE);
> > +	empty_zero_page = virt_to_page(zero_page);
> > +	flush_dcache_page(empty_zero_page);
> >  }
> >  
> >  /*
> > --- a/arch/h8300/include/asm/pgtable.h
> > +++ b/arch/h8300/include/asm/pgtable.h
> > @@ -19,11 +19,15 @@ extern void paging_init(void);
> >  
> >  static inline int pte_file(pte_t pte) { return 0; }
> >  #define swapper_pg_dir ((pgd_t *) 0)
> > +
> > +/* zero page used for uninitialized stuff */
> > +extern void *empty_zero_page;
> > +
> >  /*
> >   * ZERO_PAGE is a global shared page that is always zero: used
> >   * for zero-mapped memory areas etc..
> >   */
> > -#define ZERO_PAGE(vaddr)	(virt_to_page(0))
> > +#define ZERO_PAGE(vaddr)	(virt_to_page(empty_zero_page))
> >  
> >  /*
> >   * These would be in other places but having them here reduces the diffs.
> > --- a/arch/h8300/mm/init.c
> > +++ b/arch/h8300/mm/init.c
> > @@ -41,7 +41,8 @@
> >   * ZERO_PAGE is a special page that is used for zero-initialized
> >   * data and COW.
> >   */
> > -unsigned long empty_zero_page;
> > +void *empty_zero_page;
> > +EXPORT_SYMBOL(empty_zero_page);
> >  
> >  /*
> >   * paging_init() continues the virtual memory environment setup which
> > @@ -65,7 +66,7 @@ void __init paging_init(void)
> >  	 * Initialize the bad page table and bad page to point
> >  	 * to a couple of allocated pages.
> >  	 */
> > -	empty_zero_page = (unsigned long)memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> > +	empty_zero_page = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> >  	if (!empty_zero_page)
> >  		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> >  		      __func__, PAGE_SIZE, PAGE_SIZE);
> > --- a/arch/m68k/include/asm/pgtable_no.h
> > +++ b/arch/m68k/include/asm/pgtable_no.h
> > @@ -38,11 +38,14 @@ extern void paging_init(void);
> >  #define __pte_to_swp_entry(pte)	((swp_entry_t) { pte_val(pte) })
> >  #define __swp_entry_to_pte(x)	((pte_t) { (x).val })
> >  
> > +/* zero page used for uninitialized stuff */
> > +extern void *empty_zero_page;
> > +
> >  /*
> >   * ZERO_PAGE is a global shared page that is always zero: used
> >   * for zero-mapped memory areas etc..
> >   */
> > -#define ZERO_PAGE(vaddr)	(virt_to_page(0))
> > +#define ZERO_PAGE(vaddr)	(virt_to_page(empty_zero_page))
> >  
> >  /*
> >   * All 32bit addresses are effectively valid for vmalloc...
> 

Re: Regression with v5.18-rc1 tag on STM32F7 and STM32H7 based boards

[Greg Ungerer]

Hi Hugh,

On 16/4/22 10:58, Hugh Dickins wrote:
> On Wed, 6 Apr 2022, Patrice CHOTARD wrote:
>> On 4/6/22 08:22, Hugh Dickins wrote:
>>> Asking Arnd and others below: should noMMU arches have a good ZERO_PAGE?
>>>
>>> On Tue, 5 Apr 2022, Hugh Dickins wrote:
>>>> On Tue, 5 Apr 2022, Patrice CHOTARD wrote:
>>>>>
>>>>> We found an issue with last kernel tag v5.18-rc1 on stm32f746-disco and
>>>>> stm32h743-disco boards (ARMV7-M SoCs).
>>>>>
>>>>> Kernel hangs when executing SetPageUptodate(ZERO_PAGE(0)); in mm/filemap.c.
>>>>>
>>>>> By reverting commit 56a8c8eb1eaf ("tmpfs: do not allocate pages on read"),
>>>>> kernel boots without any issue.
>>>>
>>>> Sorry about that, thanks a lot for finding.
>>>>
>>>> I see that arch/arm/configs/stm32_defconfig says CONFIG_MMU is not set:
>>>> please confirm that is the case here.
>>>>
>>>> Yes, it looks as if NOMMU platforms are liable to have a bogus (that's my
>>>> reading, but it may be unfair) definition for ZERO_PAGE(vaddr), and I was
>>>> walking on ice to touch it without regard for !CONFIG_MMU.
>>>>
>>>> CONFIG_SHMEM depends on CONFIG_MMU, so that PageUptodate is only needed
>>>> when CONFIG_MMU.
>>>>
>>>> Easily fixed by an #ifdef CONFIG_MMU there in mm/filemap.c, but I'll hunt
>>>> around (again) for a better place to do it - though I won't want to touch
>>>> all the architectures for it.  I'll post later today.
>>>
>>> I could put #ifdef CONFIG_MMU around the SetPageUptodate(ZERO_PAGE(0))
>>> added to pagecache_init(); or if that's considered distasteful, I could
>>> skip making it potentially useful to other filesystems, revert the change
>>> to pagecache_init(), and just do it in mm/shmem.c's CONFIG_SHMEM (hence
>>> CONFIG_MMU) instance of shmem_init().
>>>
>>> But I wonder if it's safe for noMMU architectures to go on without a
>>> working ZERO_PAGE(0).  It has uses scattered throughout the tree, in
>>> drivers, fs, crypto and more, and it's not at all obvious (to me) that
>>> they all depend on CONFIG_MMU.  Some might cause (unreported) crashes,
>>> some might use an unzeroed page in place of a pageful of zeroes.
>>>
>>> arm noMMU and h8300 noMMU and m68k noMMU each has
>>> #define ZERO_PAGE(vaddr)	(virt_to_page(0))
>>> which seems riskily wrong to me.
>>>
>>> h8300 and m68k actually go to the trouble of allocating an empty_zero_page
>>> for this, but then forget to link it up to the ZERO_PAGE(vaddr) definition,
>>> which is what all the common code uses.
>>>
>>> arm noMMU does not presently allocate such a page; and I do not feel
>>> entitled to steal a page from arm noMMU platforms, for a hypothetical
>>> case, without agreement.
>>>
>>> But here's an unbuilt and untested patch for consideration - which of
>>> course should be split in three if agreed (and perhaps the h8300 part
>>> quietly forgotten if h8300 is already on its way out).
>>>
>>> (Yes, arm uses empty_zero_page in a different way from all the other
>>> architectures; but that's okay, and I think arm's way, with virt_to_page()
>>> already baked in, is better than the others; but I've no wish to get into
>>> changing them.)
>>>
>>> Patrice, does this patch build and run for you? I have no appreciation
>>> of arm early startup issues, and may have got it horribly wrong.
>>
>> This patch is okay on my side on both boards (STM32F7 and STM32H7), boot are OK.
>>
>> Thanks for your reactivity ;-)
>> Patrice
> 
> Just to wrap up this thread: the tentative arch/ patches below did not
> go into 5.18-rc2, but 5.18-rc3 will contain
> 1bdec44b1eee ("tmpfs: fix regressions from wider use of ZERO_PAGE")
> which fixes a further issue, and deletes the line which gave you trouble.
> 
> With arch/h8300 removed from linux-next, and arch/arm losing a page by
> the patch below, I don't think it's worth my arguing for those changes.
> I'd still prefer arch/m68k to expose its empty_zero_page in ZERO_PAGE(),
> or else not allocate it; but I won't be pursuing this further.

Thanks for pointing this out. It certainly does look wrong to me for
the m68k nommu case. I am not aware of any existing issues caused by
this - but there is no good reason not to fix it.

So I propose this change. Build and run tested on my m68knommu targets.

Regards
Greg


 From f809fb8fbca9e5e637b8fda380955bd799bb3926 Mon Sep 17 00:00:00 2001
From: Greg Ungerer <gerg@linux-m68k.org>
Date: Wed, 20 Apr 2022 23:27:47 +1000
Subject: [PATCH] m68knommu: set ZERO_PAGE() allocated zeroed page

The non-MMU m68k pagetable ZERO_PAGE() macro is being set to the
somewhat non-sensical value of "virt_to_page(0)". The zeroth page
is not in any way guaranteed to be a page full of "0". So the result
is that ZERO_PAGE() will almost certainly contain random values.

We already allocate a real "empty_zero_page" in the mm setup code shared
between MMU m68k and non-MMU m68k. It is just not hooked up to the
ZERO_PAGE() macro for the non-MMU m68k case.

Fix ZERO_PAGE() to use the allocated "empty_zero_page" pointer.

I am not aware of any specific issues caused by the old code.

Link: https://lore.kernel.org/linux-m68k/2a462b23-5b8e-bbf4-ec7d-778434a3b9d7@google.com/T/#t
Reported-by: Hugh Dickens <hughd@google.com>
Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
---
  arch/m68k/include/asm/pgtable_no.h | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/m68k/include/asm/pgtable_no.h b/arch/m68k/include/asm/pgtable_no.h
index 87151d67d91e..bce5ca56c388 100644
--- a/arch/m68k/include/asm/pgtable_no.h
+++ b/arch/m68k/include/asm/pgtable_no.h
@@ -42,7 +42,8 @@ extern void paging_init(void);
   * ZERO_PAGE is a global shared page that is always zero: used
   * for zero-mapped memory areas etc..
   */
-#define ZERO_PAGE(vaddr)	(virt_to_page(0))
+extern void *empty_zero_page;
+#define ZERO_PAGE(vaddr)	(virt_to_page(empty_zero_page))
  
  /*
   * All 32bit addresses are effectively valid for vmalloc...
-- 
2.25.1

Re: Regression with v5.18-rc1 tag on STM32F7 and STM32H7 based boards

[Geert Uytterhoeven]

Hi Greg,

On Wed, Apr 20, 2022 at 3:53 PM Greg Ungerer <gerg@linux-m68k.org> wrote:
> On 16/4/22 10:58, Hugh Dickins wrote:
> > Just to wrap up this thread: the tentative arch/ patches below did not
> > go into 5.18-rc2, but 5.18-rc3 will contain
> > 1bdec44b1eee ("tmpfs: fix regressions from wider use of ZERO_PAGE")
> > which fixes a further issue, and deletes the line which gave you trouble.
> >
> > With arch/h8300 removed from linux-next, and arch/arm losing a page by
> > the patch below, I don't think it's worth my arguing for those changes.
> > I'd still prefer arch/m68k to expose its empty_zero_page in ZERO_PAGE(),
> > or else not allocate it; but I won't be pursuing this further.
>
> Thanks for pointing this out. It certainly does look wrong to me for
> the m68k nommu case. I am not aware of any existing issues caused by
> this - but there is no good reason not to fix it.
>
> So I propose this change. Build and run tested on my m68knommu targets.
>
> Regards
> Greg
>
>
>  From f809fb8fbca9e5e637b8fda380955bd799bb3926 Mon Sep 17 00:00:00 2001
> From: Greg Ungerer <gerg@linux-m68k.org>
> Date: Wed, 20 Apr 2022 23:27:47 +1000
> Subject: [PATCH] m68knommu: set ZERO_PAGE() allocated zeroed page
>
> The non-MMU m68k pagetable ZERO_PAGE() macro is being set to the
> somewhat non-sensical value of "virt_to_page(0)". The zeroth page
> is not in any way guaranteed to be a page full of "0". So the result
> is that ZERO_PAGE() will almost certainly contain random values.
>
> We already allocate a real "empty_zero_page" in the mm setup code shared
> between MMU m68k and non-MMU m68k. It is just not hooked up to the
> ZERO_PAGE() macro for the non-MMU m68k case.
>
> Fix ZERO_PAGE() to use the allocated "empty_zero_page" pointer.
>
> I am not aware of any specific issues caused by the old code.
>
> Link: https://lore.kernel.org/linux-m68k/2a462b23-5b8e-bbf4-ec7d-778434a3b9d7@google.com/T/#t
> Reported-by: Hugh Dickens <hughd@google.com>
> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
> ---
>   arch/m68k/include/asm/pgtable_no.h | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/m68k/include/asm/pgtable_no.h b/arch/m68k/include/asm/pgtable_no.h
> index 87151d67d91e..bce5ca56c388 100644
> --- a/arch/m68k/include/asm/pgtable_no.h
> +++ b/arch/m68k/include/asm/pgtable_no.h
> @@ -42,7 +42,8 @@ extern void paging_init(void);
>    * ZERO_PAGE is a global shared page that is always zero: used
>    * for zero-mapped memory areas etc..
>    */
> -#define ZERO_PAGE(vaddr)       (virt_to_page(0))
> +extern void *empty_zero_page;
> +#define ZERO_PAGE(vaddr)       (virt_to_page(empty_zero_page))
>
>   /*
>    * All 32bit addresses are effectively valid for vmalloc...

And after that (or combined with this?), this can be factored
out from arch/m68k/include/asm/pgtable_{mm,no}.h into
arch/m68k/include/asm/pgtable.h.

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

Re: Regression with v5.18-rc1 tag on STM32F7 and STM32H7 based boards

[Greg Ungerer]

Hi Geert,

On 21/4/22 00:44, Geert Uytterhoeven wrote:
> On Wed, Apr 20, 2022 at 3:53 PM Greg Ungerer <gerg@linux-m68k.org> wrote:
>> On 16/4/22 10:58, Hugh Dickins wrote:
>>> Just to wrap up this thread: the tentative arch/ patches below did not
>>> go into 5.18-rc2, but 5.18-rc3 will contain
>>> 1bdec44b1eee ("tmpfs: fix regressions from wider use of ZERO_PAGE")
>>> which fixes a further issue, and deletes the line which gave you trouble.
>>>
>>> With arch/h8300 removed from linux-next, and arch/arm losing a page by
>>> the patch below, I don't think it's worth my arguing for those changes.
>>> I'd still prefer arch/m68k to expose its empty_zero_page in ZERO_PAGE(),
>>> or else not allocate it; but I won't be pursuing this further.
>>
>> Thanks for pointing this out. It certainly does look wrong to me for
>> the m68k nommu case. I am not aware of any existing issues caused by
>> this - but there is no good reason not to fix it.
>>
>> So I propose this change. Build and run tested on my m68knommu targets.
>>
>> Regards
>> Greg
>>
>>
>>   From f809fb8fbca9e5e637b8fda380955bd799bb3926 Mon Sep 17 00:00:00 2001
>> From: Greg Ungerer <gerg@linux-m68k.org>
>> Date: Wed, 20 Apr 2022 23:27:47 +1000
>> Subject: [PATCH] m68knommu: set ZERO_PAGE() allocated zeroed page
>>
>> The non-MMU m68k pagetable ZERO_PAGE() macro is being set to the
>> somewhat non-sensical value of "virt_to_page(0)". The zeroth page
>> is not in any way guaranteed to be a page full of "0". So the result
>> is that ZERO_PAGE() will almost certainly contain random values.
>>
>> We already allocate a real "empty_zero_page" in the mm setup code shared
>> between MMU m68k and non-MMU m68k. It is just not hooked up to the
>> ZERO_PAGE() macro for the non-MMU m68k case.
>>
>> Fix ZERO_PAGE() to use the allocated "empty_zero_page" pointer.
>>
>> I am not aware of any specific issues caused by the old code.
>>
>> Link: https://lore.kernel.org/linux-m68k/2a462b23-5b8e-bbf4-ec7d-778434a3b9d7@google.com/T/#t
>> Reported-by: Hugh Dickens <hughd@google.com>
>> Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
>> ---
>>    arch/m68k/include/asm/pgtable_no.h | 3 ++-
>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/m68k/include/asm/pgtable_no.h b/arch/m68k/include/asm/pgtable_no.h
>> index 87151d67d91e..bce5ca56c388 100644
>> --- a/arch/m68k/include/asm/pgtable_no.h
>> +++ b/arch/m68k/include/asm/pgtable_no.h
>> @@ -42,7 +42,8 @@ extern void paging_init(void);
>>     * ZERO_PAGE is a global shared page that is always zero: used
>>     * for zero-mapped memory areas etc..
>>     */
>> -#define ZERO_PAGE(vaddr)       (virt_to_page(0))
>> +extern void *empty_zero_page;
>> +#define ZERO_PAGE(vaddr)       (virt_to_page(empty_zero_page))
>>
>>    /*
>>     * All 32bit addresses are effectively valid for vmalloc...
> 
> And after that (or combined with this?), this can be factored
> out from arch/m68k/include/asm/pgtable_{mm,no}.h into
> arch/m68k/include/asm/pgtable.h.

I think a new patch to do that work on top of this one would be best.
I will work on that.

Regards
Greg



> 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