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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Reviewed-by: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> > Tested-by: kdevops <kdevops@xxxxxxxxxxxxxxx>
> > ---
>
> [...]
>
> > +
> > +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.
