linux-kernel - Re: [PATCH v4 6/7] mm/x86: Add missing pud helpers

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: <875xsc0xjy.ffs@tglx>
Date: Thu, 08 Aug 2024 00:37:21 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Peter Xu <peterx@...hat.com>, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org
Cc: "Aneesh Kumar K . V" <aneesh.kumar@...ux.ibm.com>, Michael Ellerman
 <mpe@...erman.id.au>, Oscar Salvador <osalvador@...e.de>, Dan Williams
 <dan.j.williams@...el.com>, James Houghton <jthoughton@...gle.com>,
 Matthew Wilcox <willy@...radead.org>, Nicholas Piggin <npiggin@...il.com>,
 Rik van Riel <riel@...riel.com>, Dave Jiang <dave.jiang@...el.com>, Andrew
 Morton <akpm@...ux-foundation.org>, x86@...nel.org, Ingo Molnar
 <mingo@...hat.com>, Rick P Edgecombe <rick.p.edgecombe@...el.com>, "Kirill
 A . Shutemov" <kirill@...temov.name>, peterx@...hat.com,
 linuxppc-dev@...ts.ozlabs.org, Mel Gorman <mgorman@...hsingularity.net>,
 Hugh Dickins <hughd@...gle.com>, Borislav Petkov <bp@...en8.de>, David
 Hildenbrand <david@...hat.com>, Vlastimil Babka <vbabka@...e.cz>, Dave
 Hansen <dave.hansen@...ux.intel.com>, Christophe Leroy
 <christophe.leroy@...roup.eu>, Huang Ying <ying.huang@...el.com>
Subject: Re: [PATCH v4 6/7] mm/x86: Add missing pud helpers

On Wed, Aug 07 2024 at 15:48, Peter Xu wrote:
> These new helpers will be needed for pud entry updates soon.  Introduce
> these helpers by referencing the pmd ones.  Namely:
>
> - pudp_invalidate()
> - pud_modify()

Zero content about what these helpers do and why they are needed. That's
not how it works, really.

  
> +static inline pud_t pud_mkinvalid(pud_t pud)
> +{
> +	return pfn_pud(pud_pfn(pud),
> +		       __pgprot(pud_flags(pud) & ~(_PAGE_PRESENT|_PAGE_PROTNONE)));

100 characters...

> +}
> +
>  static inline u64 flip_protnone_guard(u64 oldval, u64 val, u64 mask);
>  
>  static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
> @@ -834,14 +840,8 @@ static inline pmd_t pmd_modify(pmd_t pmd, pgprot_t newprot)
>  	pmd_result = __pmd(val);
>  
>  	/*
> -	 * To avoid creating Write=0,Dirty=1 PMDs, pte_modify() needs to avoid:
> -	 *  1. Marking Write=0 PMDs Dirty=1
> -	 *  2. Marking Dirty=1 PMDs Write=0
> -	 *
> -	 * The first case cannot happen because the _PAGE_CHG_MASK will filter
> -	 * out any Dirty bit passed in newprot. Handle the second case by
> -	 * going through the mksaveddirty exercise. Only do this if the old
> -	 * value was Write=1 to avoid doing this on Shadow Stack PTEs.
> +	 * Avoid creating shadow stack PMD by accident.  See comment in
> +	 * pte_modify().

The changelog is utterly silent about this comment update.

>  	 */
>  	if (oldval & _PAGE_RW)
>  		pmd_result = pmd_mksaveddirty(pmd_result);
> @@ -851,6 +851,29 @@ static inline pmd_t pmd_modify(pmd_t pmd, pgprot_t newprot)
>  	return pmd_result;
>  }
>  
> +static inline pud_t pud_modify(pud_t pud, pgprot_t newprot)
> +{
> +	pudval_t val = pud_val(pud), oldval = val;
> +	pud_t pud_result;
> +
> +	val &= _HPAGE_CHG_MASK;
> +	val |= check_pgprot(newprot) & ~_HPAGE_CHG_MASK;
> +	val = flip_protnone_guard(oldval, val, PHYSICAL_PUD_PAGE_MASK);
> +
> +	pud_result = __pud(val);
> +
> +	/*
> +	 * Avoid creating shadow stack PUD by accident.  See comment in
> +	 * pte_modify().
> +	 */
> +	if (oldval & _PAGE_RW)
> +		pud_result = pud_mksaveddirty(pud_result);
> +	else
> +		pud_result = pud_clear_saveddirty(pud_result);
> +
> +	return pud_result;
> +}
> +
>  /*
>   * mprotect needs to preserve PAT and encryption bits when updating
>   * vm_page_prot
> @@ -1389,10 +1412,26 @@ static inline pmd_t pmdp_establish(struct vm_area_struct *vma,
>  }
>  #endif
>  
> +static inline pud_t pudp_establish(struct vm_area_struct *vma,
> +		unsigned long address, pud_t *pudp, pud_t pud)

Random line break alignment.... See documentation.

> +{
> +	page_table_check_pud_set(vma->vm_mm, pudp, pud);
> +	if (IS_ENABLED(CONFIG_SMP)) {
> +		return xchg(pudp, pud);
> +	} else {
> +		pud_t old = *pudp;
> +		WRITE_ONCE(*pudp, pud);

Lacks a newline between variable declaration and code.

But seriously, why optimizing for !SMP? That's a pointless exercise and
a guarantee for bitrot.

> +		return old;
> +	}
> +}
> +
>  #define __HAVE_ARCH_PMDP_INVALIDATE_AD
>  extern pmd_t pmdp_invalidate_ad(struct vm_area_struct *vma,
>  				unsigned long address, pmd_t *pmdp);
>  
> +pud_t pudp_invalidate(struct vm_area_struct *vma, unsigned long address,
> +		      pud_t *pudp);

While 'extern' is not required, please keep the file style consistent
and use the 100 characters...

> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -641,6 +641,18 @@ pmd_t pmdp_invalidate_ad(struct vm_area_struct *vma, unsigned long address,
>  }
>  #endif
>  
> +#if defined(CONFIG_TRANSPARENT_HUGEPAGE) && \
> +	defined(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)
> +pud_t pudp_invalidate(struct vm_area_struct *vma, unsigned long address,
> +		     pud_t *pudp)
> +{
> +	VM_WARN_ON_ONCE(!pud_present(*pudp));
> +	pud_t old = pudp_establish(vma, address, pudp, pud_mkinvalid(*pudp));
> +	flush_pud_tlb_range(vma, address, address + HPAGE_PUD_SIZE);
> +	return old;

Your keyboard clearly lacks a newline key ...

Thanks,

        tglx