[MODERATED] Re: [PATCH 4/8] L1TFv8 8

[Michal Hocko <mhocko@suse.com>]

On Thu 28-06-18 10:05:47, speck for Vlastimil Babka wrote:
> On 06/27/2018 05:51 PM, speck for Michal Hocko wrote:
> > Jan Beulich has noticed that these are not correct on 32b PAE systems
> > because phys_addr_t is wider than unsigned long. So we need an explicit
> > cas for pfn_* and use phys_addr_t for other direction. I think we want
> > the following:
> > 
> >>From d3050e2b99e9070defcd990b7bc31a4b433367c5 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Wed, 27 Jun 2018 17:46:50 +0200
> > Subject: [PATCH] x86/speculation/l1tf: fix up pte->pfn conversion for PAE
> > 
> > Jan has noticed that pte_pfn and co. resp. pfn_pte are incorrect for
> > CONFIG_PAE because phys_addr_t is wider than unsigned long and so the
> > pte_val reps. shift left would get truncated. Fix this up by using
> > proper types.
> > 
> > Noticed-by: Jan Beulich <JBeulich@suse.com>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Good catch. Looks good to me, and some basic printk tests on manually
> created and modified pte's confirm that the problem does exist and the
> fix works.
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks for the review. Btw. could you add
Fixes: 6b28baca9b1f ("x86/speculation/l1tf: Protect PROT_NONE PTEs against speculation")

if you haven't pushed this yet Thomas?

> 
> > ---
> >  arch/x86/include/asm/pgtable.h | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> > index 6a090a76fdca..26fd42a91946 100644
> > --- a/arch/x86/include/asm/pgtable.h
> > +++ b/arch/x86/include/asm/pgtable.h
> > @@ -191,21 +191,21 @@ static inline u64 protnone_mask(u64 val);
> >  
> >  static inline unsigned long pte_pfn(pte_t pte)
> >  {
> > -	unsigned long pfn = pte_val(pte);
> > +	phys_addr_t pfn = pte_val(pte);
> >  	pfn ^= protnone_mask(pfn);
> >  	return (pfn & PTE_PFN_MASK) >> PAGE_SHIFT;
> >  }
> >  
> >  static inline unsigned long pmd_pfn(pmd_t pmd)
> >  {
> > -	unsigned long pfn = pmd_val(pmd);
> > +	phys_addr_t pfn = pmd_val(pmd);
> >  	pfn ^= protnone_mask(pfn);
> >  	return (pfn & pmd_pfn_mask(pmd)) >> PAGE_SHIFT;
> >  }
> >  
> >  static inline unsigned long pud_pfn(pud_t pud)
> >  {
> > -	unsigned long pfn = pud_val(pud);
> > +	phys_addr_t pfn = pud_val(pud);
> >  	pfn ^= protnone_mask(pfn);
> >  	return (pfn & pud_pfn_mask(pud)) >> PAGE_SHIFT;
> >  }
> > @@ -555,7 +555,7 @@ static inline pgprotval_t check_pgprot(pgprot_t pgprot)
> >  
> >  static inline pte_t pfn_pte(unsigned long page_nr, pgprot_t pgprot)
> >  {
> > -	phys_addr_t pfn = page_nr << PAGE_SHIFT;
> > +	phys_addr_t pfn = (phys_addr_t)page_nr << PAGE_SHIFT;
> >  	pfn ^= protnone_mask(pgprot_val(pgprot));
> >  	pfn &= PTE_PFN_MASK;
> >  	return __pte(pfn | check_pgprot(pgprot));
> > @@ -563,7 +563,7 @@ static inline pte_t pfn_pte(unsigned long page_nr, pgprot_t pgprot)
> >  
> >  static inline pmd_t pfn_pmd(unsigned long page_nr, pgprot_t pgprot)
> >  {
> > -	phys_addr_t pfn = page_nr << PAGE_SHIFT;
> > +	phys_addr_t pfn = (phys_addr_t)page_nr << PAGE_SHIFT;
> >  	pfn ^= protnone_mask(pgprot_val(pgprot));
> >  	pfn &= PHYSICAL_PMD_PAGE_MASK;
> >  	return __pmd(pfn | check_pgprot(pgprot));
> > @@ -571,7 +571,7 @@ static inline pmd_t pfn_pmd(unsigned long page_nr, pgprot_t pgprot)
> >  
> >  static inline pud_t pfn_pud(unsigned long page_nr, pgprot_t pgprot)
> >  {
> > -	phys_addr_t pfn = page_nr << PAGE_SHIFT;
> > +	phys_addr_t pfn = (phys_addr_t)page_nr << PAGE_SHIFT;
> >  	pfn ^= protnone_mask(pgprot_val(pgprot));
> >  	pfn &= PHYSICAL_PUD_PAGE_MASK;
> >  	return __pud(pfn | check_pgprot(pgprot));
> > 
> 

-- 
Michal Hocko
SUSE Labs