Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220403024928.4125-1-songmuchun@bytedance.com>
 <20220404091957.GA22790@willie-the-truck> <CAMZfGtU=4aFmbwyyh+5=M3iaqox7X7yaoPP+r=WJO9dReaNb-g@mail.gmail.com>
 <de6529b0-9360-cf4a-4233-ee1498b44176@linux.ibm.com>
In-Reply-To: <de6529b0-9360-cf4a-4233-ee1498b44176@linux.ibm.com>
From:   Muchun Song <songmuchun@bytedance.com>
Date:   Mon, 4 Apr 2022 23:24:01 +0800
Message-ID: <CAMZfGtXieqZroJfvgV6eVM+ux0P28sjz6NMtZsNwkvdebBoztQ@mail.gmail.com>
Subject: Re: [PATCH] arm64: mm: fix pmd_leaf()
To:     Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>
Cc:     Will Deacon <will@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        steven.price@arm.com, lengxujun2007@126.com,
        Arnd Bergmann <arnd@arndb.de>,
        LAK <linux-arm-kernel@lists.infradead.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Muchun Song <smuchun@gmail.com>,
        Xiongchun duan <duanxiongchun@bytedance.com>,
        Qian Cai <quic_qiancai@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.