linux-kernel - Re: [PATCHv4 17/18] x86/mm: Handle encrypted memory in page_to_virt() and _

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: <20180627215658.ol5zq3o5746gizpu@black.fi.intel.com>
Date:   Thu, 28 Jun 2018 00:56:59 +0300
From:   "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Kai Huang <kai.huang@...ux.intel.com>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCHv4 17/18] x86/mm: Handle encrypted memory in
 page_to_virt() and __pa()

On Tue, Jun 26, 2018 at 04:38:23PM +0000, Dave Hansen wrote:
> > diff --git a/arch/x86/include/asm/mktme.h b/arch/x86/include/asm/mktme.h
> > index ba83fba4f9b3..dbfbd955da98 100644
> > --- a/arch/x86/include/asm/mktme.h
> > +++ b/arch/x86/include/asm/mktme.h
> > @@ -29,6 +29,9 @@ void arch_free_page(struct page *page, int order);
> >  
> >  int sync_direct_mapping(void);
> >  
> > +#define page_to_virt(x) \
> > +	(__va(PFN_PHYS(page_to_pfn(x))) + page_keyid(x) * direct_mapping_size)
> 
> Please put this in a generic header so that this hunk represents the
> *default* x86 implementation that is used universally on x86.

As I said, I disagree with you on the style preference.

If a maintainer prefers it to be done in your way, I'll move the macros.

> Then, please do
> 
> #ifndef CONFIG_MKTME_WHATEVER
> #define page_keyid(x) (0)
> #endif

Default page_keyid() implementation returns 0.

> >  #else
> >  #define mktme_keyid_mask	((phys_addr_t)0)
> >  #define mktme_nr_keyids		0
> > diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
> > index 53c32af895ab..ffad496aadad 100644
> > --- a/arch/x86/include/asm/page_64.h
> > +++ b/arch/x86/include/asm/page_64.h
> > @@ -23,7 +23,7 @@ static inline unsigned long __phys_addr_nodebug(unsigned long x)
> >  	/* use the carry flag to determine if x was < __START_KERNEL_map */
> >  	x = y + ((x > y) ? phys_base : (__START_KERNEL_map - PAGE_OFFSET));
> >  
> > -	return x;
> > +	return x % direct_mapping_size;
> 
> There are almost *surely* performance implications from this that affect
> anyone with this compile option turned on.  There's now a 64-bit integer
> division operation which is used in places like kfree().

Fair point. Apparently, modern CPU is good enough to hide the overhead.
I'll look into how to avoid division.

After quick look the only way to get it cheap (near free on my CPU) is to
have power-of-2 direct_mapping_size and mask address before returning it.

If direct_mapping_size is not power-of-2, the best variant I've come up
with so far costs a branch for non-encrypted memory.

For encrypted it is branch, 32-bit division and some bit shifting and
masking.

I'll look into this more.

-- 
 Kirill A. Shutemov