[PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[quanyang.wang]

From: Quanyang Wang <quanyang.wang@windriver.com>

Not only the early fixmap range, but also the fixmap range should be
checked if it spans multiple pmds. When enabling CONFIG_DEBUG_HIGHMEM,
some systems which contain up to 16 CPUs will crash.

Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
---
 arch/arm/mm/mmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index a4e0060051070..57eed92073a4a 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -369,6 +369,8 @@ void __init early_fixmap_init(void)
 	 */
 	BUILD_BUG_ON((__fix_to_virt(__end_of_early_ioremap_region) >> PMD_SHIFT)
 		     != FIXADDR_TOP >> PMD_SHIFT);
+	BUILD_BUG_ON((__fix_to_virt(__end_of_fixmap_region) >> PMD_SHIFT)
+		     != FIXADDR_TOP >> PMD_SHIFT);
 
 	pmd = fixmap_pmd(FIXADDR_TOP);
 	pmd_populate_kernel(&init_mm, pmd, bm_pte);
-- 
2.25.1

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Linus Walleij]

On Wed, Oct 20, 2021 at 7:50 AM <quanyang.wang@windriver.com> wrote:

> From: Quanyang Wang <quanyang.wang@windriver.com>
>
> Not only the early fixmap range, but also the fixmap range should be
> checked if it spans multiple pmds. When enabling CONFIG_DEBUG_HIGHMEM,
> some systems which contain up to 16 CPUs will crash.
>
> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>

Looks reasonable to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Please submit this patch into Russell's patch tracker.

Yours,
Linus Walleij

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Quanyang Wang]

On 10/25/21 5:44 AM, Linus Walleij wrote:
> On Wed, Oct 20, 2021 at 7:50 AM <quanyang.wang@windriver.com> wrote:
> 
>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>
>> Not only the early fixmap range, but also the fixmap range should be
>> checked if it spans multiple pmds. When enabling CONFIG_DEBUG_HIGHMEM,
>> some systems which contain up to 16 CPUs will crash.
>>
>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> 
> Looks reasonable to me.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Please submit this patch into Russell's patch tracker.
Done.
https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=9149/1
Thanks for the review.

> 
> Yours,
> Linus Walleij
> 

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Russell King (Oracle)]

On Sun, Oct 24, 2021 at 11:44:31PM +0200, Linus Walleij wrote:
> On Wed, Oct 20, 2021 at 7:50 AM <quanyang.wang@windriver.com> wrote:
> 
> > From: Quanyang Wang <quanyang.wang@windriver.com>
> >
> > Not only the early fixmap range, but also the fixmap range should be
> > checked if it spans multiple pmds. When enabling CONFIG_DEBUG_HIGHMEM,
> > some systems which contain up to 16 CPUs will crash.
> >
> > Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> 
> Looks reasonable to me.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Please submit this patch into Russell's patch tracker.

... and has totally broken what looks like _all_ ARM kernel builds. It
can not have been tested. Maybe it's uncovered a previously unknown
problem, but causing such a wide-range regression is disappointing.
I'm going to revert this commit.

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

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Quanyang Wang]

Hi,
Sorry for the inconvenience.

On 10/26/21 4:59 PM, Russell King (Oracle) wrote:
> On Sun, Oct 24, 2021 at 11:44:31PM +0200, Linus Walleij wrote:
>> On Wed, Oct 20, 2021 at 7:50 AM <quanyang.wang@windriver.com> wrote:
>>
>>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>>
>>> Not only the early fixmap range, but also the fixmap range should be
>>> checked if it spans multiple pmds. When enabling CONFIG_DEBUG_HIGHMEM,
>>> some systems which contain up to 16 CPUs will crash.
>>>
>>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
>>
>> Looks reasonable to me.
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Please submit this patch into Russell's patch tracker.
> 
> ... and has totally broken what looks like _all_ ARM kernel builds. 
This patch is intended to trigger build error when it check the value of
__end_of_fixmap_region is equal or larger than 256.
In fact, it breaks the ARM kernel builds which NR_CPUS is equal or more 
than 16. If CONFIG_DEBUG_HIGHMEM is enabled, all ARM builds which 
NR_CPUS is more than 8 will fail.
It
> can not have been tested. 
I tested this patch with allyesconfig instead of some configs in 
arch/arm/configs/. In allyesconfig, NR_CPUS is 4, so it not trigger 
build error. Then I changed it to 8 to verify my patch.
Maybe it's uncovered a previously unknown
> problem, 
Yes, at my side, axm5516 with CONFIG_DEBUG_HIGHMEM always falls into 
crash. Other ARM platform which contains more than 8 CPUs may encounter
the same issue.
but causing such a wide-range regression is disappointing.
Sorry for not consider this thoroughly.

Thanks,
Quanyang
> I'm going to revert this commit.
> 

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Ard Biesheuvel]

On Tue, 26 Oct 2021 at 11:53, Quanyang Wang <quanyang.wang@windriver.com> wrote:
>
> Hi,
> Sorry for the inconvenience.
>
> On 10/26/21 4:59 PM, Russell King (Oracle) wrote:
> > On Sun, Oct 24, 2021 at 11:44:31PM +0200, Linus Walleij wrote:
> >> On Wed, Oct 20, 2021 at 7:50 AM <quanyang.wang@windriver.com> wrote:
> >>
> >>> From: Quanyang Wang <quanyang.wang@windriver.com>
> >>>
> >>> Not only the early fixmap range, but also the fixmap range should be
> >>> checked if it spans multiple pmds. When enabling CONFIG_DEBUG_HIGHMEM,
> >>> some systems which contain up to 16 CPUs will crash.
> >>>
> >>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> >>
> >> Looks reasonable to me.
> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>
> >> Please submit this patch into Russell's patch tracker.
> >
> > ... and has totally broken what looks like _all_ ARM kernel builds.
> This patch is intended to trigger build error when it check the value of
> __end_of_fixmap_region is equal or larger than 256.

Why? The fixmap region is larger than one PMD, so why do we need to cap it?

> In fact, it breaks the ARM kernel builds which NR_CPUS is equal or more
> than 16. If CONFIG_DEBUG_HIGHMEM is enabled, all ARM builds which
> NR_CPUS is more than 8 will fail.

You really need to be more specific about the failure mode here.

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Quanyang Wang]

Hi Ard,

On 10/26/21 6:12 PM, Ard Biesheuvel wrote:
> On Tue, 26 Oct 2021 at 11:53, Quanyang Wang <quanyang.wang@windriver.com> wrote:
>>
>> Hi,
>> Sorry for the inconvenience.
>>
>> On 10/26/21 4:59 PM, Russell King (Oracle) wrote:
>>> On Sun, Oct 24, 2021 at 11:44:31PM +0200, Linus Walleij wrote:
>>>> On Wed, Oct 20, 2021 at 7:50 AM <quanyang.wang@windriver.com> wrote:
>>>>
>>>>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>>>>
>>>>> Not only the early fixmap range, but also the fixmap range should be
>>>>> checked if it spans multiple pmds. When enabling CONFIG_DEBUG_HIGHMEM,
>>>>> some systems which contain up to 16 CPUs will crash.
>>>>>
>>>>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
>>>>
>>>> Looks reasonable to me.
>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>
>>>> Please submit this patch into Russell's patch tracker.
>>>
>>> ... and has totally broken what looks like _all_ ARM kernel builds.
>> This patch is intended to trigger build error when it check the value of
>> __end_of_fixmap_region is equal or larger than 256.
> 
> Why? The fixmap region is larger than one PMD, so why do we need to cap it?
In __kmap_local_pfn_prot, arch_kmap_local_set_pte(&init_mm, vaddr, 
kmap_pte - idx, pteval) is used to set pteval.
But the ptep is calculated by "kmap_pte - idx", which means all ptes 
must be placed next to each other and no gaps. But for ARM, the ptes for 
the range "0xffe00000~0xfff00000" is not next to the ptes for the range
"0xffc80000~0xffdfffff".

When the idx is larger than 256, virtual address is in 0xffdxxxxx, 
access this address will crash since its pteval isn't set correctly.

> 
>> In fact, it breaks the ARM kernel builds which NR_CPUS is equal or more
>> than 16. If CONFIG_DEBUG_HIGHMEM is enabled, all ARM builds which
>> NR_CPUS is more than 8 will fail.
> 
> You really need to be more specific about the failure mode here.
OK, I will be more careful about this.

Thanks,
Quanyang
> 

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Russell King (Oracle)]

On Tue, Oct 26, 2021 at 06:38:16PM +0800, Quanyang Wang wrote:
> Hi Ard,
> 
> On 10/26/21 6:12 PM, Ard Biesheuvel wrote:
> > On Tue, 26 Oct 2021 at 11:53, Quanyang Wang <quanyang.wang@windriver.com> wrote:
> > > 
> > > Hi,
> > > Sorry for the inconvenience.
> > > 
> > > On 10/26/21 4:59 PM, Russell King (Oracle) wrote:
> > > > On Sun, Oct 24, 2021 at 11:44:31PM +0200, Linus Walleij wrote:
> > > > > On Wed, Oct 20, 2021 at 7:50 AM <quanyang.wang@windriver.com> wrote:
> > > > > 
> > > > > > From: Quanyang Wang <quanyang.wang@windriver.com>
> > > > > > 
> > > > > > Not only the early fixmap range, but also the fixmap range should be
> > > > > > checked if it spans multiple pmds. When enabling CONFIG_DEBUG_HIGHMEM,
> > > > > > some systems which contain up to 16 CPUs will crash.
> > > > > > 
> > > > > > Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> > > > > 
> > > > > Looks reasonable to me.
> > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > > 
> > > > > Please submit this patch into Russell's patch tracker.
> > > > 
> > > > ... and has totally broken what looks like _all_ ARM kernel builds.
> > > This patch is intended to trigger build error when it check the value of
> > > __end_of_fixmap_region is equal or larger than 256.
> > 
> > Why? The fixmap region is larger than one PMD, so why do we need to cap it?
> In __kmap_local_pfn_prot, arch_kmap_local_set_pte(&init_mm, vaddr, kmap_pte
> - idx, pteval) is used to set pteval.
> But the ptep is calculated by "kmap_pte - idx", which means all ptes must be
> placed next to each other and no gaps. But for ARM, the ptes for the range
> "0xffe00000~0xfff00000" is not next to the ptes for the range
> "0xffc80000~0xffdfffff".
> 
> When the idx is larger than 256, virtual address is in 0xffdxxxxx, access
> this address will crash since its pteval isn't set correctly.

Thanks for the explanation.

Sadly, this does seem to be correct. Even if the PTE tables are
located next to each other in memory, they _still_ won't be a
contiguous array of entries due to being interleaved with the Linux
PTE table and the hardware PTE table.

Since the address range 0xffe00000-0xfff00000 is already half of one
PTE table containing 512 contiguous entries, we are limited to 256
fixmap PTEs maximum. If we have more than that we will start trampling
over memory below the PTE table _and_ we will start corrupting Linux
PTE entries in the 0xfff00000-0xffffffff range.

I suspect this hasn't been seen because of a general lack of ARM
systems with more than 4 CPUs.

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

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Ard Biesheuvel]

On Tue, 26 Oct 2021 at 12:55, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Tue, Oct 26, 2021 at 06:38:16PM +0800, Quanyang Wang wrote:
> > Hi Ard,
> >
> > On 10/26/21 6:12 PM, Ard Biesheuvel wrote:
> > > On Tue, 26 Oct 2021 at 11:53, Quanyang Wang <quanyang.wang@windriver.com> wrote:
> > > >
> > > > Hi,
> > > > Sorry for the inconvenience.
> > > >
> > > > On 10/26/21 4:59 PM, Russell King (Oracle) wrote:
> > > > > On Sun, Oct 24, 2021 at 11:44:31PM +0200, Linus Walleij wrote:
> > > > > > On Wed, Oct 20, 2021 at 7:50 AM <quanyang.wang@windriver.com> wrote:
> > > > > >
> > > > > > > From: Quanyang Wang <quanyang.wang@windriver.com>
> > > > > > >
> > > > > > > Not only the early fixmap range, but also the fixmap range should be
> > > > > > > checked if it spans multiple pmds. When enabling CONFIG_DEBUG_HIGHMEM,
> > > > > > > some systems which contain up to 16 CPUs will crash.
> > > > > > >
> > > > > > > Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> > > > > >
> > > > > > Looks reasonable to me.
> > > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > > >
> > > > > > Please submit this patch into Russell's patch tracker.
> > > > >
> > > > > ... and has totally broken what looks like _all_ ARM kernel builds.
> > > > This patch is intended to trigger build error when it check the value of
> > > > __end_of_fixmap_region is equal or larger than 256.
> > >
> > > Why? The fixmap region is larger than one PMD, so why do we need to cap it?
> > In __kmap_local_pfn_prot, arch_kmap_local_set_pte(&init_mm, vaddr, kmap_pte
> > - idx, pteval) is used to set pteval.
> > But the ptep is calculated by "kmap_pte - idx", which means all ptes must be
> > placed next to each other and no gaps. But for ARM, the ptes for the range
> > "0xffe00000~0xfff00000" is not next to the ptes for the range
> > "0xffc80000~0xffdfffff".
> >
> > When the idx is larger than 256, virtual address is in 0xffdxxxxx, access
> > this address will crash since its pteval isn't set correctly.
>
> Thanks for the explanation.
>
> Sadly, this does seem to be correct. Even if the PTE tables are
> located next to each other in memory, they _still_ won't be a
> contiguous array of entries due to being interleaved with the Linux
> PTE table and the hardware PTE table.
>
> Since the address range 0xffe00000-0xfff00000 is already half of one
> PTE table containing 512 contiguous entries, we are limited to 256
> fixmap PTEs maximum. If we have more than that we will start trampling
> over memory below the PTE table _and_ we will start corrupting Linux
> PTE entries in the 0xfff00000-0xffffffff range.
>
> I suspect this hasn't been seen because of a general lack of ARM
> systems with more than 4 CPUs.
>

But doesn't that make it a kmap_local regression? Or do you think this
issue existed before that as well?

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Russell King (Oracle)]

On Tue, Oct 26, 2021 at 12:56:08PM +0200, Ard Biesheuvel wrote:
> On Tue, 26 Oct 2021 at 12:55, Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Tue, Oct 26, 2021 at 06:38:16PM +0800, Quanyang Wang wrote:
> > > Hi Ard,
> > >
> > > On 10/26/21 6:12 PM, Ard Biesheuvel wrote:
> > > > On Tue, 26 Oct 2021 at 11:53, Quanyang Wang <quanyang.wang@windriver.com> wrote:
> > > > >
> > > > > Hi,
> > > > > Sorry for the inconvenience.
> > > > >
> > > > > On 10/26/21 4:59 PM, Russell King (Oracle) wrote:
> > > > > > On Sun, Oct 24, 2021 at 11:44:31PM +0200, Linus Walleij wrote:
> > > > > > > On Wed, Oct 20, 2021 at 7:50 AM <quanyang.wang@windriver.com> wrote:
> > > > > > >
> > > > > > > > From: Quanyang Wang <quanyang.wang@windriver.com>
> > > > > > > >
> > > > > > > > Not only the early fixmap range, but also the fixmap range should be
> > > > > > > > checked if it spans multiple pmds. When enabling CONFIG_DEBUG_HIGHMEM,
> > > > > > > > some systems which contain up to 16 CPUs will crash.
> > > > > > > >
> > > > > > > > Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> > > > > > >
> > > > > > > Looks reasonable to me.
> > > > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > > > >
> > > > > > > Please submit this patch into Russell's patch tracker.
> > > > > >
> > > > > > ... and has totally broken what looks like _all_ ARM kernel builds.
> > > > > This patch is intended to trigger build error when it check the value of
> > > > > __end_of_fixmap_region is equal or larger than 256.
> > > >
> > > > Why? The fixmap region is larger than one PMD, so why do we need to cap it?
> > > In __kmap_local_pfn_prot, arch_kmap_local_set_pte(&init_mm, vaddr, kmap_pte
> > > - idx, pteval) is used to set pteval.
> > > But the ptep is calculated by "kmap_pte - idx", which means all ptes must be
> > > placed next to each other and no gaps. But for ARM, the ptes for the range
> > > "0xffe00000~0xfff00000" is not next to the ptes for the range
> > > "0xffc80000~0xffdfffff".
> > >
> > > When the idx is larger than 256, virtual address is in 0xffdxxxxx, access
> > > this address will crash since its pteval isn't set correctly.
> >
> > Thanks for the explanation.
> >
> > Sadly, this does seem to be correct. Even if the PTE tables are
> > located next to each other in memory, they _still_ won't be a
> > contiguous array of entries due to being interleaved with the Linux
> > PTE table and the hardware PTE table.
> >
> > Since the address range 0xffe00000-0xfff00000 is already half of one
> > PTE table containing 512 contiguous entries, we are limited to 256
> > fixmap PTEs maximum. If we have more than that we will start trampling
> > over memory below the PTE table _and_ we will start corrupting Linux
> > PTE entries in the 0xfff00000-0xffffffff range.
> >
> > I suspect this hasn't been seen because of a general lack of ARM
> > systems with more than 4 CPUs.
> >
> 
> But doesn't that make it a kmap_local regression? Or do you think this
> issue existed before that as well?

It definitely is a bug in tglx's kmap_local code, which assumes all
PTEs in the fixmap region are contiguously arranged.

Looking back further, when local kmaps were handled in arch code, this
bug did /not/ exist. We used to get the PTE entry to update via:

	unsigned long vaddr = __fix_to_virt(idx);
	pte_t *ptep = pte_offset_kernel(pmd_off_k(vaddr), vaddr);

which later became:

	pte_t *ptep = virt_to_kpte(vaddr);

Both of which walk the page tables.

So in summary a regression caused by converting ARM to kmap_local.

I think we could fix it by providing our own arch_kmap_local_set_pte()
which ignores the ptep argument, and instead walks the page tables
using the vaddr argument.

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

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Ard Biesheuvel]

On Tue, 26 Oct 2021 at 13:16, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Tue, Oct 26, 2021 at 12:56:08PM +0200, Ard Biesheuvel wrote:
> > On Tue, 26 Oct 2021 at 12:55, Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Tue, Oct 26, 2021 at 06:38:16PM +0800, Quanyang Wang wrote:
> > > > Hi Ard,
> > > >
> > > > On 10/26/21 6:12 PM, Ard Biesheuvel wrote:
> > > > > On Tue, 26 Oct 2021 at 11:53, Quanyang Wang <quanyang.wang@windriver.com> wrote:
...
> > > > But the ptep is calculated by "kmap_pte - idx", which means all ptes must be
> > > > placed next to each other and no gaps. But for ARM, the ptes for the range
> > > > "0xffe00000~0xfff00000" is not next to the ptes for the range
> > > > "0xffc80000~0xffdfffff".
> > > >
> > > > When the idx is larger than 256, virtual address is in 0xffdxxxxx, access
> > > > this address will crash since its pteval isn't set correctly.
> > >
> > > Thanks for the explanation.
> > >
> > > Sadly, this does seem to be correct. Even if the PTE tables are
> > > located next to each other in memory, they _still_ won't be a
> > > contiguous array of entries due to being interleaved with the Linux
> > > PTE table and the hardware PTE table.
> > >
> > > Since the address range 0xffe00000-0xfff00000 is already half of one
> > > PTE table containing 512 contiguous entries, we are limited to 256
> > > fixmap PTEs maximum. If we have more than that we will start trampling
> > > over memory below the PTE table _and_ we will start corrupting Linux
> > > PTE entries in the 0xfff00000-0xffffffff range.
> > >
> > > I suspect this hasn't been seen because of a general lack of ARM
> > > systems with more than 4 CPUs.
> > >
> >
> > But doesn't that make it a kmap_local regression? Or do you think this
> > issue existed before that as well?
>
> It definitely is a bug in tglx's kmap_local code, which assumes all
> PTEs in the fixmap region are contiguously arranged.
>
> Looking back further, when local kmaps were handled in arch code, this
> bug did /not/ exist. We used to get the PTE entry to update via:
>
>         unsigned long vaddr = __fix_to_virt(idx);
>         pte_t *ptep = pte_offset_kernel(pmd_off_k(vaddr), vaddr);
>
> which later became:
>
>         pte_t *ptep = virt_to_kpte(vaddr);
>
> Both of which walk the page tables.
>
> So in summary a regression caused by converting ARM to kmap_local.
>
> I think we could fix it by providing our own arch_kmap_local_set_pte()
> which ignores the ptep argument, and instead walks the page tables
> using the vaddr argument.
>

Removing all occurrences of 'kmap_pte - idx' and replacing them with
virt_to_kpte() seems to do the trick. Unfortunately, these occur in
other places as well, not only on the map path, so I doubt that
overriding arch_kmap_local_set_pte will be sufficient.

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[Russell King (Oracle)]

On Tue, Oct 26, 2021 at 01:26:05PM +0200, Ard Biesheuvel wrote:
> On Tue, 26 Oct 2021 at 13:16, Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Tue, Oct 26, 2021 at 12:56:08PM +0200, Ard Biesheuvel wrote:
> > > On Tue, 26 Oct 2021 at 12:55, Russell King (Oracle)
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Tue, Oct 26, 2021 at 06:38:16PM +0800, Quanyang Wang wrote:
> > > > > Hi Ard,
> > > > >
> > > > > On 10/26/21 6:12 PM, Ard Biesheuvel wrote:
> > > > > > On Tue, 26 Oct 2021 at 11:53, Quanyang Wang <quanyang.wang@windriver.com> wrote:
> ...
> > > > > But the ptep is calculated by "kmap_pte - idx", which means all ptes must be
> > > > > placed next to each other and no gaps. But for ARM, the ptes for the range
> > > > > "0xffe00000~0xfff00000" is not next to the ptes for the range
> > > > > "0xffc80000~0xffdfffff".
> > > > >
> > > > > When the idx is larger than 256, virtual address is in 0xffdxxxxx, access
> > > > > this address will crash since its pteval isn't set correctly.
> > > >
> > > > Thanks for the explanation.
> > > >
> > > > Sadly, this does seem to be correct. Even if the PTE tables are
> > > > located next to each other in memory, they _still_ won't be a
> > > > contiguous array of entries due to being interleaved with the Linux
> > > > PTE table and the hardware PTE table.
> > > >
> > > > Since the address range 0xffe00000-0xfff00000 is already half of one
> > > > PTE table containing 512 contiguous entries, we are limited to 256
> > > > fixmap PTEs maximum. If we have more than that we will start trampling
> > > > over memory below the PTE table _and_ we will start corrupting Linux
> > > > PTE entries in the 0xfff00000-0xffffffff range.
> > > >
> > > > I suspect this hasn't been seen because of a general lack of ARM
> > > > systems with more than 4 CPUs.
> > > >
> > >
> > > But doesn't that make it a kmap_local regression? Or do you think this
> > > issue existed before that as well?
> >
> > It definitely is a bug in tglx's kmap_local code, which assumes all
> > PTEs in the fixmap region are contiguously arranged.
> >
> > Looking back further, when local kmaps were handled in arch code, this
> > bug did /not/ exist. We used to get the PTE entry to update via:
> >
> >         unsigned long vaddr = __fix_to_virt(idx);
> >         pte_t *ptep = pte_offset_kernel(pmd_off_k(vaddr), vaddr);
> >
> > which later became:
> >
> >         pte_t *ptep = virt_to_kpte(vaddr);
> >
> > Both of which walk the page tables.
> >
> > So in summary a regression caused by converting ARM to kmap_local.
> >
> > I think we could fix it by providing our own arch_kmap_local_set_pte()
> > which ignores the ptep argument, and instead walks the page tables
> > using the vaddr argument.
> >
> 
> Removing all occurrences of 'kmap_pte - idx' and replacing them with
> virt_to_kpte() seems to do the trick. Unfortunately, these occur in
> other places as well, not only on the map path, so I doubt that
> overriding arch_kmap_local_set_pte will be sufficient.

Right. That's probably going to upset some folk if we make everyone
walk page tables, so we probably need to add overrideable macros
just like arch_kmap_local_set_pte() is... which feels rather yucky.

tglx - any opinions on how you'd like this regression to be fixed?

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

Re: [PATCH] ARM: add BUILD_BUG_ON to check if fixmap range spans multiple pmds

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5551 bytes --]

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on arm/for-next]
[also build test ERROR on xilinx-xlnx/master soc/for-next rockchip/for-next arm64/for-next/core shawnguo/for-next clk/clk-next kvmarm/next linus/master keystone/next v5.15-rc7 next-20211029]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/quanyang-wang-windriver-com/ARM-add-BUILD_BUG_ON-to-check-if-fixmap-range-spans-multiple-pmds/20211020-135145
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git for-next
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/54649c5c19414a00a548a13dd596037c5e50a241
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review quanyang-wang-windriver-com/ARM-add-BUILD_BUG_ON-to-check-if-fixmap-range-spans-multiple-pmds/20211020-135145
        git checkout 54649c5c19414a00a548a13dd596037c5e50a241
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/arm/mm/mmu.c:118:13: warning: no previous prototype for 'init_default_cache_policy' [-Wmissing-prototypes]
     118 | void __init init_default_cache_policy(unsigned long pmd)
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~
   arch/arm/mm/mmu.c:1158:13: warning: no previous prototype for 'adjust_lowmem_bounds' [-Wmissing-prototypes]
    1158 | void __init adjust_lowmem_bounds(void)
         |             ^~~~~~~~~~~~~~~~~~~~
   arch/arm/mm/mmu.c:1724:13: warning: no previous prototype for 'paging_init' [-Wmissing-prototypes]
    1724 | void __init paging_init(const struct machine_desc *mdesc)
         |             ^~~~~~~~~~~
   arch/arm/mm/mmu.c:1757:13: warning: no previous prototype for 'early_mm_init' [-Wmissing-prototypes]
    1757 | void __init early_mm_init(const struct machine_desc *mdesc)
         |             ^~~~~~~~~~~~~
   In file included from <command-line>:
   arch/arm/mm/mmu.c: In function 'early_fixmap_init':
>> include/linux/compiler_types.h:322:45: error: call to '__compiletime_assert_295' declared with attribute error: BUILD_BUG_ON failed: (__fix_to_virt(__end_of_fixmap_region) >> PMD_SHIFT) != FIXADDR_TOP >> PMD_SHIFT
     322 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |                                             ^
   include/linux/compiler_types.h:303:25: note: in definition of macro '__compiletime_assert'
     303 |                         prefix ## suffix();                             \
         |                         ^~~~~~
   include/linux/compiler_types.h:322:9: note: in expansion of macro '_compiletime_assert'
     322 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |         ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
      39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
         |                                     ^~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:50:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
      50 |         BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
         |         ^~~~~~~~~~~~~~~~
   arch/arm/mm/mmu.c:372:9: note: in expansion of macro 'BUILD_BUG_ON'
     372 |         BUILD_BUG_ON((__fix_to_virt(__end_of_fixmap_region) >> PMD_SHIFT)
         |         ^~~~~~~~~~~~


vim +/__compiletime_assert_295 +322 include/linux/compiler_types.h

eb5c2d4b45e3d2 Will Deacon 2020-07-21  308  
eb5c2d4b45e3d2 Will Deacon 2020-07-21  309  #define _compiletime_assert(condition, msg, prefix, suffix) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21  310  	__compiletime_assert(condition, msg, prefix, suffix)
eb5c2d4b45e3d2 Will Deacon 2020-07-21  311  
eb5c2d4b45e3d2 Will Deacon 2020-07-21  312  /**
eb5c2d4b45e3d2 Will Deacon 2020-07-21  313   * compiletime_assert - break build and emit msg if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21  314   * @condition: a compile-time constant condition to check
eb5c2d4b45e3d2 Will Deacon 2020-07-21  315   * @msg:       a message to emit if condition is false
eb5c2d4b45e3d2 Will Deacon 2020-07-21  316   *
eb5c2d4b45e3d2 Will Deacon 2020-07-21  317   * In tradition of POSIX assert, this macro will break the build if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21  318   * supplied condition is *false*, emitting the supplied error message if the
eb5c2d4b45e3d2 Will Deacon 2020-07-21  319   * compiler has support to do so.
eb5c2d4b45e3d2 Will Deacon 2020-07-21  320   */
eb5c2d4b45e3d2 Will Deacon 2020-07-21  321  #define compiletime_assert(condition, msg) \
eb5c2d4b45e3d2 Will Deacon 2020-07-21 @322  	_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
eb5c2d4b45e3d2 Will Deacon 2020-07-21  323  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 55403 bytes --]