Messages in this thread | | | Date | Thu, 14 Apr 2022 19:18:11 +0800 | From | Muchun Song <> | Subject | Re: [PATCH v2] arm64: mm: fix pmd_leaf() |
| |
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.
| |