RE: Subject: [PATCH v2] slab: kmalloc_size_roundup() must not return 0 for non-zero size

[David Laight]

From: Vlastimil Babka
> Sent: 11 September 2023 17:23
> 
> On 9/11/23 18:12, David Laight wrote:
> > From: Vlastimil Babka
> >> Sent: 11 September 2023 16:54
> >>
> >> On 9/8/23 10:26, David Laight wrote:
> >> > From: Kees Cook
> >> >> Sent: 07 September 2023 20:38
> >> >>
> >> >> On Thu, Sep 07, 2023 at 12:42:20PM +0000, David Laight wrote:
> >> >> > The typical use of kmalloc_size_roundup() is:
> >> >> > 	ptr = kmalloc(sz = kmalloc_size_roundup(size), ...);
> >> >> > 	if (!ptr) return -ENOMEM.
...
> >> >> > +		c = kmalloc_slab(size, GFP_KERNEL, 0);
> >> >> > +		return likely(c) ? c->object_size : size;
> >> >>
> >> >> I would like to have this fail "safe". c should never be NULL here, so
> >> >> let's return "KMALLOC_MAX_SIZE + 1" to force failures.
> >> >
> >> > Why even try to force failure here?
> >> > The whole function is just an optimisation so that the caller
> >> > can use the spare space.
> >> >
> >> > The only thing it mustn't do is return a smaller value.
> >>
> >> If "c" is NULL it means either the kernel build must be broken e.g. by
> >> somebody breaking the KMALLOC_MAX_CACHE_SIZE value, and we could just ignore
> >> c being NULL and let it crash because of that.
> >> But I think it can also be NULL due to trying to call kmalloc_size_roundup()
> >> too early, when kmalloc_caches array is not yet populated. Note if we call
> >> kmalloc() itself too early, we get a NULL as a result, AFAICS. I can imagine
> >> two scenarios:
> >>
> >> - kmalloc_size_roundup() is called with result immediately fed to kmalloc()
> >> that happens too early, in that case we best should not crash on c being
> >> NULL and make sure the kmalloc() returns NULL.
> >> - kmalloc_size_roundup() is called in some init code to get a value that
> >> some later kmalloc() call uses. We might want also not crash in that case,
> >> but informing the developer that they did something wrong would be also useful?
> >>
> >> Clearly returning 0 if c == NULL, as done currently, is wrong for both
> >> scenarios. Retuning "size" is OK for the first scenario, also valid for the
> >> second one, but the caller will silently lose the benefit of
> >> kmalloc_size_roundup() and the developer introducing that won't realize it's
> >> done too early and could be fixed.
> >
> > I'm sure that won't matter.
> 
> For the performance, sure. It just feels silly to me to have a code that
> looks like it does something, but silently doesn't. Leads to cargo cult
> copying it to other places etc.
> 
> >> So perhaps the best would be to return size for c == NULL, but also do a
> >> WARN_ONCE?
> >
> > That would add a real function call to an otherwise leaf function
> > and almost certainly require the compiler create a stack frame.
> 
> Hm I thought WARN is done by tripping on undefined instruction like BUG
> these days. Also any code that accepts the call to kmalloc_size_roundup
> probably could accept that too.

It's probably just worth removing the c == NULL check and
assuming there won't be any fallout.
The NULL pointer deref is an easy to debug as anything else.

If it gets called in any early init code it'll soon show up.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)