Re: [PATCH] ARM: mmu: fix access to illegal address when using earlycon & memblock=debug

From: Victor Hassan
Date: Sat Sep 03 2022 - 04:54:53 EST

Dear Rob,

On 2022/9/1 21:21, Rob Herring wrote:
On Thu, Sep 1, 2022 at 7:54 AM Victor Hassan <victor@xxxxxxxxxxxxxxxxx> wrote:



On 2022/8/31 20:37, Victor Hassan wrote:
On 8/31/2022 7:52 PM, Marek Szyprowski wrote:
Hi Victor,

On 16.03.2022 03:33, Victor Hassan wrote:
earlycon uses fixmap to create a memory map,
So we need to close earlycon before closing fixmap,
otherwise printk will access illegal addresses.

How? Due to recent changes in how printk and the consoles work or just
because create_mapping() can print? In the latter case, the only
variable input is the phys address. I think most if not all prints
cannot occur.

After creating a new memory map, we open earlycon again.

Signed-off-by: Victor Hassan <victor@xxxxxxxxxxxxxxxxx>

This patch landed in linux next-20220831 as commit a76886d117cb ("ARM:
9223/1: mmu: fix access to illegal address when using earlycon &
memblock=debug"). Unfortunately it breaks booting of all my test boards
which *do not* use earlycon. It can be easily reproduced even with QEMU.

With kernel compiled from multi_v7_defconfig the following setup boots:

$ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append
"console=ttyAMA0 earlycon" -M virt -smp 2 -m 512

while this one doesn't:

$ qemu-system-arm -nographic -kernel arch/arm/boot/zImage -append
"console=ttyAMA0" -M virt -smp 2 -m 512


---
arch/arm/mm/mmu.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 274e4f73fd33..f3511f07a7d0 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -14,6 +14,7 @@
#include <linux/fs.h>
#include <linux/vmalloc.h>
#include <linux/sizes.h>
+#include <linux/console.h>
#include <asm/cp15.h>
#include <asm/cputype.h>
@@ -1695,6 +1696,9 @@ static void __init early_fixmap_shutdown(void)
pmd_clear(fixmap_pmd(va));
local_flush_tlb_kernel_page(va);
+#ifdef CONFIG_FIX_EARLYCON_MEM
+ console_stop(console_drivers);
+#endif
for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
pte_t *pte;
struct map_desc map;
@@ -1713,6 +1717,9 @@ static void __init early_fixmap_shutdown(void)
create_mapping(&map);
}
+#ifdef CONFIG_FIX_EARLYCON_MEM
+ console_start(console_drivers);
+#endif
}
/*

Best regards

Dear Marek,
Thank you for the notice. I'll figure it out and feed back to you as
soon as possible.

Regards,
Victor

Hi Marek,

Sorry, didn't take into account that console_drivers is NULL when
earlycon is not used.

Here is the patch-v2. Please review:

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index a49f0b9..a240f38 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -14,6 +14,7 @@
#include <linux/fs.h>
#include <linux/vmalloc.h>
#include <linux/sizes.h>
+#include <linux/console.h>

#include <asm/cp15.h>
#include <asm/cputype.h>
@@ -1730,6 +1731,10 @@
pmd_clear(fixmap_pmd(va));
local_flush_tlb_kernel_page(va);

+#ifdef CONFIG_FIX_EARLYCON_MEM

This is always true for CONFIG_MMU and this file is only built for
CONFIG_MMU. So you don't need it.

Yes, you are right.


+ if (console_drivers)
+ console_stop(console_drivers);

console_drivers is a list, so you are only stopping the 1st one.
Couldn't console_lock() be used here?


Thanks for the suggestion: console_lock is actually the same as console_stop in the test, and the code is more compact.

Also, this should be before pmd_clear().

During the test, I found that the console failed after executing local_flush_tlb_kernel_page, so I think the pmd_clear function can output in time if there is printing. This doesn't seem possible, so before pmd_clear it's not bad either.


+#endif
for (i = 0; i < __end_of_permanent_fixed_addresses; i++) {
pte_t *pte;
struct map_desc map;
@@ -1748,6 +1753,10 @@

create_mapping(&map);
}
+#ifdef CONFIG_FIX_EARLYCON_MEM
+ if (console_drivers)
+ console_start(console_drivers);
+#endif
}

BTW, should I resend the patch-v2 through the site
(https://www.armlinux.org.uk/developer/patches/add.php), or should I
send the patch-v2 through E-mail to Linux-Mainline?

Thanks you.

Regards,
Victor

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Here is the patch-v3. Please review:

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index a49f0b9..57ca77f 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -14,6 +14,7 @@
#include <linux/fs.h>
#include <linux/vmalloc.h>
#include <linux/sizes.h>
+#include <linux/console.h>

#include <asm/cp15.h>
#include <asm/cputype.h>
@@ -1727,6 +1728,7 @@
unsigned long va = fix_to_virt(__end_of_permanent_fixed_addresses - 1);

pte_offset_fixmap = pte_offset_late_fixmap;
+ console_lock();
pmd_clear(fixmap_pmd(va));
local_flush_tlb_kernel_page(va);

@@ -1748,6 +1750,7 @@

create_mapping(&map);
}
+ console_unlock();
}

Thanks you.

Regards,
Victor