LinuxKPI: switch mallocarray to an lpi implementation using __kmalloc()

[bz]

Summary

With mallocarray() we cannot guarantee that any size larger than
PAGE_SIZE will be contiguous. Switch to use the internal __kmalloc()
implementation which now does provide that guarantee.

Sponsored by: The FreeBSD Foundation
MFC after: 3 days

[emaste]

OK

[bz]

Also change the (internal) mallocarray() calls in linux_compat.c to lkpi_mallocarray()
given we are using the LinuxKPI malloc type (M_KMALLOC) there already; better for
consistency.

[bz]

@jhb any comments?

[emaste]

Probably fine to go in

[manu]

I'm not a fan of this, mallocarray is a OpenBSD thing, not a Linux thing, I don't see why we should have an lkpi implementation.

[bz]

It's a helper function; because we do use mallocarray internally and it doesn't go by Linux guarantees on memory being physically contiguous in some cases. I can replace all the mallocarray calls directly with __kmalloc() but that just makes it harder to maintain in many places rather one. We can name this whatever we want it to be if you do not like the name...

[jhb]

This seems certainly fine. This is the semantics required for kcalloc and kmalloc_array

sys/compat/linuxkpi/common/include/linux/slab.h

115–116

This isn't enforcing contigmalloc for size > PAGE_SIZE?

147–148

Similarly, this also needs to enforce contigmalloc.

I would be tempted to implement the *calloc functions by setting the ZERO flag and then calling the malloc_array function, e.g.:

kcalloc_node()
{
   flags |= __GFP_ZERO;
   return kmalloc_array_node(...)
}

Same can be true for kcalloc() calling kmalloc_array(). Then, you could just have the ABI symbols be lkpi_kmalloc_array and lkpi_kmalloc_array_node as you do today for kmalloc. This also suggests btw that you need to add a shim for kmalloc_node that uses contigmalloc for larger sizes.

sys/compat/linuxkpi/common/src/linux_compat.c

2770–2771

Maybe use just use the kmalloc_array here instead and elsewhere in this file, then you aren't exposing the internal symbol more widely?

[bz]

Drop lkpi_mallocarray as requested by @manu.

Add a lkpi___kmalloc_node() in parallel to lkpi___kmalloc() which does the if and the actual
allocation in a C file so we do not spread malloc calls in inline functions anymore.

Follow the path suggested by @jhb which works out nicely:
Use two former functions as base for their *_array functions. Use those as base for the
remaing wrappers.

Switch to kmalloc_array() inseatd of our lkpi_ version to avoid further exposure of it.

Add comments about realloc as it seems kern_malloc.c does not handle that yet and we need
to have a chat about it.

[bz]

sys/compat/linuxkpi/common/src/linux_slab.c

230

@jhb, @emaste, This should be <= as well, correct?

[bz]

I really shouldn't code with a fever. Need to reshuffle the functions so they will compile. Also )

[bz]

Make compile as well fixing order and ()

[jhb]

sys/compat/linuxkpi/common/include/linux/slab.h

189

Yes, these need to have the same semantics as if you did:

void *new;

size_t old_size = alloc_size(ptr);
new = kmalloc(size, flags);
memcpy(new, ptr, old_size);
kfree(ptr);
return (new);

Probably you need a __lkpi_realloc to handle this case. If the old pointer was not contigmalloc()'d and the new size is <= PAGE_SIZE, you can call realloc, otherwise you need to do the code I pasted above. You can probably do this by teaching malloc_usable_size() in kern_malloc.c to handle the contigmalloc() case in its switch on the slab cookie, then you can implement __lkpi_realloc as something like:

old_size = malloc_usable_size(ptr);
if (size <= old_size)
    return (ptr);

if (old_size <= PAGE_SIZE && size <= PAGE_SIZE)
    return (realloc(....));

new = kmalloc(size);
memcpy(new, ptr, old_size);
kfree(ptr);
return (new);

198

I would reimplement this as a call to krealloc so then you only have to deal with fixing krealloc above.

[bz]

sys/compat/linuxkpi/common/include/linux/slab.h

189

Can you remind me why we did not do the native realloc for contigmalloc also? I'll go and have a look at the above, but can we do realloc in a separate review and make sure these bits here are in order?

[bz]

Use kmalloc instead of __kmalloc in one case.
Remove the XXX comment from realloc; separate review in a minute for that

[jhb]

sys/compat/linuxkpi/common/include/linux/slab.h

180–183

This one may be fine as is. The docs for kvmalloc are as such:

If you are not sure whether the allocation size is too large for
`kmalloc`, it is possible to use kvmalloc() and its derivatives. It will
try to allocate memory with `kmalloc` and if the allocation fails it
will be retried with `vmalloc`. There are restrictions on which GFP
flags can be used with `kvmalloc`; please see kvmalloc_node() reference
documentation. Note that `kvmalloc` may return memory that is not
physically contiguous.

That means that just using plain malloc for kvmalloc is probably ok, and kvmalloc_array should just be a simple wrapper around kvmalloc. I would probably implement kvmalloc_array as a wrapper around kvmalloc even though kvmalloc isn't yet implemented just for symmetry with the other *_array functions.

189

Well, contigmalloc() requests accept several parameters such as low/high memory, etc. that aren't available when realloc() is called (and aren't stored in the slab metadata). Should a realloc() of memory allocated vi contigmalloc() honor those original parameters? Probably, so realloc() should fail (as it does today) if a caller tries to extend a contigmalloc region.

[bz]

sys/compat/linuxkpi/common/include/linux/slab.h

180–183

kvmalloc is "implemented":

#define kvmalloc(size, flags)           kmalloc(size, flags)

I'll do the array as a wrapper around it. But that means we probably should implement kvmalloc() as function around malloc as well.

[bz]

Implement kvmalloc[_array] as suggested by @jhb

[bz]

Do we think we'll be good here now?