2022-03-17 05:35:53

by Victor Hassan

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

earlycon uses fixmap to create a memory map,
So we need to close earlycon before closing fixmap,
otherwise printk will access illegal addresses.
After creating a new memory map, we open earlycon again.

Signed-off-by: Victor Hassan <[email protected]>
---
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
}

/*
--
2.29.0


2022-04-18 12:24:14

by Linus Walleij

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

On Wed, Mar 16, 2022 at 3:33 AM Victor Hassan <[email protected]> wrote:

> earlycon uses fixmap to create a memory map,
> So we need to close earlycon before closing fixmap,
> otherwise printk will access illegal addresses.
> After creating a new memory map, we open earlycon again.
>
> Signed-off-by: Victor Hassan <[email protected]>

I think noone really noticed this because everyone on Arm systems
use CONFIG_DEBUG_LL, and that makes printascii hammer out
stuff on the console very early, it even accounts for whether we have
MMU on or not.

How are you using this on Arm even? What system and what serial
driver?

That said, it looks correct.
Acked-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

2022-04-19 01:06:32

by Victor Hassan

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

On 18/04/2022 07:21, Linus Walleij wrote:
> On Wed, Mar 16, 2022 at 3:33 AM Victor Hassan <[email protected]> wrote:
>
>> earlycon uses fixmap to create a memory map,
>> So we need to close earlycon before closing fixmap,
>> otherwise printk will access illegal addresses.
>> After creating a new memory map, we open earlycon again.
>>
>> Signed-off-by: Victor Hassan <[email protected]>
>
> I think noone really noticed this because everyone on Arm systems
> use CONFIG_DEBUG_LL, and that makes printascii hammer out
> stuff on the console very early, it even accounts for whether we have
> MMU on or not.
>
Hi Linus,
Thank you for the reply. I used earlycon, in early_fixmap_shutdown, the
base address of earlycon is in the critical stage of release and
reassignment, so early_fixmap_shutdown -> create_mapping should not call
earlycon in this process, and create_mapping has a lot of conditions
that trigger print, memblock=debug just makes it easier to expose problems.

> How are you using this on Arm even? What system and what serial
> driver?
I'm using serial driver 8250 on arm32, with cmdline: memblock=debug.
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8520_CONSOLE=y
>
> That said, it looks correct.
> Acked-by: Linus Walleij <[email protected]>
Thank you.
>
> Yours,
> Linus Walleij

Sincerely,
Victor Hassan

2022-06-17 14:00:06

by Victor Hassan

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

On 4/18/2022 11:08 PM, Victor Hassan wrote:
> On 18/04/2022 07:21, Linus Walleij wrote:
>> On Wed, Mar 16, 2022 at 3:33 AM Victor Hassan
>> <[email protected]> wrote:
>>
>>> earlycon uses fixmap to create a memory map,
>>> So we need to close earlycon before closing fixmap,
>>> otherwise printk will access illegal addresses.
>>> After creating a new memory map, we open earlycon again.
>>>
>>> Signed-off-by: Victor Hassan <[email protected]>
>>
>> I think noone really noticed this because everyone on Arm systems
>> use CONFIG_DEBUG_LL, and that makes printascii hammer out
>> stuff on the console very early, it even accounts for whether we have
>> MMU on or not.
>>
> Hi Linus,
> Thank you for the reply. I used earlycon, in early_fixmap_shutdown, the
> base address of earlycon is in the critical stage of release and
> reassignment, so early_fixmap_shutdown -> create_mapping should not call
> earlycon in this process, and create_mapping has a lot of conditions
> that trigger print, memblock=debug just makes it easier to expose problems.
>
>> How are you using this on Arm even? What system and what serial
>> driver?
> I'm using serial driver 8250 on arm32, with cmdline: memblock=debug.
> CONFIG_SERIAL_8250=y
> CONFIG_SERIAL_8520_CONSOLE=y
>>
>> That said, it looks correct.
>> Acked-by: Linus Walleij <[email protected]>

Hi Linus,
Sorry to disturb. Is there any question about this issue? Thank you :)

2022-06-28 08:49:11

by Linus Walleij

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

On Fri, Jun 17, 2022 at 3:31 PM Victor Hassan <[email protected]> wrote:
> On 4/18/2022 11:08 PM, Victor Hassan wrote:
> > On 18/04/2022 07:21, Linus Walleij wrote:
> >> On Wed, Mar 16, 2022 at 3:33 AM Victor Hassan
> >> <[email protected]> wrote:
> >>
> >>> earlycon uses fixmap to create a memory map,
> >>> So we need to close earlycon before closing fixmap,
> >>> otherwise printk will access illegal addresses.
> >>> After creating a new memory map, we open earlycon again.
> >>>
> >>> Signed-off-by: Victor Hassan <[email protected]>
> >>
> >> I think noone really noticed this because everyone on Arm systems
> >> use CONFIG_DEBUG_LL, and that makes printascii hammer out
> >> stuff on the console very early, it even accounts for whether we have
> >> MMU on or not.
> >>
> > Hi Linus,
> > Thank you for the reply. I used earlycon, in early_fixmap_shutdown, the
> > base address of earlycon is in the critical stage of release and
> > reassignment, so early_fixmap_shutdown -> create_mapping should not call
> > earlycon in this process, and create_mapping has a lot of conditions
> > that trigger print, memblock=debug just makes it easier to expose problems.
> >
> >> How are you using this on Arm even? What system and what serial
> >> driver?
> > I'm using serial driver 8250 on arm32, with cmdline: memblock=debug.
> > CONFIG_SERIAL_8250=y
> > CONFIG_SERIAL_8520_CONSOLE=y
> >>
> >> That said, it looks correct.
> >> Acked-by: Linus Walleij <[email protected]>
>
> Hi Linus,
> Sorry to disturb. Is there any question about this issue? Thank you :)

No, I understand the problem, I provided my ACK.

Maybe you wanna add the patch to Russell's patch tracker so
he can apply it?
https://www.arm.linux.org.uk/developer/patches/

Yours,
Linus Walleij

2022-08-31 11:57:09

by Marek Szyprowski

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

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.
> After creating a new memory map, we open earlycon again.
>
> Signed-off-by: Victor Hassan <[email protected]>

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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

2022-08-31 13:36:01

by Victor Hassan

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

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.
>> After creating a new memory map, we open earlycon again.
>>
>> Signed-off-by: Victor Hassan <[email protected]>
>
> 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

2022-08-31 14:06:07

by Russell King (Oracle)

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

On Wed, Aug 31, 2022 at 01:52:57PM +0200, 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.
> > After creating a new memory map, we open earlycon again.
> >
> > Signed-off-by: Victor Hassan <[email protected]>
>
> 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

That's the second patch to drop from my merge yesterday... not good.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

2022-09-01 12:58:26

by Victor Hassan

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



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.
>>> After creating a new memory map, we open earlycon again.
>>>
>>> Signed-off-by: Victor Hassan <[email protected]>
>>
>> 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
+ if (console_drivers)
+ console_stop(console_drivers);
+#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

2022-09-01 13:58:34

by Rob Herring (Arm)

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

On Thu, Sep 1, 2022 at 7:54 AM Victor Hassan <[email protected]> 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 <[email protected]>
> >>
> >> 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.

> + 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?

Also, this should be before pmd_clear().

> +#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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2022-09-01 14:42:37

by Rob Herring (Arm)

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

On Thu, Sep 1, 2022 at 8:21 AM Rob Herring <[email protected]> wrote:
>
> On Thu, Sep 1, 2022 at 7:54 AM Victor Hassan <[email protected]> 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.

Ah, I missed the memblock debug part on the subject...

Rob

2022-09-03 09:32:35

by Victor Hassan

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

Dear Rob,

On 2022/9/1 21:21, Rob Herring wrote:
> On Thu, Sep 1, 2022 at 7:54 AM Victor Hassan <[email protected]> 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 <[email protected]>
>>>>
>>>> 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
>> [email protected]
>> 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