[PATCH] arm64: mm: fix pmd_leaf()

[PATCH] 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>
---
 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)
-- 
2.11.0

[PATCH] 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>
---
 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)
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Will Deacon]

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 <quic_qiancai@quicinc.com>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  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

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

[Will Deacon]

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 <quic_qiancai@quicinc.com>
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Steven Price]

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 <quic_qiancai@quicinc.com>
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>> ---
>>  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

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

[Steven Price]

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 <quic_qiancai@quicinc.com>
>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>> ---
>>  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Muchun Song]

On Mon, Apr 4, 2022 at 5:20 PM Will Deacon <will@kernel.org> 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 <quic_qiancai@quicinc.com>
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  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.

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

[Muchun Song]

On Mon, Apr 4, 2022 at 5:20 PM Will Deacon <will@kernel.org> 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 <quic_qiancai@quicinc.com>
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Aneesh Kumar K V]

On 4/4/22 5:10 PM, Muchun Song wrote:
> On Mon, Apr 4, 2022 at 5:20 PM Will Deacon <will@kernel.org> 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 <quic_qiancai@quicinc.com>
>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>> ---
>>>   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.


ppc64 also doesn't do a pmd_present check.

>>
> 
> 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.
> 


I would expect pmd_leaf check to return true, if the said pmd page table 
entry can point to a leaf page table entry which can also be a not 
present page table entry?

-aneesh

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

[Aneesh Kumar K V]

On 4/4/22 5:10 PM, Muchun Song wrote:
> On Mon, Apr 4, 2022 at 5:20 PM Will Deacon <will@kernel.org> 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 <quic_qiancai@quicinc.com>
>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>> ---
>>>   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.


ppc64 also doesn't do a pmd_present check.

>>
> 
> 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.
> 


I would expect pmd_leaf check to return true, if the said pmd page table 
entry can point to a leaf page table entry which can also be a not 
present page table entry?

-aneesh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Muchun Song]

On Mon, Apr 4, 2022 at 10:10 PM Aneesh Kumar K V
<aneesh.kumar@linux.ibm.com> wrote:
>
> On 4/4/22 5:10 PM, Muchun Song wrote:
> > On Mon, Apr 4, 2022 at 5:20 PM Will Deacon <will@kernel.org> 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 <quic_qiancai@quicinc.com>
> >>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> >>> ---
> >>>   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.
>
>
> ppc64 also doesn't do a pmd_present check.
>
> >>
> >
> > 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.
> >
>
>
> I would expect pmd_leaf check to return true, if the said pmd page table
> entry can point to a leaf page table entry which can also be a not
> present page table entry?
>

All right. In order to exclude the pmd_none() case.  How about
the following code?

#define pmd_leaf(pmd)                (pmd_val(pmd) && !(pmd_val(pmd) &
PMD_TABLE_BIT))

Thanks.

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

[Muchun Song]

On Mon, Apr 4, 2022 at 10:10 PM Aneesh Kumar K V
<aneesh.kumar@linux.ibm.com> wrote:
>
> On 4/4/22 5:10 PM, Muchun Song wrote:
> > On Mon, Apr 4, 2022 at 5:20 PM Will Deacon <will@kernel.org> 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 <quic_qiancai@quicinc.com>
> >>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> >>> ---
> >>>   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.
>
>
> ppc64 also doesn't do a pmd_present check.
>
> >>
> >
> > 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.
> >
>
>
> I would expect pmd_leaf check to return true, if the said pmd page table
> entry can point to a leaf page table entry which can also be a not
> present page table entry?
>

All right. In order to exclude the pmd_none() case.  How about
the following code?

#define pmd_leaf(pmd)                (pmd_val(pmd) && !(pmd_val(pmd) &
PMD_TABLE_BIT))

Thanks.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel