| Message ID | 20240717222427.2211-2-dakr@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Align kvrealloc() with krealloc() | expand |
On Thu, Jul 18, 2024 at 12:24:01AM +0200, Danilo Krummrich wrote: > +extern void * __must_check vrealloc_noprof(const void *p, size_t size, > + gfp_t flags) __realloc_size(2); No need for the extern here. It would also help readability a bit to keep the __realloc_size on a separate like, aka: void *__must_check vrealloc_noprof(const void *p, size_t size, gfp_t flags) __realloc_size(2); > + if (size <= old_size) { > + /* TODO: Can we optimize and shrink the allocation? What would > + * be a good metric for when to shrink the vm_area? > + */ Kernel comment style is to keep the /* on a line of it's own. A realloc that doesn't shrink is a bit pointless. The same heuristics as for krealloc should probably apply here, adjust to the fact that we always deal with whole pages. > + /* TODO: Can we optimize and extend the existing allocation if we have > + * enough contiguous space left in the virtual address space? > + */ I don't understand this comment. > +EXPORT_SYMBOL(vrealloc_noprof); New interfaces should be EXPORT_SYMBOL_GPL. That being said for this to be added an exported we'll need an actual user to start with anyway.
On Wed, Jul 17, 2024 at 08:16:29PM -0700, Christoph Hellwig wrote: > On Thu, Jul 18, 2024 at 12:24:01AM +0200, Danilo Krummrich wrote: > > +extern void * __must_check vrealloc_noprof(const void *p, size_t size, > > + gfp_t flags) __realloc_size(2); > > No need for the extern here. > > It would also help readability a bit to keep the __realloc_size on a > separate like, aka: > > void *__must_check vrealloc_noprof(const void *p, size_t size, gfp_t flags) > __realloc_size(2); Sure, will change that. > > > + if (size <= old_size) { > > + /* TODO: Can we optimize and shrink the allocation? What would > > + * be a good metric for when to shrink the vm_area? > > + */ > > Kernel comment style is to keep the > > /* > > on a line of it's own. I think it differs a bit throughout subsystems - will change. > A realloc that doesn't shrink is a bit pointless. Indeed - the idea was to mostly clean up kvrealloc() in this series first and implement proper shrinking and growing in a subsequent one. Does that make sense? > The same heuristics as for krealloc should probably apply > here, adjust to the fact that we always deal with whole pages. Sounds reasonable, but are there any? In __do_krealloc() if ks > new_size we only call into kasan_krealloc() to poison things and return the original pointer. Do I miss anything? > > > > + /* TODO: Can we optimize and extend the existing allocation if we have > > + * enough contiguous space left in the virtual address space? > > + */ > > I don't understand this comment. I meant to say that we could optimze by allocating additional pages and map them at the end of the existing allocation, if there is enough space in the VA space. If that doesn't work, we can still allocate additional pages and remap everything. > > > +EXPORT_SYMBOL(vrealloc_noprof); > > New interfaces should be EXPORT_SYMBOL_GPL. That being said for this > to be added an exported we'll need an actual user to start with anyway. Besides kvrealloc(), the Rust abstractions will be a user soon, but for that we probably don't need the export either. I will remove it for now.
On Thu, Jul 18, 2024 at 12:24:01AM +0200, Danilo Krummrich wrote: > Implement vrealloc() analogous to krealloc(). > > Currently, krealloc() requires the caller to pass the size of the > previous memory allocation, which, instead, should be self-contained. > > We attempt to fix this in a subsequent patch which, in order to do so, > requires vrealloc(). > > Besides that, we need realloc() functions for kernel allocators in Rust > too. With `Vec` or `KVec` respectively, potentially growing (and > shrinking) data structures are rather common. > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > --- > include/linux/vmalloc.h | 4 +++ > mm/vmalloc.c | 58 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 62 insertions(+) > > diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h > index e4a631ec430b..9ff0a8e5c323 100644 > --- a/include/linux/vmalloc.h > +++ b/include/linux/vmalloc.h > @@ -189,6 +189,10 @@ extern void *__vcalloc_noprof(size_t n, size_t size, gfp_t flags) __alloc_size(1 > extern void *vcalloc_noprof(size_t n, size_t size) __alloc_size(1, 2); > #define vcalloc(...) alloc_hooks(vcalloc_noprof(__VA_ARGS__)) > > +extern void * __must_check vrealloc_noprof(const void *p, size_t size, > + gfp_t flags) __realloc_size(2); > +#define vrealloc(...) alloc_hooks(vrealloc_noprof(__VA_ARGS__)) > + > extern void vfree(const void *addr); > extern void vfree_atomic(const void *addr); > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index e34ea860153f..4ec949ac9d9d 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -4036,6 +4036,64 @@ void *vzalloc_node_noprof(unsigned long size, int node) > } > EXPORT_SYMBOL(vzalloc_node_noprof); > > +/** > + * vrealloc - reallocate virtually contiguous memory; contents remain unchanged > + * @p: object to reallocate memory for > + * @size: the size to reallocate > + * @flags: the flags for the page level allocator > + * > + * The contents of the object pointed to are preserved up to the lesser of the > + * new and old size (__GFP_ZERO flag is effectively ignored). > + * > + * If @p is %NULL, vrealloc() behaves exactly like vmalloc(). If @size is 0 and > + * @p is not a %NULL pointer, the object pointed to is freed. > + * > + * Return: pointer to the allocated memory; %NULL if @size is zero or in case of > + * failure > + */ > +void *vrealloc_noprof(const void *p, size_t size, gfp_t flags) > +{ > + size_t old_size = 0; > + void *n; > + > + if (!size) { > + vfree(p); > + return NULL; > + } > + > + if (p) { > + struct vm_struct *vm; > + > + vm = find_vm_area(p); > Concurrent vfree() will lead to use-after-free. Either add a comment that a user is responsible for not using vrealloc()/vfree() on the same pointer concurrently or use find_unlink_vmap_area() which might be more complex when it comes to design of the vrealloc(). -- Uladzislau Rezki
On Tue, Jul 23, 2024 at 01:28:32PM +0200, Uladzislau Rezki wrote: > Concurrent vfree() will lead to use-after-free. Either add a comment > that a user is responsible for not using vrealloc()/vfree() on the same > pointer concurrently or use find_unlink_vmap_area() which might be more > complex when it comes to design of the vrealloc(). You can never use *free concurrently with *realloc. I guess it doesn't hurt to clearly document that, but other than that we should not try to cater to that use pattern at all.
On Tue, Jul 23, 2024 at 06:44:56AM -0700, Christoph Hellwig wrote: > On Tue, Jul 23, 2024 at 01:28:32PM +0200, Uladzislau Rezki wrote: > > Concurrent vfree() will lead to use-after-free. Either add a comment > > that a user is responsible for not using vrealloc()/vfree() on the same > > pointer concurrently or use find_unlink_vmap_area() which might be more > > complex when it comes to design of the vrealloc(). > > You can never use *free concurrently with *realloc. I guess it doesn't > hurt to clearly document that, but other than that we should not try > to cater to that use pattern at all. > Agree, i mentioned that as a first option. I think, it is enough to document that. Thanks! -- Uladzislau Rezki
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h index e4a631ec430b..9ff0a8e5c323 100644 --- a/include/linux/vmalloc.h +++ b/include/linux/vmalloc.h @@ -189,6 +189,10 @@ extern void *__vcalloc_noprof(size_t n, size_t size, gfp_t flags) __alloc_size(1 extern void *vcalloc_noprof(size_t n, size_t size) __alloc_size(1, 2); #define vcalloc(...) alloc_hooks(vcalloc_noprof(__VA_ARGS__)) +extern void * __must_check vrealloc_noprof(const void *p, size_t size, + gfp_t flags) __realloc_size(2); +#define vrealloc(...) alloc_hooks(vrealloc_noprof(__VA_ARGS__)) + extern void vfree(const void *addr); extern void vfree_atomic(const void *addr); diff --git a/mm/vmalloc.c b/mm/vmalloc.c index e34ea860153f..4ec949ac9d9d 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -4036,6 +4036,64 @@ void *vzalloc_node_noprof(unsigned long size, int node) } EXPORT_SYMBOL(vzalloc_node_noprof); +/** + * vrealloc - reallocate virtually contiguous memory; contents remain unchanged + * @p: object to reallocate memory for + * @size: the size to reallocate + * @flags: the flags for the page level allocator + * + * The contents of the object pointed to are preserved up to the lesser of the + * new and old size (__GFP_ZERO flag is effectively ignored). + * + * If @p is %NULL, vrealloc() behaves exactly like vmalloc(). If @size is 0 and + * @p is not a %NULL pointer, the object pointed to is freed. + * + * Return: pointer to the allocated memory; %NULL if @size is zero or in case of + * failure + */ +void *vrealloc_noprof(const void *p, size_t size, gfp_t flags) +{ + size_t old_size = 0; + void *n; + + if (!size) { + vfree(p); + return NULL; + } + + if (p) { + struct vm_struct *vm; + + vm = find_vm_area(p); + if (unlikely(!vm)) { + WARN(1, "Trying to vrealloc() nonexistent vm area (%p)\n", p); + return NULL; + } + + old_size = get_vm_area_size(vm); + } + + if (size <= old_size) { + /* TODO: Can we optimize and shrink the allocation? What would + * be a good metric for when to shrink the vm_area? + */ + return (void *)p; + } + + /* TODO: Can we optimize and extend the existing allocation if we have + * enough contiguous space left in the virtual address space? + */ + n = __vmalloc_noprof(size, flags); + + if (p) { + memcpy(n, p, old_size); + vfree(p); + } + + return n; +} +EXPORT_SYMBOL(vrealloc_noprof); + #if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32) #define GFP_VMALLOC32 (GFP_DMA32 | GFP_KERNEL) #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)
Implement vrealloc() analogous to krealloc(). Currently, krealloc() requires the caller to pass the size of the previous memory allocation, which, instead, should be self-contained. We attempt to fix this in a subsequent patch which, in order to do so, requires vrealloc(). Besides that, we need realloc() functions for kernel allocators in Rust too. With `Vec` or `KVec` respectively, potentially growing (and shrinking) data structures are rather common. Signed-off-by: Danilo Krummrich <dakr@kernel.org> --- include/linux/vmalloc.h | 4 +++ mm/vmalloc.c | 58 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+)