| Message ID | 20220403024928.4125-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: mm: fix pmd_leaf() | expand |
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
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
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.
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
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.
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)
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(-)