Re: [PATCH v2] arm64: mm: fix pmd_leaf()

[Steven Price <steven.price@arm.com>]

On 13/04/2022 11:19, Will Deacon wrote:
> On Mon, Apr 11, 2022 at 08:26:53PM +0800, Muchun Song wrote:
>> The pmd_leaf() is used to test a leaf mapped PMD, however, it misses
>> the PROT_NONE mapped PMD on arm64.  Fix it.  A real world issue [1]
>> caused by this was reported by Qian Cai.
>>
>> Link: https://patchwork.kernel.org/comment/24798260/ [1]
>> Fixes: 8aa82df3c123 ("arm64: mm: add p?d_leaf() definitions")
>> Reported-by: Qian Cai <quic_qiancai@quicinc.com>
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>> ---
>> v2:
>> - Replace pmd_present() with pmd_val() since we expect pmd_leaf() works
>>   well on non-present pmd case.
>>
>>  arch/arm64/include/asm/pgtable.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index ad9b221963d4..00cdd2d895d3 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -551,7 +551,7 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>>  				 PMD_TYPE_TABLE)
>>  #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>>  				 PMD_TYPE_SECT)
>> -#define pmd_leaf(pmd)		pmd_sect(pmd)
>> +#define pmd_leaf(pmd)		(pmd_val(pmd) && !(pmd_val(pmd) & PMD_TABLE_BIT))
>>  #define pmd_bad(pmd)		(!pmd_table(pmd))
> 
> I'm still trying to get my head around the desired semantics here.
> 
> If we want to fix the original report, then we need to take PROT_NONE
> entries into account. The easiest way to do that is, as you originally
> suggested, by using pmd_present():
> 
> #define pmd_leaf(pmd)	(pmd_present(pmd) && !pmd_table(pmd))
> 
> But now you seem to be saying that !pmd_present() entries should also be
> considered as pmd_leaf() -- is there a real need for that?
> 
> If so, then I think this simply becomes:
> 
> #define pmd_leaf(pmd)	(!pmd_table(pmd))
> 
> which is, amusingly, identical to pmd_bad().
> 
> The documentation/comment that Steven referred to also desperately needs
> clarifying as it currently states:
> 
>   "Only meaningful when called on a valid entry."
> 
> whatever that means.

The intention at the time is that this had the same meaning as
pmd_huge() (when CONFIG_HUGETLB_PAGE is defined), which would then match
this patch. This is referred in the comment, albeit in a rather weak way:

>  * This differs from p?d_huge() by the fact that they are always available (if
>  * the architecture supports large pages at the appropriate level) even
>  * if CONFIG_HUGETLB_PAGE is not defined.

However, the real issue here is that the definition of pmd_leaf() isn't
clear. I know what the original uses of it needed but since then it's
been used in other areas, and I'm afraid my 'documentation' isn't
precise enough to actually be useful.

At the time I wrote that comment I think I meant "valid" in the AArch64
sense (i.e. the LSB of the entry). PROT_NONE isn't 'valid' by that
definition (and I hadn't considered it). But of course that definition
of 'valid' is pretty meaningless in the cross-architecture case.

So I think we also need updated documentation which describes the
required semantics in an architecture-agnostic way.

Steve

> Finally, if this has implications beyond PROT_NONE (as I think you're
> suggesting in your v2) then pud_leaf() probably needs similar treatment.
> And we can remove pmd_sect() altogether if we no longer need it.
> 
> Will