[PATCH v3] arm64: Restrict pagetable teardown to avoid false warning

[PATCH v3] arm64: Restrict pagetable teardown to avoid false warning

[Dev Jain]

Commit 9c006972c3fe removes the pxd_present() checks because the caller
checks pxd_present(). But, in case of vmap_try_huge_pud(), the caller only
checks pud_present(); pud_free_pmd_page() recurses on each pmd through
pmd_free_pte_page(), wherein the pmd may be none. Thus it is possible to
hit a warning in the latter, since pmd_none => !pmd_table(). Thus, add
a pmd_present() check in pud_free_pmd_page().

This problem was found by code inspection.

Fixes: 9c006972c3fe (arm64: mmu: drop pXd_present() checks from pXd_free_pYd_table())
Cc: <stable@vger.kernel.org>
Reported-by: Ryan Roberts <ryan.roberts@arm.com> 
Acked-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Dev Jain <dev.jain@arm.com>
---
This patch is based on 6.15-rc6.

v2->v3:
 - Use pmdp_get()

v1->v2:
 - Enforce check in caller

 arch/arm64/mm/mmu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index ea6695d53fb9..5a9bf291c649 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1286,7 +1286,8 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
 	next = addr;
 	end = addr + PUD_SIZE;
 	do {
-		pmd_free_pte_page(pmdp, next);
+		if (pmd_present(pmdp_get(pmdp)))
+			pmd_free_pte_page(pmdp, next);
 	} while (pmdp++, next += PMD_SIZE, next != end);
 
 	pud_clear(pudp);
-- 
2.30.2

Re: [PATCH v3] arm64: Restrict pagetable teardown to avoid false warning

[Anshuman Khandual]

On 5/27/25 13:56, Dev Jain wrote:
> Commit 9c006972c3fe removes the pxd_present() checks because the caller
> checks pxd_present(). But, in case of vmap_try_huge_pud(), the caller only
> checks pud_present(); pud_free_pmd_page() recurses on each pmd through
> pmd_free_pte_page(), wherein the pmd may be none. Thus it is possible to
> hit a warning in the latter, since pmd_none => !pmd_table(). Thus, add
> a pmd_present() check in pud_free_pmd_page().
> 
> This problem was found by code inspection.
> 
> Fixes: 9c006972c3fe (arm64: mmu: drop pXd_present() checks from pXd_free_pYd_table())
> Cc: <stable@vger.kernel.org>
> Reported-by: Ryan Roberts <ryan.roberts@arm.com> 
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> ---
> This patch is based on 6.15-rc6.
> 
> v2->v3:
>  - Use pmdp_get()
> 
> v1->v2:
>  - Enforce check in caller
> 
>  arch/arm64/mm/mmu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index ea6695d53fb9..5a9bf291c649 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1286,7 +1286,8 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>  	next = addr;
>  	end = addr + PUD_SIZE;
>  	do {
> -		pmd_free_pte_page(pmdp, next);
> +		if (pmd_present(pmdp_get(pmdp)))

This code path is only called for the kernel mapping. Hence should
pmd_valid() be used instead of pmd_present() which also checks for
present invalid scenarios as well ? 

> +			pmd_free_pte_page(pmdp, next);
>  	} while (pmdp++, next += PMD_SIZE, next != end);
>  
>  	pud_clear(pudp);

Re: [PATCH v3] arm64: Restrict pagetable teardown to avoid false warning

[Ryan Roberts]

On 29/05/2025 10:02, Anshuman Khandual wrote:
> 
> 
> On 5/27/25 13:56, Dev Jain wrote:
>> Commit 9c006972c3fe removes the pxd_present() checks because the caller
>> checks pxd_present(). But, in case of vmap_try_huge_pud(), the caller only
>> checks pud_present(); pud_free_pmd_page() recurses on each pmd through
>> pmd_free_pte_page(), wherein the pmd may be none. Thus it is possible to
>> hit a warning in the latter, since pmd_none => !pmd_table(). Thus, add
>> a pmd_present() check in pud_free_pmd_page().
>>
>> This problem was found by code inspection.
>>
>> Fixes: 9c006972c3fe (arm64: mmu: drop pXd_present() checks from pXd_free_pYd_table())
>> Cc: <stable@vger.kernel.org>
>> Reported-by: Ryan Roberts <ryan.roberts@arm.com> 
>> Acked-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Dev Jain <dev.jain@arm.com>

LGTM!

Reviewed-by: Ryan Roberts <ryan.roberts@arm.com>

>> ---
>> This patch is based on 6.15-rc6.
>>
>> v2->v3:
>>  - Use pmdp_get()
>>
>> v1->v2:
>>  - Enforce check in caller
>>
>>  arch/arm64/mm/mmu.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index ea6695d53fb9..5a9bf291c649 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1286,7 +1286,8 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>>  	next = addr;
>>  	end = addr + PUD_SIZE;
>>  	do {
>> -		pmd_free_pte_page(pmdp, next);
>> +		if (pmd_present(pmdp_get(pmdp)))
> 
> This code path is only called for the kernel mapping. Hence should
> pmd_valid() be used instead of pmd_present() which also checks for
> present invalid scenarios as well ? 

I think a similar question came up in a previous round, where we concluded that
it's better to be consistent with what vmalloc is already doing. So personally
I'd leave it as pmd_present():

	if (pmd_present(*pmd) && !pmd_free_pte_page(pmd, addr))
		return 0;

> 
>> +			pmd_free_pte_page(pmdp, next);
>>  	} while (pmdp++, next += PMD_SIZE, next != end);
>>  
>>  	pud_clear(pudp);

Re: [PATCH v3] arm64: Restrict pagetable teardown to avoid false warning

[Anshuman Khandual]

On 5/27/25 13:56, Dev Jain wrote:
> Commit 9c006972c3fe removes the pxd_present() checks because the caller
> checks pxd_present(). But, in case of vmap_try_huge_pud(), the caller only
> checks pud_present(); pud_free_pmd_page() recurses on each pmd through
> pmd_free_pte_page(), wherein the pmd may be none. Thus it is possible to
> hit a warning in the latter, since pmd_none => !pmd_table(). Thus, add
> a pmd_present() check in pud_free_pmd_page().
> 
> This problem was found by code inspection.
> 
> Fixes: 9c006972c3fe (arm64: mmu: drop pXd_present() checks from pXd_free_pYd_table())
> Cc: <stable@vger.kernel.org>
> Reported-by: Ryan Roberts <ryan.roberts@arm.com> 
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> ---
> This patch is based on 6.15-rc6.
> 
> v2->v3:
>  - Use pmdp_get()
> 
> v1->v2:
>  - Enforce check in caller
> 
>  arch/arm64/mm/mmu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index ea6695d53fb9..5a9bf291c649 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1286,7 +1286,8 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>  	next = addr;
>  	end = addr + PUD_SIZE;
>  	do {
> -		pmd_free_pte_page(pmdp, next);
> +		if (pmd_present(pmdp_get(pmdp)))
> +			pmd_free_pte_page(pmdp, next);
>  	} while (pmdp++, next += PMD_SIZE, next != end);
>  
>  	pud_clear(pudp);

Agree with Ryan about keeping pmd_present() to be consistent.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

Re: [PATCH v3] arm64: Restrict pagetable teardown to avoid false warning

[Dev Jain]

On 27/05/25 1:56 pm, Dev Jain wrote:
> Commit 9c006972c3fe removes the pxd_present() checks because the caller
> checks pxd_present(). But, in case of vmap_try_huge_pud(), the caller only
> checks pud_present(); pud_free_pmd_page() recurses on each pmd through
> pmd_free_pte_page(), wherein the pmd may be none. Thus it is possible to
> hit a warning in the latter, since pmd_none => !pmd_table(). Thus, add
> a pmd_present() check in pud_free_pmd_page().
>
> This problem was found by code inspection.
>
> Fixes: 9c006972c3fe (arm64: mmu: drop pXd_present() checks from pXd_free_pYd_table())

I missed double quotes around the fixes commit message. Can Will or Catalin fix that,
or shall I resend.

> Cc: <stable@vger.kernel.org>
> Reported-by: Ryan Roberts <ryan.roberts@arm.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> ---
> This patch is based on 6.15-rc6.
>
> v2->v3:
>   - Use pmdp_get()
>
> v1->v2:
>   - Enforce check in caller
>
>   arch/arm64/mm/mmu.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index ea6695d53fb9..5a9bf291c649 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1286,7 +1286,8 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>   	next = addr;
>   	end = addr + PUD_SIZE;
>   	do {
> -		pmd_free_pte_page(pmdp, next);
> +		if (pmd_present(pmdp_get(pmdp)))
> +			pmd_free_pte_page(pmdp, next);
>   	} while (pmdp++, next += PMD_SIZE, next != end);
>   
>   	pud_clear(pudp);

Re: [PATCH v3] arm64: Restrict pagetable teardown to avoid false warning

[Ryan Roberts]

On 30/05/2025 04:55, Dev Jain wrote:
> 
> On 27/05/25 1:56 pm, Dev Jain wrote:
>> Commit 9c006972c3fe removes the pxd_present() checks because the caller

nit: "Commit 9c006972c3fe" should have actually been:

Commit 9c006972c3fe ("arm64: mmu: drop pXd_present() checks from
pXd_free_pYd_table()")

>> checks pxd_present(). But, in case of vmap_try_huge_pud(), the caller only
>> checks pud_present(); pud_free_pmd_page() recurses on each pmd through
>> pmd_free_pte_page(), wherein the pmd may be none. Thus it is possible to
>> hit a warning in the latter, since pmd_none => !pmd_table(). Thus, add
>> a pmd_present() check in pud_free_pmd_page().
>>
>> This problem was found by code inspection.
>>
>> Fixes: 9c006972c3fe (arm64: mmu: drop pXd_present() checks from
>> pXd_free_pYd_table())
> 
> I missed double quotes around the fixes commit message. Can Will or Catalin fix
> that,
> or shall I resend.

For future, I have the following in my ~/.gitconfig

"""
[pretty]
        fixes = Fixes: %h (\"%s\")
        commit = Commit %h (\"%s\")
"""

Then I can do:

$ git show --pretty=fixes <SHA> | head -n 1

or

$ git show --pretty=commit <SHA> | head -n 1

to get the correct format. Note that "Fixes:" is a tag and should all be on a
single line. "Commit" is just a way to refer to other commits in prose and can
be broken across lines at the usual character limit.

Perhaps there is an even easier way to do it, but this works for me.

Thanks,
Ryan


> 
>> Cc: <stable@vger.kernel.org>
>> Reported-by: Ryan Roberts <ryan.roberts@arm.com>
>> Acked-by: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>> ---
>> This patch is based on 6.15-rc6.
>>
>> v2->v3:
>>   - Use pmdp_get()
>>
>> v1->v2:
>>   - Enforce check in caller
>>
>>   arch/arm64/mm/mmu.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index ea6695d53fb9..5a9bf291c649 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1286,7 +1286,8 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>>       next = addr;
>>       end = addr + PUD_SIZE;
>>       do {
>> -        pmd_free_pte_page(pmdp, next);
>> +        if (pmd_present(pmdp_get(pmdp)))
>> +            pmd_free_pte_page(pmdp, next);
>>       } while (pmdp++, next += PMD_SIZE, next != end);
>>         pud_clear(pudp);

Re: [PATCH v3] arm64: Restrict pagetable teardown to avoid false warning

[Catalin Marinas]

On Tue, May 27, 2025 at 01:56:33PM +0530, Dev Jain wrote:
> Commit 9c006972c3fe removes the pxd_present() checks because the caller
> checks pxd_present(). But, in case of vmap_try_huge_pud(), the caller only
> checks pud_present(); pud_free_pmd_page() recurses on each pmd through
> pmd_free_pte_page(), wherein the pmd may be none. Thus it is possible to
> hit a warning in the latter, since pmd_none => !pmd_table(). Thus, add
> a pmd_present() check in pud_free_pmd_page().
> 
> This problem was found by code inspection.
> 
> Fixes: 9c006972c3fe (arm64: mmu: drop pXd_present() checks from pXd_free_pYd_table())
> Cc: <stable@vger.kernel.org>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v3] arm64: Restrict pagetable teardown to avoid false warning

[Will Deacon]

On Tue, 27 May 2025 13:56:33 +0530, Dev Jain wrote:
> Commit 9c006972c3fe removes the pxd_present() checks because the caller
> checks pxd_present(). But, in case of vmap_try_huge_pud(), the caller only
> checks pud_present(); pud_free_pmd_page() recurses on each pmd through
> pmd_free_pte_page(), wherein the pmd may be none. Thus it is possible to
> hit a warning in the latter, since pmd_none => !pmd_table(). Thus, add
> a pmd_present() check in pud_free_pmd_page().
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: Restrict pagetable teardown to avoid false warning
      https://git.kernel.org/arm64/c/650768c512fa

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev