linux-kernel - Re: [PATCH v2 2/2] mm: kvmalloc: align kvrealloc() with krealloc()

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: <Zp-xLOa7NFOt4r1V@pollux>
Date: Tue, 23 Jul 2024 15:33:32 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Michal Hocko <mhocko@...e.com>
Cc: cl@...ux.com, penberg@...nel.org, rientjes@...gle.com,
	iamjoonsoo.kim@....com, akpm@...ux-foundation.org, vbabka@...e.cz,
	roman.gushchin@...ux.dev, 42.hyeyoo@...il.com, urezki@...il.com,
	hch@...radead.org, kees@...nel.org, ojeda@...nel.org,
	wedsonaf@...il.com, mpe@...erman.id.au, chandan.babu@...cle.com,
	christian.koenig@....com, maz@...nel.org, oliver.upton@...ux.dev,
	linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 2/2] mm: kvmalloc: align kvrealloc() with krealloc()

On Tue, Jul 23, 2024 at 02:12:23PM +0200, Michal Hocko wrote:
> On Tue 23-07-24 13:55:48, Danilo Krummrich wrote:
> > On Tue, Jul 23, 2024 at 12:55:45PM +0200, Michal Hocko wrote:
> > > On Tue 23-07-24 12:42:17, Danilo Krummrich wrote:
> > > > On Tue, Jul 23, 2024 at 09:50:13AM +0200, Michal Hocko wrote:
> > > > > On Mon 22-07-24 18:29:24, Danilo Krummrich wrote:
> > > [...]
> > > > > > Besides that, implementing kvrealloc() by making use of krealloc() and
> > > > > > vrealloc() provides oppertunities to grow (and shrink) allocations more
> > > > > > efficiently. For instance, vrealloc() can be optimized to allocate and
> > > > > > map additional pages to grow the allocation or unmap and free unused
> > > > > > pages to shrink the allocation.
> > > > > 
> > > > > This seems like a change that is independent on the above and should be
> > > > > a patch on its own.
> > > > 
> > > > The optimizations you mean? Yes, I intend to do this in a separate series. For
> > > > now, I put TODOs in vrealloc.
> > > 
> > > No I mean, that the change of the signature and semantic should be done along with
> > > update to callers and the new implementation of the function itself
> > > should be done in its own patch.
> > 
> > Sorry, it seems like you lost me a bit.
> > 
> > There is one patch that implements vrealloc() and one patch that does the change
> > of krealloc()'s signature, semantics and the corresponding update to the
> > callers.
> > 
> > Isn't that already what you ask for?
> 
> No, because this second patch reimplements kvrealloc wo to use krealloc
> and vrealloc fallback. More clear now?

I'm very sorry, but no. The second patch just changes kvrealloc(), how do you
want to split it up?

> > > > > > +	if (is_vmalloc_addr(p))
> > > > > > +		return vrealloc_noprof(p, size, flags);
> > > > > > +
> > > > > > +	n = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));
> > > > > > +	if (!n) {
> > > > > > +		/* We failed to krealloc(), fall back to kvmalloc(). */
> > > > > > +		n = kvmalloc_noprof(size, flags);
> > > > > 
> > > > > Why don't you simply use vrealloc_noprof here?
> > > > 
> > > > We could do that, but we'd also need to do the same checks kvmalloc() does, i.e.
> > > > 
> > > > 	/*
> > > > 	 * It doesn't really make sense to fallback to vmalloc for sub page
> > > > 	 * requests
> > > > 	 */
> > > > 	if (ret || size <= PAGE_SIZE)
> > > > 		return ret;
> > > 
> > > With the early !size && p check we wouldn't right?
> > 
> > I think that's unrelated. Your proposed early check checks for size == 0 to free
> > and return early. Whereas this check bails out if we fail kmalloc() with
> > size <= PAGE_SIZE, because a subsequent vmalloc() wouldn't make a lot of sense.
> 
> It seems we are not on the same page here. Here is what I would like
> kvrealloc to look like in the end:
> 
> void *kvrealloc_noprof(const void *p, size_t size, gfp_t flags)
> {
>         void *newp;
> 
>         if (!size && p) {
>                 kvfree(p);
>                 return NULL;
>         }
> 
>         if (!is_vmalloc_addr(p))
>                 newp = krealloc_noprof(p, size, kmalloc_gfp_adjust(flags, size));
> 
>         if (newp)
>                 return newp;
> 
>         return vrealloc_noprof(p, size, flags);
> }
> EXPORT_SYMBOL(kvrealloc_noprof);

This looks weird. The fact that you're passing p to vrealloc_noprof() if
krealloc_noprof() fails, implies that vrealloc_noprof() must be able to deal
with pointers to kmalloc'd memory.

Also, you never migrate from kmalloc memory to vmalloc memory and never free p.
Given the above, do you mean to say that vrealloc_noprof() should do all that?

If so, I strongly disagree here. vrealloc() should only deal with vmalloc
memory.

> 
> krealloc_noprof should be extended for the maximum allowed size

krealloc_noprof() already has a maximum allowed size.

> and so does vrealloc_noprof.

Probably, but I don't think this series is the correct scope for this change.
I'd offer to send a separate patch for this though.

> The implementation of the kvrealloc cannot get any
> easier and more straightforward AFAICS. See my point?
> -- 
> Michal Hocko
> SUSE Labs
>