linux-kernel - Re: [PATCH 1/5] ioremap: Rework pXd_free_pYd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f4b73d9d61e87712fec1712bb6225e2b385a16e3.camel@hpe.com>
Date:   Mon, 17 Sep 2018 18:38:28 +0000
From:   "Kani, Toshi" <toshi.kani@....com>
To:     "will.deacon@....com" <will.deacon@....com>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "cpandya@...eaurora.org" <cpandya@...eaurora.org>,
        "Hocko, Michal" <MHocko@...e.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH 1/5] ioremap: Rework pXd_free_pYd_page() API

On Mon, 2018-09-17 at 12:33 +0100, Will Deacon wrote:
> On Fri, Sep 14, 2018 at 09:10:49PM +0000, Kani, Toshi wrote:
> > On Fri, 2018-09-14 at 14:36 -0600, Toshi Kani wrote:
> > > On Wed, 2018-09-12 at 11:26 +0100, Will Deacon wrote:
> > > > The recently merged API for ensuring break-before-make on page-table
> > > > entries when installing huge mappings in the vmalloc/ioremap region is
> > > > fairly counter-intuitive, resulting in the arch freeing functions
> > > > (e.g. pmd_free_pte_page()) being called even on entries that aren't
> > > > present. This resulted in a minor bug in the arm64 implementation, giving
> > > > rise to spurious VM_WARN messages.
> > > > 
> > > > This patch moves the pXd_present() checks out into the core code,
> > > > refactoring the callsites at the same time so that we avoid the complex
> > > > conjunctions when determining whether or not we can put down a huge
> > > > mapping.
> > > > 
> > > > Cc: Chintan Pandya <cpandya@...eaurora.org>
> > > > Cc: Toshi Kani <toshi.kani@....com>
> > > > Cc: Thomas Gleixner <tglx@...utronix.de>
> > > > Cc: Michal Hocko <mhocko@...e.com>
> > > > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > > > Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> > > > Signed-off-by: Will Deacon <will.deacon@....com>
> > > 
> > > Yes, this looks nicer.
> > > 
> > > Reviewed-by: Toshi Kani <toshi.kani@....com>
> > 
> > Sorry, I take it back since I got a question...
> > 
> > +static int ioremap_try_huge_pmd(pmd_t *pmd, unsigned long addr,
> > > +				unsigned long end, phys_addr_t
> > 
> > phys_addr,
> > > +				pgprot_t prot)
> > > +{
> > > +	if (!ioremap_pmd_enabled())
> > > +		return 0;
> > > +
> > > +	if ((end - addr) != PMD_SIZE)
> > > +		return 0;
> > > +
> > > +	if (!IS_ALIGNED(phys_addr, PMD_SIZE))
> > > +		return 0;
> > > +
> > > +	if (pmd_present(*pmd) && !pmd_free_pte_page(pmd, addr))
> > > +		return 0;
> > 
> > Is pm_present() a proper check here?  We probably do not have this case
> > for iomap, but I wonder if one can drop p-bit while it has a pte page
> > underneath.
> 
> For ioremap/vunmap the pXd_present() check is correct, yes. The vunmap()
> code only ever clears leaf entries, leaving table entries intact. 

Right. I was thinking if such case happens in future.

> If it
> did clear table entries, you'd be stuck here because you wouldn't have
> the address of the table to free.
> 
> If somebody called pmd_mknotpresent() on a table entry, we may run into
> problems, but it's only used for huge mappings afaict.

Treating a table entry valid when p-bit is off is risky as well. So, I
agree with the pXd_present() check.

Reviewed-by: Toshi Kani <toshi.kani@....com>

Thanks,
-Toshi