linux-kernel - Re: [PATCH] x86/vmemmap: Synchronize with global pgds if populating init

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] [day] [month] [year] [list]

Message-ID: <Z72BHNv18NNq1e2F@localhost.localdomain>
Date: Tue, 25 Feb 2025 09:36:44 +0100
From: Oscar Salvador <osalvador@...e.de>
To: Dave Hansen <dave.hansen@...el.com>
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>,
	linux-kernel@...r.kernel.org, 42.hyeyoo@...il.com, byungchul@...com,
	dave.hansen@...ux.intel.com, luto@...nel.org, peterz@...radead.org,
	akpm@...ux-foundation.org, stable@...r.kernel.org,
	linux-mm@...ck.org, max.byungchul.park@...com,
	max.byungchul.park@...il.com
Subject: Re: [PATCH] x86/vmemmap: Synchronize with global pgds if populating
 init_mm's pgd

On Thu, Feb 20, 2025 at 10:02:54AM -0800, Dave Hansen wrote:
> On 2/19/25 22:41, Gwan-gyeong Mun wrote:
> > diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> > index 01ea7c6df303..7935859bcc21 100644
> > --- a/arch/x86/mm/init_64.c
> > +++ b/arch/x86/mm/init_64.c
> > @@ -1498,6 +1498,54 @@ static long __meminitdata addr_start, addr_end;
> >  static void __meminitdata *p_start, *p_end;
> >  static int __meminitdata node_start;
> >  
> > +static void * __meminit vmemmap_alloc_block_zero(unsigned long size, int node)
> > +{
> > +	void *p = vmemmap_alloc_block(size, node);
> > +
> > +	if (!p)
> > +		return NULL;
> > +	memset(p, 0, size);
> > +
> > +	return p;
> > +}
> 
> This is a pure copy and paste of the generic function. I assume this is
> because the mm/sparse-vmemmap.c is static. But this kind of copying is
> really unfortunate.

Agreed.

> ...
> > +pgd_t * __meminit vmemmap_pgd_populate(unsigned long addr, int node)
> > +{
> > +	pgd_t *pgd = pgd_offset_k(addr);
> > +
> > +	if (pgd_none(*pgd)) {
> > +		void *p = vmemmap_alloc_block_zero(PAGE_SIZE, node);
> > +
> > +		if (!p)
> > +			return NULL;
> > +
> > +		pgd_populate(&init_mm, pgd, p);
> > +		sync_global_pgds(addr, addr);
> > +	}
> > +
> > +	return pgd;
> > +}
> 
> I'd _really_ like to find another way to do this. We really don't want
> to add copy-and-paste versions of generic functions that we now need to
> maintain on the x86 side.
> 
> The _best_ way is probably to create some p*d_populate_kernel() helpers:
> 
> void pgd_populate_kernel(unsigned long addr, pgd_t *pgd, p4d_t *p4d)
> {
> 	pgd_populate(&init_mm, pgd, p4d);
> 	arch_sync_global_pgds(addr, addr+something);
> }
> 
> and move over most of the callers of:
> 
> 	p*d_populate(&init_mm, ...);

I think this makes more sense, yes.
So for those that do not need the sync, arch_sync_* would be just a
noop, so we could avoid all these code duplication.


-- 
Oscar Salvador
SUSE Labs