On Sun, Apr 03, 2022 at 10:49:28AM +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 <[email protected]>
> Signed-off-by: Muchun Song <[email protected]>
> ---
> 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 94e147e5456c..09eaae46a19b 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -535,7 +535,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_present(pmd) && !(pmd_val(pmd) & PMD_TABLE_BIT))
> #define pmd_bad(pmd) (!pmd_table(pmd))
>
> #define pmd_leaf_size(pmd) (pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
A bunch of the users of pmd_leaf() already check pmd_present() -- is it
documented that we need to handle this check inside the macro? afaict x86
doesn't do this either.
Will
On 04/04/2022 10:19, Will Deacon wrote:
> On Sun, Apr 03, 2022 at 10:49:28AM +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 <[email protected]>
>> Signed-off-by: Muchun Song <[email protected]>
>> ---
>> 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 94e147e5456c..09eaae46a19b 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -535,7 +535,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_present(pmd) && !(pmd_val(pmd) & PMD_TABLE_BIT))
>> #define pmd_bad(pmd) (!pmd_table(pmd))
>>
>> #define pmd_leaf_size(pmd) (pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
>
> A bunch of the users of pmd_leaf() already check pmd_present() -- is it
> documented that we need to handle this check inside the macro? afaict x86
> doesn't do this either.
The documentation is with the fallback implementations that always
return 0:
> /*
> * p?d_leaf() - true if this entry is a final mapping to a physical address.
> * 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.
> * Only meaningful when called on a valid entry.
> */
I guess the term "valid entry" is a bit vague but my intention was that
meant p?d_present().
I have to admit I hadn't considered PROT_NONE mappings before.
Steve
On Mon, Apr 4, 2022 at 5:20 PM Will Deacon <[email protected]> wrote:
>
> On Sun, Apr 03, 2022 at 10:49:28AM +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 <[email protected]>
> > Signed-off-by: Muchun Song <[email protected]>
> > ---
> > 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 94e147e5456c..09eaae46a19b 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -535,7 +535,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_present(pmd) && !(pmd_val(pmd) & PMD_TABLE_BIT))
> > #define pmd_bad(pmd) (!pmd_table(pmd))
> >
> > #define pmd_leaf_size(pmd) (pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
>
> A bunch of the users of pmd_leaf() already check pmd_present() -- is it
> documented that we need to handle this check inside the macro? afaict x86
> doesn't do this either.
>
arm64 is different from x86 here. pmd_leaf() could return true for
the none pmd without pmd_present() check, the check of
pmd_present() aims to exclude the pmd_none() case. However,
it could work properly on x86 without pmd_present() or pmd_none().
So we don't see pmd_present() or pmd_none() check in pmd_leaf().
For this reason, I think this check is necessary.
BTW, there are some users of pmd_leaf() (e.g. apply_to_pmd_range,
walk_pmd_range, ptdump_pmd_entry) which do not check pmd_present()
or pmd_none() before the call of pmd_leaf(). So it is also necessary
to add the check.
Thanks.