Message-ID: <Z8HAWu4zQFeg19KR@kernel.org>
Date: Fri, 28 Feb 2025 15:55:38 +0200
From: Mike Rapoport <rppt@...nel.org>
To: Ryan Roberts <ryan.roberts@....com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Luis Chamberlain <mcgrof@...nel.org>, Dev Jain <dev.jain@....com>,
	Andreas Larsson <andreas@...sler.com>,
	Andy Lutomirski <luto@...nel.org>, Ard Biesheuvel <ardb@...nel.org>,
	Arnd Bergmann <arnd@...db.de>, Borislav Petkov <bp@...en8.de>,
	Brian Cain <bcain@...cinc.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Christoph Hellwig <hch@...radead.org>,
	Christophe Leroy <christophe.leroy@...roup.eu>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Dinh Nguyen <dinguyen@...nel.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Guo Ren <guoren@...nel.org>, Helge Deller <deller@....de>,
	Huacai Chen <chenhuacai@...nel.org>, Ingo Molnar <mingo@...hat.com>,
	Johannes Berg <johannes@...solutions.net>,
	John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>,
	Kent Overstreet <kent.overstreet@...ux.dev>,
	"Liam R. Howlett" <Liam.Howlett@...cle.com>,
	Mark Rutland <mark.rutland@....com>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Matt Turner <mattst88@...il.com>, Max Filippov <jcmvbkbc@...il.com>,
	Michael Ellerman <mpe@...erman.id.au>,
	Michal Simek <monstr@...str.eu>, Oleg Nesterov <oleg@...hat.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Richard Weinberger <richard@....at>,
	Russell King <linux@...linux.org.uk>, Song Liu <song@...nel.org>,
	Stafford Horne <shorne@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Suren Baghdasaryan <surenb@...gle.com>,
	Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Uladzislau Rezki <urezki@...il.com>,
	Vineet Gupta <vgupta@...nel.org>, Will Deacon <will@...nel.org>,
	bpf@...r.kernel.org, linux-alpha@...r.kernel.org,
	linux-arch@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-csky@...r.kernel.org, linux-hexagon@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-m68k@...ts.linux-m68k.org,
	linux-mips@...r.kernel.org, linux-mm@...ck.org,
	linux-modules@...r.kernel.org, linux-openrisc@...r.kernel.org,
	linux-parisc@...r.kernel.org, linux-riscv@...ts.infradead.org,
	linux-sh@...r.kernel.org, linux-snps-arc@...ts.infradead.org,
	linux-trace-kernel@...r.kernel.org, linux-um@...ts.infradead.org,
	linuxppc-dev@...ts.ozlabs.org, loongarch@...ts.linux.dev,
	sparclinux@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH v7 7/8] execmem: add support for cache of large ROX pages

Hi Ryan,

On Thu, Feb 27, 2025 at 11:13:29AM +0000, Ryan Roberts wrote:
> Hi Mike,
> 
> Drive by review comments below...
> 
> 
> On 23/10/2024 17:27, Mike Rapoport wrote:
> > From: "Mike Rapoport (Microsoft)" <rppt@...nel.org>
> > 
> > Using large pages to map text areas reduces iTLB pressure and improves
> > performance.
> > 
> > Extend execmem_alloc() with an ability to use huge pages with ROX
> > permissions as a cache for smaller allocations.
> > 
> > To populate the cache, a writable large page is allocated from vmalloc with
> > VM_ALLOW_HUGE_VMAP, filled with invalid instructions and then remapped as
> > ROX.
> > 
> > The direct map alias of that large page is exculded from the direct map.
> > 
> > Portions of that large page are handed out to execmem_alloc() callers
> > without any changes to the permissions.
> > 
> > When the memory is freed with execmem_free() it is invalidated again so
> > that it won't contain stale instructions.
> > 
> > An architecture has to implement execmem_fill_trapping_insns() callback
> > and select ARCH_HAS_EXECMEM_ROX configuration option to be able to use
> > the ROX cache.
> > 
> > The cache is enabled on per-range basis when an architecture sets
> > EXECMEM_ROX_CACHE flag in definition of an execmem_range.
> > 
> > Signed-off-by: Mike Rapoport (Microsoft) <rppt@...nel.org>
> > Reviewed-by: Luis Chamberlain <mcgrof@...nel.org>
> > Tested-by: kdevops <kdevops@...ts.linux.dev>
> > ---
> 
> [...]
> 
> > +
> > +static int execmem_cache_populate(struct execmem_range *range, size_t size)
> > +{
> > +	unsigned long vm_flags = VM_ALLOW_HUGE_VMAP;
> > +	unsigned long start, end;
> > +	struct vm_struct *vm;
> > +	size_t alloc_size;
> > +	int err = -ENOMEM;
> > +	void *p;
> > +
> > +	alloc_size = round_up(size, PMD_SIZE);
> > +	p = execmem_vmalloc(range, alloc_size, PAGE_KERNEL, vm_flags);
> 
> Shouldn't this be passing PAGE_KERNEL_ROX? Otherwise I don't see how the
> allocated memory is ROX? I don't see any call below where you change the permission.

The memory is allocated RW, filled with invalid instructions, unammped in
vmalloc space, removed from the direct map and then mapped as ROX in
vmalloc address space.
 
> Given the range has the pgprot in it, you could just drop passing the pgprot
> explicitly here and have execmem_vmalloc() use range->pgprot directly?

Here range->prprot and the prot passed to vmalloc are different.
 
> Thanks,
> Ryan
> 
> > +	if (!p)
> > +		return err;
> > +
> > +	vm = find_vm_area(p);
> > +	if (!vm)
> > +		goto err_free_mem;
> > +
> > +	/* fill memory with instructions that will trap */
> > +	execmem_fill_trapping_insns(p, alloc_size, /* writable = */ true);
> > +
> > +	start = (unsigned long)p;
> > +	end = start + alloc_size;
> > +
> > +	vunmap_range(start, end);
> > +
> > +	err = execmem_set_direct_map_valid(vm, false);
> > +	if (err)
> > +		goto err_free_mem;
> > +
> > +	err = vmap_pages_range_noflush(start, end, range->pgprot, vm->pages,
> > +				       PMD_SHIFT);
> > +	if (err)
> > +		goto err_free_mem;
> > +
> > +	err = execmem_cache_add(p, alloc_size);
> > +	if (err)
> > +		goto err_free_mem;
> > +
> > +	return 0;
> > +
> > +err_free_mem:
> > +	vfree(p);
> > +	return err;
> > +}
> 
> [...]
> 

-- 
Sincerely yours,
Mike.

Powered by blists - more mailing lists