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)" <r...@kernel.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) <r...@kernel.org>
> > Reviewed-by: Luis Chamberlain <mcg...@kernel.org>
> > Tested-by: kdevops <kdev...@lists.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.
