[PATCH] powerpc/64s/radix: Fix unmapping huge vmaps when CONFIG_HUGETLB_PAGE=n

[PATCH] powerpc/64s/radix: Fix unmapping huge vmaps when CONFIG_HUGETLB_PAGE=n

[Nicholas Piggin]

pmd_huge is defined out to false when HUGETLB_PAGE is not configured,
but the vmap code still installs huge PMDs. This leads to errors
encountering bad PMDs when vunmapping because it is not seen as a
huge PTE, and the bad PMD check catches it. The end result may not
be much more serious than some bad pmd warning messages, because the
pmd_none_or_clear_bad() does what we wanted and clears the huge PTE
anyway.

Fix this by checking pmd_is_leaf(), which checks for a PTE regardless
of config options. The whole huge/large/leaf stuff is a tangled mess
but that's kernel-wide and not something we can improve much in
arch/powerpc code.

Fixes: d909f9109c30 ("powerpc/64s/radix: Enable HAVE_ARCH_HUGE_VMAP")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/mm/book3s64/radix_pgtable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index 99dbee114539..7559638068ef 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -1089,7 +1089,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
 
 int pud_clear_huge(pud_t *pud)
 {
-	if (pud_huge(*pud)) {
+	if (pud_is_leaf(*pud)) {
 		pud_clear(pud);
 		return 1;
 	}
@@ -1136,7 +1136,7 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
 
 int pmd_clear_huge(pmd_t *pmd)
 {
-	if (pmd_huge(*pmd)) {
+	if (pmd_is_leaf(*pmd)) {
 		pmd_clear(pmd);
 		return 1;
 	}
-- 
2.23.0

Re: [PATCH] powerpc/64s/radix: Fix unmapping huge vmaps when CONFIG_HUGETLB_PAGE=n

[Daniel Axtens]

Hi,

> pmd_huge is defined out to false when HUGETLB_PAGE is not configured,
> but the vmap code still installs huge PMDs. This leads to errors
> encountering bad PMDs when vunmapping because it is not seen as a
> huge PTE, and the bad PMD check catches it. The end result may not
> be much more serious than some bad pmd warning messages, because the
> pmd_none_or_clear_bad() does what we wanted and clears the huge PTE
> anyway.

Huh. So vmap seems to key off arch_vmap_p?d_supported which checks for
radix and HAVE_ARCH_HUGE_VMAP.

> Fix this by checking pmd_is_leaf(), which checks for a PTE regardless
> of config options. The whole huge/large/leaf stuff is a tangled mess
> but that's kernel-wide and not something we can improve much in
> arch/powerpc code.

I guess I'm a bit late to the party here because p?d_is_leaf was added
in 2019 in commit d6eacedd1f0e ("powerpc/book3s: Use config independent
helpers for page table walk") but why wouldn't we just make pmd_huge()
not config dependent?

Also, looking at that commit, there are a few places that might still
throw warnings, e.g. find_linux_pte, find_current_mm_pte, pud_page which
seem like they might still throw warnings if they were to encounter a
huge vmap page:

struct page *pud_page(pud_t pud)
{
	if (pud_is_leaf(pud)) {
		VM_WARN_ON(!pud_huge(pud));

Do these functions need special treatment for huge vmappings()?

Apart from those questions, the patch itself makes sense to me and I can
follow how it would fix a problem.

Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

Re: [PATCH] powerpc/64s/radix: Fix unmapping huge vmaps when CONFIG_HUGETLB_PAGE=n

[Nicholas Piggin]

Excerpts from Daniel Axtens's message of November 26, 2021 4:09 pm:
> Hi,
> 
>> pmd_huge is defined out to false when HUGETLB_PAGE is not configured,
>> but the vmap code still installs huge PMDs. This leads to errors
>> encountering bad PMDs when vunmapping because it is not seen as a
>> huge PTE, and the bad PMD check catches it. The end result may not
>> be much more serious than some bad pmd warning messages, because the
>> pmd_none_or_clear_bad() does what we wanted and clears the huge PTE
>> anyway.
> 
> Huh. So vmap seems to key off arch_vmap_p?d_supported which checks for
> radix and HAVE_ARCH_HUGE_VMAP.
> 
>> Fix this by checking pmd_is_leaf(), which checks for a PTE regardless
>> of config options. The whole huge/large/leaf stuff is a tangled mess
>> but that's kernel-wide and not something we can improve much in
>> arch/powerpc code.
> 
> I guess I'm a bit late to the party here because p?d_is_leaf was added
> in 2019 in commit d6eacedd1f0e ("powerpc/book3s: Use config independent
> helpers for page table walk") but why wouldn't we just make pmd_huge()
> not config dependent?

I guess so it constant folds code if hugetlbfs is not configured 
(and maybe so !huge kernels would correctly print a bad PMD warning if
they got huge PMD in user mappings).

> 
> Also, looking at that commit, there are a few places that might still
> throw warnings, e.g. find_linux_pte, find_current_mm_pte, pud_page which
> seem like they might still throw warnings if they were to encounter a
> huge vmap page:
> 
> struct page *pud_page(pud_t pud)
> {
> 	if (pud_is_leaf(pud)) {
> 		VM_WARN_ON(!pud_huge(pud));

Oh, hmm. That is used in vmalloc.c so maybe that warning should be
removed as a false positive. Good catch.

> Do these functions need special treatment for huge vmappings()?

find_linux_pte etc could be called for vmaps. I'm not sure I see a
problem in that function.

Thanks,
Nick

> 
> Apart from those questions, the patch itself makes sense to me and I can
> follow how it would fix a problem.
> 
> Reviewed-by: Daniel Axtens <dja@axtens.net>
> 
> Kind regards,
> Daniel
>