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

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

[Muchun Song]

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))
 
 #define pmd_leaf_size(pmd)	(pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
-- 
2.11.0

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

[Will Deacon]

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.

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

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

[Steven Price]

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

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

[Will Deacon]

On Wed, Apr 13, 2022 at 11:39:49AM +0100, Steven Price wrote:
> 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.

arm64 'valid' + PROT_NONE is roughly what 'present' means. So we could say
that this only works for present entries, but then Muchun's latest patch
wants to work with !present which is why I tried to work this through.

Will

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

[Muchun Song]

On Thu, Apr 14, 2022 at 11:05:35AM +0100, Will Deacon wrote:
> On Wed, Apr 13, 2022 at 11:39:49AM +0100, Steven Price wrote:
> > 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.
> 
> arm64 'valid' + PROT_NONE is roughly what 'present' means. So we could say
> that this only works for present entries, but then Muchun's latest patch
> wants to work with !present which is why I tried to work this through.
>

My bad. In the previous version, Aneesh seems want to make
pmd_leaf() works for a not present page table entry, I am
trying doing this in this version.  Seems like I do the right
thing in the previous version from your explanation.

I'll use the previos version and fix pud_leaf() as well and
update the documentation.  Do you think this is okay?

Thanks.



 

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

[Will Deacon]

On Thu, Apr 14, 2022 at 07:18:11PM +0800, Muchun Song wrote:
> On Thu, Apr 14, 2022 at 11:05:35AM +0100, Will Deacon wrote:
> > On Wed, Apr 13, 2022 at 11:39:49AM +0100, Steven Price wrote:
> > > On 13/04/2022 11:19, Will Deacon wrote:
> > > > 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.
> > 
> > arm64 'valid' + PROT_NONE is roughly what 'present' means. So we could say
> > that this only works for present entries, but then Muchun's latest patch
> > wants to work with !present which is why I tried to work this through.
> >
> 
> My bad. In the previous version, Aneesh seems want to make
> pmd_leaf() works for a not present page table entry, I am
> trying doing this in this version.  Seems like I do the right
> thing in the previous version from your explanation.
> 
> I'll use the previos version and fix pud_leaf() as well and
> update the documentation.  Do you think this is okay?

Yes, thanks, that sounds good to me. We can define both of these as present
&& !table.

Will