[PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Kefeng Wang]

This reverts commit 602946ec2f90d5bd965857753880db29d2d9a1e9.

If CONFIG_HIGHMEM enabled, highmem will be disappeared with max_mapnr
set to max_low_pfn, see mem_init(), 

  for (pfn = highmem_mapnr; pfn < max_mapnr; ++pfn) {
        ...
	free_highmem_page();
  }

Revert it and will fix virt_addr_valid() check in the next patch.

Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Fixes: 602946ec2f90 ("powerpc: Set max_mapnr correctly")
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
v4:
- new patch
 arch/powerpc/mm/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 8e301cd8925b..4d221d033804 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -255,7 +255,7 @@ void __init mem_init(void)
 #endif
 
 	high_memory = (void *) __va(max_low_pfn * PAGE_SIZE);
-	set_max_mapnr(max_low_pfn);
+	set_max_mapnr(max_pfn);
 
 	kasan_late_init();
 
-- 
2.26.2

[PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Kefeng Wang]

This reverts commit 602946ec2f90d5bd965857753880db29d2d9a1e9.

If CONFIG_HIGHMEM enabled, highmem will be disappeared with max_mapnr
set to max_low_pfn, see mem_init(), 

  for (pfn = highmem_mapnr; pfn < max_mapnr; ++pfn) {
        ...
	free_highmem_page();
  }

Revert it and will fix virt_addr_valid() check in the next patch.

Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
Fixes: 602946ec2f90 ("powerpc: Set max_mapnr correctly")
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
v4:
- new patch
 arch/powerpc/mm/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 8e301cd8925b..4d221d033804 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -255,7 +255,7 @@ void __init mem_init(void)
 #endif
 
 	high_memory = (void *) __va(max_low_pfn * PAGE_SIZE);
-	set_max_mapnr(max_low_pfn);
+	set_max_mapnr(max_pfn);
 
 	kasan_late_init();
 
-- 
2.26.2

[PATCH v4 2/2] powerpc: Fix virt_addr_valid() check

[Kefeng Wang]

When run ethtool eth0 on PowerPC64, the BUG occurred,

  usercopy: Kernel memory exposure attempt detected from SLUB object not in SLUB page?! (offset 0, size 1048)!
  kernel BUG at mm/usercopy.c:99
  ...
  usercopy_abort+0x64/0xa0 (unreliable)
  __check_heap_object+0x168/0x190
  __check_object_size+0x1a0/0x200
  dev_ethtool+0x2494/0x2b20
  dev_ioctl+0x5d0/0x770
  sock_do_ioctl+0xf0/0x1d0
  sock_ioctl+0x3ec/0x5a0
  __se_sys_ioctl+0xf0/0x160
  system_call_exception+0xfc/0x1f0
  system_call_common+0xf8/0x200

The code shows below,

  data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN));
  copy_to_user(useraddr, data, gstrings.len * ETH_GSTRING_LEN))

The data is alloced by vmalloc(), virt_addr_valid(ptr) will return true
on PowerPC64, which leads to the panic.

As commit 4dd7554a6456 ("powerpc/64: Add VIRTUAL_BUG_ON checks for __va
and __pa addresses") does, make sure the virt addr above PAGE_OFFSET in
the virt_addr_valid() for PowerPC64, also add upper limit check to make
sure the virt is below high_memory.

Meanwhile, for PowerPC32 PAGE_OFFSET is the virtual address of the start
of lowmem, high_memory is the upper low virtual address, the check is
suitable for PowerPC32, this will fix the issue mentioned in commit
602946ec2f90 ("powerpc: Set max_mapnr correctly") too.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
v4:
- add upper limit check
v3:
- update changelog and remove a redundant cast
 arch/powerpc/include/asm/page.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index 254687258f42..7a1ba27a7285 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -132,7 +132,11 @@ static inline bool pfn_valid(unsigned long pfn)
 #define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
 #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
 
-#define virt_addr_valid(kaddr)	pfn_valid(virt_to_pfn(kaddr))
+#define virt_addr_valid(vaddr)	({					\
+	unsigned long _addr = (unsigned long)vaddr;			\
+	_addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory &&	\
+	pfn_valid(virt_to_pfn(_addr));					\
+})
 
 /*
  * On Book-E parts we need __va to parse the device tree and we can't
-- 
2.26.2

[PATCH v4 2/2] powerpc: Fix virt_addr_valid() check

[Kefeng Wang]

When run ethtool eth0 on PowerPC64, the BUG occurred,

  usercopy: Kernel memory exposure attempt detected from SLUB object not in SLUB page?! (offset 0, size 1048)!
  kernel BUG at mm/usercopy.c:99
  ...
  usercopy_abort+0x64/0xa0 (unreliable)
  __check_heap_object+0x168/0x190
  __check_object_size+0x1a0/0x200
  dev_ethtool+0x2494/0x2b20
  dev_ioctl+0x5d0/0x770
  sock_do_ioctl+0xf0/0x1d0
  sock_ioctl+0x3ec/0x5a0
  __se_sys_ioctl+0xf0/0x160
  system_call_exception+0xfc/0x1f0
  system_call_common+0xf8/0x200

The code shows below,

  data = vzalloc(array_size(gstrings.len, ETH_GSTRING_LEN));
  copy_to_user(useraddr, data, gstrings.len * ETH_GSTRING_LEN))

The data is alloced by vmalloc(), virt_addr_valid(ptr) will return true
on PowerPC64, which leads to the panic.

As commit 4dd7554a6456 ("powerpc/64: Add VIRTUAL_BUG_ON checks for __va
and __pa addresses") does, make sure the virt addr above PAGE_OFFSET in
the virt_addr_valid() for PowerPC64, also add upper limit check to make
sure the virt is below high_memory.

Meanwhile, for PowerPC32 PAGE_OFFSET is the virtual address of the start
of lowmem, high_memory is the upper low virtual address, the check is
suitable for PowerPC32, this will fix the issue mentioned in commit
602946ec2f90 ("powerpc: Set max_mapnr correctly") too.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
v4:
- add upper limit check
v3:
- update changelog and remove a redundant cast
 arch/powerpc/include/asm/page.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h
index 254687258f42..7a1ba27a7285 100644
--- a/arch/powerpc/include/asm/page.h
+++ b/arch/powerpc/include/asm/page.h
@@ -132,7 +132,11 @@ static inline bool pfn_valid(unsigned long pfn)
 #define virt_to_page(kaddr)	pfn_to_page(virt_to_pfn(kaddr))
 #define pfn_to_kaddr(pfn)	__va((pfn) << PAGE_SHIFT)
 
-#define virt_addr_valid(kaddr)	pfn_valid(virt_to_pfn(kaddr))
+#define virt_addr_valid(vaddr)	({					\
+	unsigned long _addr = (unsigned long)vaddr;			\
+	_addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory &&	\
+	pfn_valid(virt_to_pfn(_addr));					\
+})
 
 /*
  * On Book-E parts we need __va to parse the device tree and we can't
-- 
2.26.2

Re: [v4,1/2] Revert "powerpc: Set max_mapnr correctly"

[Christophe Leroy]

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Re: [v4,2/2] powerpc: Fix virt_addr_valid() check

[Christophe Leroy]

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Kefeng Wang]

Hi maintainers，

I saw the patches has been reviewed[1], could they be merged?

Many thanks.

[1] https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=286464

On 2022/2/16 20:11, Kefeng Wang wrote:
> This reverts commit 602946ec2f90d5bd965857753880db29d2d9a1e9.
>
> If CONFIG_HIGHMEM enabled, highmem will be disappeared with max_mapnr
> set to max_low_pfn, see mem_init(),
>
>    for (pfn = highmem_mapnr; pfn < max_mapnr; ++pfn) {
>          ...
> 	free_highmem_page();
>    }
>
> Revert it and will fix virt_addr_valid() check in the next patch.
>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Fixes: 602946ec2f90 ("powerpc: Set max_mapnr correctly")
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> v4:
> - new patch
>   arch/powerpc/mm/mem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 8e301cd8925b..4d221d033804 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -255,7 +255,7 @@ void __init mem_init(void)
>   #endif
>   
>   	high_memory = (void *) __va(max_low_pfn * PAGE_SIZE);
> -	set_max_mapnr(max_low_pfn);
> +	set_max_mapnr(max_pfn);
>   
>   	kasan_late_init();
>   

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Kefeng Wang]

Hi maintainers，

I saw the patches has been reviewed[1], could they be merged?

Many thanks.

[1] https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=286464

On 2022/2/16 20:11, Kefeng Wang wrote:
> This reverts commit 602946ec2f90d5bd965857753880db29d2d9a1e9.
>
> If CONFIG_HIGHMEM enabled, highmem will be disappeared with max_mapnr
> set to max_low_pfn, see mem_init(),
>
>    for (pfn = highmem_mapnr; pfn < max_mapnr; ++pfn) {
>          ...
> 	free_highmem_page();
>    }
>
> Revert it and will fix virt_addr_valid() check in the next patch.
>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Fixes: 602946ec2f90 ("powerpc: Set max_mapnr correctly")
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> v4:
> - new patch
>   arch/powerpc/mm/mem.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 8e301cd8925b..4d221d033804 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -255,7 +255,7 @@ void __init mem_init(void)
>   #endif
>   
>   	high_memory = (void *) __va(max_low_pfn * PAGE_SIZE);
> -	set_max_mapnr(max_low_pfn);
> +	set_max_mapnr(max_pfn);
>   
>   	kasan_late_init();
>   

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Michael Ellerman]

Kefeng Wang <wangkefeng.wang@huawei.com> writes:
> Hi maintainers，
>
> I saw the patches has been reviewed[1], could they be merged?

Maybe I'm just misreading the change log, but it seems wrong that we
need to add extra checks. pfn_valid() shouldn't return true for vmalloc
addresses in the first place, shouldn't we fix that instead? Who knows
what else that might be broken because of that.

cheers

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Michael Ellerman]

Kefeng Wang <wangkefeng.wang@huawei.com> writes:
> Hi maintainers，
>
> I saw the patches has been reviewed[1], could they be merged?

Maybe I'm just misreading the change log, but it seems wrong that we
need to add extra checks. pfn_valid() shouldn't return true for vmalloc
addresses in the first place, shouldn't we fix that instead? Who knows
what else that might be broken because of that.

cheers

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Christophe Leroy]

Le 28/03/2022 à 12:37, Michael Ellerman a écrit :
> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>> Hi maintainers，
>>
>> I saw the patches has been reviewed[1], could they be merged?
> 
> Maybe I'm just misreading the change log, but it seems wrong that we
> need to add extra checks. pfn_valid() shouldn't return true for vmalloc
> addresses in the first place, shouldn't we fix that instead? Who knows
> what else that might be broken because of that.
> 

pfn_valid() doesn't take an address but a PFN

If you have 1Gbyte of memory you have 256k PFNs.

In a generic config the kernel will map 768 Mbytes of mémory (From 
0xc0000000 to 0xe0000000) and will use 0xf0000000-0xffffffff for 
everything else including vmalloc.

If you take a page above that 768 Mbytes limit, and tries to linarly 
convert it's PFN to a va, you'll hip vmalloc space. Anyway that PFN is 
valid.

So the check really needs to be done in virt_addr_valid().

There is another thing however that would be worth fixing (in another 
patch): that's the virt_to_pfn() in PPC64:

#define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)

#define __pa(x)								\
({									\
	VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET);		\
	(unsigned long)(x) & 0x0fffffffffffffffUL;			\
})


So 0xc000000000000000 and 0xd000000000000000 have the same PFN. That's 
wrong.

Christophe

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Christophe Leroy]

Le 28/03/2022 à 12:37, Michael Ellerman a écrit :
> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>> Hi maintainers，
>>
>> I saw the patches has been reviewed[1], could they be merged?
> 
> Maybe I'm just misreading the change log, but it seems wrong that we
> need to add extra checks. pfn_valid() shouldn't return true for vmalloc
> addresses in the first place, shouldn't we fix that instead? Who knows
> what else that might be broken because of that.
> 

pfn_valid() doesn't take an address but a PFN

If you have 1Gbyte of memory you have 256k PFNs.

In a generic config the kernel will map 768 Mbytes of mémory (From 
0xc0000000 to 0xe0000000) and will use 0xf0000000-0xffffffff for 
everything else including vmalloc.

If you take a page above that 768 Mbytes limit, and tries to linarly 
convert it's PFN to a va, you'll hip vmalloc space. Anyway that PFN is 
valid.

So the check really needs to be done in virt_addr_valid().

There is another thing however that would be worth fixing (in another 
patch): that's the virt_to_pfn() in PPC64:

#define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)

#define __pa(x)								\
({									\
	VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET);		\
	(unsigned long)(x) & 0x0fffffffffffffffUL;			\
})


So 0xc000000000000000 and 0xd000000000000000 have the same PFN. That's 
wrong.

Christophe

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Christophe Leroy]

Hi,

Le 26/03/2022 à 08:55, Kefeng Wang a écrit :
> Hi maintainers，
> 
> I saw the patches has been reviewed[1], could they be merged?

Thinking about it once more, I think the patches should go in reverse 
order. Patch 2 should go first and patch 1 should go after.

Otherwise, once patch 1 is applied and patch 2 is not applied yet, 
virt_addr_valid() doesn't work anymore.

Christophe

> 
> Many thanks.
> 
> [1] https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=286464
> 
> On 2022/2/16 20:11, Kefeng Wang wrote:
>> This reverts commit 602946ec2f90d5bd965857753880db29d2d9a1e9.
>>
>> If CONFIG_HIGHMEM enabled, highmem will be disappeared with max_mapnr
>> set to max_low_pfn, see mem_init(),
>>
>>    for (pfn = highmem_mapnr; pfn < max_mapnr; ++pfn) {
>>          ...
>>     free_highmem_page();
>>    }
>>
>> Revert it and will fix virt_addr_valid() check in the next patch.
>>
>> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Fixes: 602946ec2f90 ("powerpc: Set max_mapnr correctly")
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>> v4:
>> - new patch
>>   arch/powerpc/mm/mem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
>> index 8e301cd8925b..4d221d033804 100644
>> --- a/arch/powerpc/mm/mem.c
>> +++ b/arch/powerpc/mm/mem.c
>> @@ -255,7 +255,7 @@ void __init mem_init(void)
>>   #endif
>>       high_memory = (void *) __va(max_low_pfn * PAGE_SIZE);
>> -    set_max_mapnr(max_low_pfn);
>> +    set_max_mapnr(max_pfn);
>>       kasan_late_init();

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Christophe Leroy]

Hi,

Le 26/03/2022 à 08:55, Kefeng Wang a écrit :
> Hi maintainers，
> 
> I saw the patches has been reviewed[1], could they be merged?

Thinking about it once more, I think the patches should go in reverse 
order. Patch 2 should go first and patch 1 should go after.

Otherwise, once patch 1 is applied and patch 2 is not applied yet, 
virt_addr_valid() doesn't work anymore.

Christophe

> 
> Many thanks.
> 
> [1] https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=286464
> 
> On 2022/2/16 20:11, Kefeng Wang wrote:
>> This reverts commit 602946ec2f90d5bd965857753880db29d2d9a1e9.
>>
>> If CONFIG_HIGHMEM enabled, highmem will be disappeared with max_mapnr
>> set to max_low_pfn, see mem_init(),
>>
>>    for (pfn = highmem_mapnr; pfn < max_mapnr; ++pfn) {
>>          ...
>>     free_highmem_page();
>>    }
>>
>> Revert it and will fix virt_addr_valid() check in the next patch.
>>
>> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Fixes: 602946ec2f90 ("powerpc: Set max_mapnr correctly")
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>> v4:
>> - new patch
>>   arch/powerpc/mm/mem.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
>> index 8e301cd8925b..4d221d033804 100644
>> --- a/arch/powerpc/mm/mem.c
>> +++ b/arch/powerpc/mm/mem.c
>> @@ -255,7 +255,7 @@ void __init mem_init(void)
>>   #endif
>>       high_memory = (void *) __va(max_low_pfn * PAGE_SIZE);
>> -    set_max_mapnr(max_low_pfn);
>> +    set_max_mapnr(max_pfn);
>>       kasan_late_init();

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Kefeng Wang]

On 2022/3/28 22:12, Christophe Leroy wrote:
> Hi,
>
> Le 26/03/2022 à 08:55, Kefeng Wang a écrit :
>> Hi maintainers，
>>
>> I saw the patches has been reviewed[1], could they be merged?
> Thinking about it once more, I think the patches should go in reverse
> order. Patch 2 should go first and patch 1 should go after.
>
> Otherwise, once patch 1 is applied and patch 2 is not applied yet,
> virt_addr_valid() doesn't work anymore.
Should I resend them or could the maintainer reverse order when merging 
them?
>
> Christophe
>
>> Many thanks.
>>
>> [1] https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=286464
>>

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Kefeng Wang]

On 2022/3/28 22:12, Christophe Leroy wrote:
> Hi,
>
> Le 26/03/2022 à 08:55, Kefeng Wang a écrit :
>> Hi maintainers，
>>
>> I saw the patches has been reviewed[1], could they be merged?
> Thinking about it once more, I think the patches should go in reverse
> order. Patch 2 should go first and patch 1 should go after.
>
> Otherwise, once patch 1 is applied and patch 2 is not applied yet,
> virt_addr_valid() doesn't work anymore.
Should I resend them or could the maintainer reverse order when merging 
them?
>
> Christophe
>
>> Many thanks.
>>
>> [1] https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=286464
>>

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 28/03/2022 à 12:37, Michael Ellerman a écrit :
>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>> Hi maintainers，
>>>
>>> I saw the patches has been reviewed[1], could they be merged?
>>
>> Maybe I'm just misreading the change log, but it seems wrong that we
>> need to add extra checks. pfn_valid() shouldn't return true for vmalloc
>> addresses in the first place, shouldn't we fix that instead? Who knows
>> what else that might be broken because of that.
>
> pfn_valid() doesn't take an address but a PFN

Yeah sorry that was unclear wording on my part.

What I mean is that pfn_valid(virt_to_pfn(some_vmalloc_addr)) should be
false, because virt_to_pfn(vmalloc_addr) should fail.

The right way to convert a vmalloc address to a pfn is with
vmalloc_to_pfn(), which walks the page tables to find the actual pfn
backing that vmalloc addr.

> If you have 1Gbyte of memory you have 256k PFNs.
>
> In a generic config the kernel will map 768 Mbytes of mémory (From
> 0xc0000000 to 0xe0000000) and will use 0xf0000000-0xffffffff for
> everything else including vmalloc.
>
> If you take a page above that 768 Mbytes limit, and tries to linarly
> convert it's PFN to a va, you'll hip vmalloc space. Anyway that PFN is
> valid.

That's true, but it's just some random page in vmalloc space, there's no
guarantee that it's the same page as the PFN you started with.

Note it's not true on 64-bit Book3S. There if you take a valid PFN (ie.
backed by RAM) and convert it to a virtual address (with __va()), you
will never get a vmalloc address.

> So the check really needs to be done in virt_addr_valid().

I don't think it has to, but with the way our virt_to_pfn()/__pa() works
I guess for now it's the easiest solution.

> There is another thing however that would be worth fixing (in another
> patch): that's the virt_to_pfn() in PPC64:
>
> #define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)
>
> #define __pa(x)								\
> ({									\
> 	VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET);		\
> 	(unsigned long)(x) & 0x0fffffffffffffffUL;			\
> })
>
>
> So 0xc000000000000000 and 0xd000000000000000 have the same PFN. That's
> wrong.

Yes it was wrong. But we don't use 0xd000000000000000 anymore, so it
shouldn't be an issue in practice.

See 0034d395f89d ("powerpc/mm/hash64: Map all the kernel regions in the same 0xc range").

I guess it is still a problem for 64-bit Book3E, because they use 0xc
and 0x8.

I looked at fixing __pa()/__va() to use +/- a few years back, but from
memory it still didn't work and/or generated bad code. There's a comment
about it working around a GCC miscompile.

The other thing that makes me reluctant to change it is that I worry we
have places that inadvertantly use __pa() on addresses that are already
physical addresses. If we changed __pa() to use subtraction that would
break, ie. we'd end up with a negative address.

See eg. a6e2c226c3d5 ("powerpc: Fix kernel crash in show_instructions() w/DEBUG_VIRTUAL")

So to fix it we'd have to 1) verify that the compiler bug is no longer
an issue and 2) audit uses of __pa()/__va().

cheers

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 28/03/2022 à 12:37, Michael Ellerman a écrit :
>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>> Hi maintainers，
>>>
>>> I saw the patches has been reviewed[1], could they be merged?
>>
>> Maybe I'm just misreading the change log, but it seems wrong that we
>> need to add extra checks. pfn_valid() shouldn't return true for vmalloc
>> addresses in the first place, shouldn't we fix that instead? Who knows
>> what else that might be broken because of that.
>
> pfn_valid() doesn't take an address but a PFN

Yeah sorry that was unclear wording on my part.

What I mean is that pfn_valid(virt_to_pfn(some_vmalloc_addr)) should be
false, because virt_to_pfn(vmalloc_addr) should fail.

The right way to convert a vmalloc address to a pfn is with
vmalloc_to_pfn(), which walks the page tables to find the actual pfn
backing that vmalloc addr.

> If you have 1Gbyte of memory you have 256k PFNs.
>
> In a generic config the kernel will map 768 Mbytes of mémory (From
> 0xc0000000 to 0xe0000000) and will use 0xf0000000-0xffffffff for
> everything else including vmalloc.
>
> If you take a page above that 768 Mbytes limit, and tries to linarly
> convert it's PFN to a va, you'll hip vmalloc space. Anyway that PFN is
> valid.

That's true, but it's just some random page in vmalloc space, there's no
guarantee that it's the same page as the PFN you started with.

Note it's not true on 64-bit Book3S. There if you take a valid PFN (ie.
backed by RAM) and convert it to a virtual address (with __va()), you
will never get a vmalloc address.

> So the check really needs to be done in virt_addr_valid().

I don't think it has to, but with the way our virt_to_pfn()/__pa() works
I guess for now it's the easiest solution.

> There is another thing however that would be worth fixing (in another
> patch): that's the virt_to_pfn() in PPC64:
>
> #define virt_to_pfn(kaddr)	(__pa(kaddr) >> PAGE_SHIFT)
>
> #define __pa(x)								\
> ({									\
> 	VIRTUAL_BUG_ON((unsigned long)(x) < PAGE_OFFSET);		\
> 	(unsigned long)(x) & 0x0fffffffffffffffUL;			\
> })
>
>
> So 0xc000000000000000 and 0xd000000000000000 have the same PFN. That's
> wrong.

Yes it was wrong. But we don't use 0xd000000000000000 anymore, so it
shouldn't be an issue in practice.

See 0034d395f89d ("powerpc/mm/hash64: Map all the kernel regions in the same 0xc range").

I guess it is still a problem for 64-bit Book3E, because they use 0xc
and 0x8.

I looked at fixing __pa()/__va() to use +/- a few years back, but from
memory it still didn't work and/or generated bad code. There's a comment
about it working around a GCC miscompile.

The other thing that makes me reluctant to change it is that I worry we
have places that inadvertantly use __pa() on addresses that are already
physical addresses. If we changed __pa() to use subtraction that would
break, ie. we'd end up with a negative address.

See eg. a6e2c226c3d5 ("powerpc: Fix kernel crash in show_instructions() w/DEBUG_VIRTUAL")

So to fix it we'd have to 1) verify that the compiler bug is no longer
an issue and 2) audit uses of __pa()/__va().

cheers

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Christophe Leroy]

Le 01/04/2022 à 13:23, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 28/03/2022 à 12:37, Michael Ellerman a écrit :
>>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>>> Hi maintainers，
>>>>
>>>> I saw the patches has been reviewed[1], could they be merged?
>>>
>>> Maybe I'm just misreading the change log, but it seems wrong that we
>>> need to add extra checks. pfn_valid() shouldn't return true for vmalloc
>>> addresses in the first place, shouldn't we fix that instead? Who knows
>>> what else that might be broken because of that.
>>
>> pfn_valid() doesn't take an address but a PFN
> 
> Yeah sorry that was unclear wording on my part.
> 
> What I mean is that pfn_valid(virt_to_pfn(some_vmalloc_addr)) should be
> false, because virt_to_pfn(vmalloc_addr) should fail.

Yes that's probably how it should be but none of the main architectures 
do that.

The best we find is some architecture that WARN_ON(some valloc addr) in 
virt_to_pfn(). That's wouldn't help in our case, as it would then WARN_ON()


> 
> The right way to convert a vmalloc address to a pfn is with
> vmalloc_to_pfn(), which walks the page tables to find the actual pfn
> backing that vmalloc addr.
> 
>> If you have 1Gbyte of memory you have 256k PFNs.
>>
>> In a generic config the kernel will map 768 Mbytes of mémory (From
>> 0xc0000000 to 0xe0000000) and will use 0xf0000000-0xffffffff for
>> everything else including vmalloc.
>>
>> If you take a page above that 768 Mbytes limit, and tries to linarly
>> convert it's PFN to a va, you'll hip vmalloc space. Anyway that PFN is
>> valid.
> 
> That's true, but it's just some random page in vmalloc space, there's no
> guarantee that it's the same page as the PFN you started with.

Yes sure, what I meant however is that pfn_valid(some_valid_pfn) should 
return true, even if virt_to_pfn(some_vmalloc_address) profides a valid PFN.

> 
> Note it's not true on 64-bit Book3S. There if you take a valid PFN (ie.
> backed by RAM) and convert it to a virtual address (with __va()), you
> will never get a vmalloc address.
> 
>> So the check really needs to be done in virt_addr_valid().
> 
> I don't think it has to, but with the way our virt_to_pfn()/__pa() works
> I guess for now it's the easiest solution.
> 

At least other architectures do it that way. See for instance how ARM 
does it:

#define virt_addr_valid(kaddr)	(((unsigned long)(kaddr) >= PAGE_OFFSET 
&& (unsigned long)(kaddr) < (unsigned long)high_memory) \
					&& pfn_valid(virt_to_pfn(kaddr)))



high_memory being the top of linear RAM mapping

Christophe

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Christophe Leroy]

Le 01/04/2022 à 13:23, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 28/03/2022 à 12:37, Michael Ellerman a écrit :
>>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>>> Hi maintainers，
>>>>
>>>> I saw the patches has been reviewed[1], could they be merged?
>>>
>>> Maybe I'm just misreading the change log, but it seems wrong that we
>>> need to add extra checks. pfn_valid() shouldn't return true for vmalloc
>>> addresses in the first place, shouldn't we fix that instead? Who knows
>>> what else that might be broken because of that.
>>
>> pfn_valid() doesn't take an address but a PFN
> 
> Yeah sorry that was unclear wording on my part.
> 
> What I mean is that pfn_valid(virt_to_pfn(some_vmalloc_addr)) should be
> false, because virt_to_pfn(vmalloc_addr) should fail.

Yes that's probably how it should be but none of the main architectures 
do that.

The best we find is some architecture that WARN_ON(some valloc addr) in 
virt_to_pfn(). That's wouldn't help in our case, as it would then WARN_ON()


> 
> The right way to convert a vmalloc address to a pfn is with
> vmalloc_to_pfn(), which walks the page tables to find the actual pfn
> backing that vmalloc addr.
> 
>> If you have 1Gbyte of memory you have 256k PFNs.
>>
>> In a generic config the kernel will map 768 Mbytes of mémory (From
>> 0xc0000000 to 0xe0000000) and will use 0xf0000000-0xffffffff for
>> everything else including vmalloc.
>>
>> If you take a page above that 768 Mbytes limit, and tries to linarly
>> convert it's PFN to a va, you'll hip vmalloc space. Anyway that PFN is
>> valid.
> 
> That's true, but it's just some random page in vmalloc space, there's no
> guarantee that it's the same page as the PFN you started with.

Yes sure, what I meant however is that pfn_valid(some_valid_pfn) should 
return true, even if virt_to_pfn(some_vmalloc_address) profides a valid PFN.

> 
> Note it's not true on 64-bit Book3S. There if you take a valid PFN (ie.
> backed by RAM) and convert it to a virtual address (with __va()), you
> will never get a vmalloc address.
> 
>> So the check really needs to be done in virt_addr_valid().
> 
> I don't think it has to, but with the way our virt_to_pfn()/__pa() works
> I guess for now it's the easiest solution.
> 

At least other architectures do it that way. See for instance how ARM 
does it:

#define virt_addr_valid(kaddr)	(((unsigned long)(kaddr) >= PAGE_OFFSET 
&& (unsigned long)(kaddr) < (unsigned long)high_memory) \
					&& pfn_valid(virt_to_pfn(kaddr)))



high_memory being the top of linear RAM mapping

Christophe

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Michael Ellerman]

Kefeng Wang <wangkefeng.wang@huawei.com> writes:
> On 2022/3/28 22:12, Christophe Leroy wrote:
>> Hi,
>>
>> Le 26/03/2022 à 08:55, Kefeng Wang a écrit :
>>> Hi maintainers，
>>>
>>> I saw the patches has been reviewed[1], could they be merged?
>> Thinking about it once more, I think the patches should go in reverse
>> order. Patch 2 should go first and patch 1 should go after.
>>
>> Otherwise, once patch 1 is applied and patch 2 is not applied yet,
>> virt_addr_valid() doesn't work anymore.
>
> Should I resend them or could the maintainer reverse order when merging 
> them?

I'll reverse them. I've found some breakage in other code while testing
this, so I'll fix that up first before merging these.

In patch 2 you didn't say what hardware you hit this on, what CPU does
your system have?

cheers

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Michael Ellerman]

Kefeng Wang <wangkefeng.wang@huawei.com> writes:
> On 2022/3/28 22:12, Christophe Leroy wrote:
>> Hi,
>>
>> Le 26/03/2022 à 08:55, Kefeng Wang a écrit :
>>> Hi maintainers，
>>>
>>> I saw the patches has been reviewed[1], could they be merged?
>> Thinking about it once more, I think the patches should go in reverse
>> order. Patch 2 should go first and patch 1 should go after.
>>
>> Otherwise, once patch 1 is applied and patch 2 is not applied yet,
>> virt_addr_valid() doesn't work anymore.
>
> Should I resend them or could the maintainer reverse order when merging 
> them?

I'll reverse them. I've found some breakage in other code while testing
this, so I'll fix that up first before merging these.

In patch 2 you didn't say what hardware you hit this on, what CPU does
your system have?

cheers

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Kefeng Wang]

On 2022/4/4 20:31, Michael Ellerman wrote:
> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>> On 2022/3/28 22:12, Christophe Leroy wrote:
>>> Hi,
>>>
>>> Le 26/03/2022 à 08:55, Kefeng Wang a écrit :
>>>> Hi maintainers，
>>>>
>>>> I saw the patches has been reviewed[1], could they be merged?
>>> Thinking about it once more, I think the patches should go in reverse
>>> order. Patch 2 should go first and patch 1 should go after.
>>>
>>> Otherwise, once patch 1 is applied and patch 2 is not applied yet,
>>> virt_addr_valid() doesn't work anymore.
>> Should I resend them or could the maintainer reverse order when merging
>> them?
> I'll reverse them. I've found some breakage in other code while testing
> this, so I'll fix that up first before merging these.
Thanks.
>
> In patch 2 you didn't say what hardware you hit this on, what CPU does
> your system have?

CPU e5500  from fsl,P5040DS.

>
> cheers
> .

Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Kefeng Wang]

On 2022/4/4 20:31, Michael Ellerman wrote:
> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>> On 2022/3/28 22:12, Christophe Leroy wrote:
>>> Hi,
>>>
>>> Le 26/03/2022 à 08:55, Kefeng Wang a écrit :
>>>> Hi maintainers，
>>>>
>>>> I saw the patches has been reviewed[1], could they be merged?
>>> Thinking about it once more, I think the patches should go in reverse
>>> order. Patch 2 should go first and patch 1 should go after.
>>>
>>> Otherwise, once patch 1 is applied and patch 2 is not applied yet,
>>> virt_addr_valid() doesn't work anymore.
>> Should I resend them or could the maintainer reverse order when merging
>> them?
> I'll reverse them. I've found some breakage in other code while testing
> this, so I'll fix that up first before merging these.
Thanks.
>
> In patch 2 you didn't say what hardware you hit this on, what CPU does
> your system have?

CPU e5500  from fsl,P5040DS.

>
> cheers
> .