Re: [PATCH RFC 01/10] vmalloc: Add basic perm alloc implementation

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Sat, 2020-11-21 at 20:10 -0800, Andy Lutomirski wrote:
> On Fri, Nov 20, 2020 at 12:30 PM Rick Edgecombe
> <rick.p.edgecombe@intel.com> wrote:
> > In order to allow for future arch specific optimizations for
> > vmalloc
> > permissions, first add an implementation of a new interface that
> > will
> > work cross arch by using the existing set_memory_() functions.
> > 
> > When allocating some memory that will be RO, for example it should
> > be used
> > like:
> > 
> > /* Reserve va */
> > struct perm_allocation *alloc = perm_alloc(vstart, vend, page_cnt,
> > PERM_R);
> 
> I'm sure I could reverse-engineer this from the code, but:
> 
> Where do vstart and vend come from?

They are the start and end virtual address range to try to allocate in,
like __vmalloc_node_range() has. So like, MODULES_VADDR and
MODULES_END. Sorry for the terse example. The header in this patch has
some comments about each of the new functions to supplement it a bit.

> Does perm_alloc() allocate memory or just virtual addresses? Is the
> caller expected to call vmalloc()?

The caller does not need to call vmalloc(). perm_alloc() behaves
similar to __vmalloc_node_range(), where it allocates both memory and
virtual addresses. I left a little wiggle room in the descriptions in
the header, that the virtual address range doesn't actually need to be
mapped until after perm_writable_finish(). But both of the
implementations in this series will map it right away like a vmalloc().

So the interface could actually pretty easily be changed to look like
another flavor of vmalloc() that just returns a pointer to allocation.
The reason why it returns this new struct instead is that, unlike most
vmalloc()'s, the callers will be looking up metadata about the
allocation a bunch of times (the writable address). Having this
metadata stored in some struct inside vmalloc would mean
perm_writable_addr() would have to do something like find_vmap_area()
every time in order to find the writable allocation address from the
allocations address passed in. So returning a struct makes it so the
writable translation can skip a global lock and lookup. 

Another option could be putting the new metadata in vm_struct and just
return that, like get_vm_area(). Then we don't need to invent a new
struct. But then normal vmalloc()'s would have a bit of wasted memory
since they don't need this metadata.

A nice thing about that though, is there would be a central place to
translate to the writable addresses in cases where only a virtual
address is available. In later patches here, a similar lookup happens
anyway for modules using __module_address() to get the writable
address. This is due to some existing code where plumbing the new
struct all the way through would have resulted in too many changes.

I'm not sure which is best.

> How does one free this thing?

void perm_free(struct perm_allocation *alloc);