linux-kernel - Re: [PATCH v2 1/2] mm: vmalloc: implement vrealloc()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <07491799-9753-4fc9-b642-6d7d7d9575aa@suse.cz>
Date: Fri, 26 Jul 2024 16:37:43 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Danilo Krummrich <dakr@...nel.org>, cl@...ux.com, penberg@...nel.org,
 rientjes@...gle.com, iamjoonsoo.kim@....com, akpm@...ux-foundation.org,
 roman.gushchin@...ux.dev, 42.hyeyoo@...il.com, urezki@...il.com,
 hch@...radead.org, kees@...nel.org, ojeda@...nel.org, wedsonaf@...il.com,
 mhocko@...nel.org, mpe@...erman.id.au, chandan.babu@...cle.com,
 christian.koenig@....com, maz@...nel.org, oliver.upton@...ux.dev
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 1/2] mm: vmalloc: implement vrealloc()

On 7/22/24 6:29 PM, 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@...nel.org>

Acked-by: Vlastimil Babka <vbabka@...e.cz>

> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -4037,6 +4037,65 @@ 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).

Well, technically not correct as we don't shrink. Get 8 pages, kvrealloc to
4 pages, kvrealloc back to 8 and the last 4 are not zeroed. But it's not
new, kvrealloc() did the same before patch 2/2.

But it's also fundamentally not true for krealloc(), or kvrealloc()
switching from a kmalloc to valloc. ksize() returns the size of the kmalloc
bucket, we don't know what was the exact prior allocation size. Worse, we
started poisoning the padding in debug configurations, so even a
kmalloc(__GFP_ZERO) followed by krealloc(__GFP_ZERO) can give you unexpected
poison now...

I guess we should just document __GFP_ZERO is not honored at all for
realloc, and maybe start even warning :/ Hopefully nobody relies on that.

> + *
> + * 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: Shrink the vm_area, i.e. unmap and free unused pages.
> +		 * What would be a good heuristic for when to shrink the
> +		 * vm_area?
> +		 */
> +		return (void *)p;
> +	}
> +
> +	/* TODO: Grow the vm_area, i.e. allocate and map additional pages. */
> +	n = __vmalloc_noprof(size, flags);
> +	if (!n)
> +		return NULL;
> +
> +	if (p) {
> +		memcpy(n, p, old_size);
> +		vfree(p);
> +	}
> +
> +	return n;
> +}
> +
>  #if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32)
>  #define GFP_VMALLOC32 (GFP_DMA32 | GFP_KERNEL)
>  #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)