| Message ID | 4d31a2bf7eb544749023cf491c0eccc8@AcuMS.aculab.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Subject: [PATCH v2] slab: kmalloc_size_roundup() must not return 0 for non-zero size | expand |
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. > This means it is vitally important that the returned value isn't > less than the argument even if the argument is insane. > In particular if kmalloc_slab() fails or the value is above > (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return > it's single zero-length buffer. > > Fix by returning the input size on error or if the size exceeds > a 'sanity' limit. > kmalloc() will then return NULL is the size really is too big. > > > Signed-off-by: David Laight <david.laight@aculab.com> > Fixes: 05a940656e1eb ("slab: Introduce kmalloc_size_roundup()") > --- > v2: > - Use KMALLOC_MAX_SIZE for upper limit. > (KMALLOC_MAX_SIZE + 1 may give better code on some archs!) > - Invert test for overlarge for consistency. > - Put a likely() on result of kmalloc_slab(). > > mm/slab_common.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/mm/slab_common.c b/mm/slab_common.c > index cd71f9581e67..0fb7c7e19bad 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -747,22 +747,22 @@ size_t kmalloc_size_roundup(size_t size) > { > struct kmem_cache *c; > > - /* Short-circuit the 0 size case. */ > - if (unlikely(size == 0)) > - return 0; If we want to allow 0, let's just leave this case as-is: the compiler will optimize it against the other tests. > - /* Short-circuit saturated "too-large" case. */ > - if (unlikely(size == SIZE_MAX)) > - return SIZE_MAX; > + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { > + /* > + * The flags don't matter since size_index is common to all. > + * Neither does the caller for just getting ->object_size. > + */ > + 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. > + } > + > /* Above the smaller buckets, size is a multiple of page size. */ > - if (size > KMALLOC_MAX_CACHE_SIZE) > + if (size && size <= KMALLOC_MAX_SIZE) > return PAGE_SIZE << get_order(size); > > - /* > - * The flags don't matter since size_index is common to all. > - * Neither does the caller for just getting ->object_size. > - */ > - c = kmalloc_slab(size, GFP_KERNEL, 0); > - return c ? c->object_size : 0; > + /* Return 'size' for 0 and very large - kmalloc() may fail. */ I want to _be certain_ failure happens. If we get here we need to return "KMALLOC_MAX_SIZE + 1" -Kees > + return size; > + > } > EXPORT_SYMBOL(kmalloc_size_roundup); > > -- > 2.17.1 > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
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. > > This means it is vitally important that the returned value isn't > > less than the argument even if the argument is insane. > > In particular if kmalloc_slab() fails or the value is above > > (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return > > it's single zero-length buffer. > > > > Fix by returning the input size on error or if the size exceeds > > a 'sanity' limit. > > kmalloc() will then return NULL is the size really is too big. > > > > > > Signed-off-by: David Laight <david.laight@aculab.com> > > Fixes: 05a940656e1eb ("slab: Introduce kmalloc_size_roundup()") > > --- > > v2: > > - Use KMALLOC_MAX_SIZE for upper limit. > > (KMALLOC_MAX_SIZE + 1 may give better code on some archs!) > > - Invert test for overlarge for consistency. > > - Put a likely() on result of kmalloc_slab(). > > > > mm/slab_common.c | 26 +++++++++++++------------- > > 1 file changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/mm/slab_common.c b/mm/slab_common.c > > index cd71f9581e67..0fb7c7e19bad 100644 > > --- a/mm/slab_common.c > > +++ b/mm/slab_common.c > > @@ -747,22 +747,22 @@ size_t kmalloc_size_roundup(size_t size) > > { > > struct kmem_cache *c; > > > > - /* Short-circuit the 0 size case. */ > > - if (unlikely(size == 0)) > > - return 0; > > If we want to allow 0, let's just leave this case as-is: the compiler > will optimize it against the other tests. I doubt the compiler will optimise it away - especially with the unlikely(). OTOH the explicit checks for (size && size <= LIMIT) do get optimised to ((size - 1) <= LIMIT - 1) so become a single compare. Then returning 'size' at the bottom means that zero is returned in the arg is zero - which is fine. > > > - /* Short-circuit saturated "too-large" case. */ > > - if (unlikely(size == SIZE_MAX)) > > - return SIZE_MAX; > > + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { > > + /* > > + * The flags don't matter since size_index is common to all. > > + * Neither does the caller for just getting ->object_size. > > + */ > > + 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. > > > + } > > + > > /* Above the smaller buckets, size is a multiple of page size. */ > > - if (size > KMALLOC_MAX_CACHE_SIZE) > > + if (size && size <= KMALLOC_MAX_SIZE) > > return PAGE_SIZE << get_order(size); > > > > - /* > > - * The flags don't matter since size_index is common to all. > > - * Neither does the caller for just getting ->object_size. > > - */ > > - c = kmalloc_slab(size, GFP_KERNEL, 0); > > - return c ? c->object_size : 0; > > + /* Return 'size' for 0 and very large - kmalloc() may fail. */ > > I want to _be certain_ failure happens. If we get here we need to return > "KMALLOC_MAX_SIZE + 1" Why care? David > > -Kees > > > + return size; > > + > > } > > EXPORT_SYMBOL(kmalloc_size_roundup); > > > > -- > > 2.17.1 > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales) > > > > -- > Kees Cook - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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. >> > This means it is vitally important that the returned value isn't >> > less than the argument even if the argument is insane. >> > In particular if kmalloc_slab() fails or the value is above >> > (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return >> > it's single zero-length buffer. >> > >> > Fix by returning the input size on error or if the size exceeds >> > a 'sanity' limit. >> > kmalloc() will then return NULL is the size really is too big. >> > >> > >> > Signed-off-by: David Laight <david.laight@aculab.com> >> > Fixes: 05a940656e1eb ("slab: Introduce kmalloc_size_roundup()") >> > --- >> > v2: >> > - Use KMALLOC_MAX_SIZE for upper limit. >> > (KMALLOC_MAX_SIZE + 1 may give better code on some archs!) >> > - Invert test for overlarge for consistency. >> > - Put a likely() on result of kmalloc_slab(). >> > >> > mm/slab_common.c | 26 +++++++++++++------------- >> > 1 file changed, 13 insertions(+), 13 deletions(-) >> > >> > diff --git a/mm/slab_common.c b/mm/slab_common.c >> > index cd71f9581e67..0fb7c7e19bad 100644 >> > --- a/mm/slab_common.c >> > +++ b/mm/slab_common.c >> > @@ -747,22 +747,22 @@ size_t kmalloc_size_roundup(size_t size) >> > { >> > struct kmem_cache *c; >> > >> > - /* Short-circuit the 0 size case. */ >> > - if (unlikely(size == 0)) >> > - return 0; >> >> If we want to allow 0, let's just leave this case as-is: the compiler >> will optimize it against the other tests. > > I doubt the compiler will optimise it away - especially with > the unlikely(). Yeah I also think compiler can't do much optimizations except for build-time constant 0 here. > OTOH the explicit checks for (size && size <= LIMIT) do > get optimised to ((size - 1) <= LIMIT - 1) so become > a single compare. > > Then returning 'size' at the bottom means that zero is returned > in the arg is zero - which is fine. > >> >> > - /* Short-circuit saturated "too-large" case. */ >> > - if (unlikely(size == SIZE_MAX)) >> > - return SIZE_MAX; >> > + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { >> > + /* >> > + * The flags don't matter since size_index is common to all. >> > + * Neither does the caller for just getting ->object_size. >> > + */ >> > + 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. So perhaps the best would be to return size for c == NULL, but also do a WARN_ONCE? >> >> > + } >> > + >> > /* Above the smaller buckets, size is a multiple of page size. */ >> > - if (size > KMALLOC_MAX_CACHE_SIZE) >> > + if (size && size <= KMALLOC_MAX_SIZE) >> > return PAGE_SIZE << get_order(size); >> > >> > - /* >> > - * The flags don't matter since size_index is common to all. >> > - * Neither does the caller for just getting ->object_size. >> > - */ >> > - c = kmalloc_slab(size, GFP_KERNEL, 0); >> > - return c ? c->object_size : 0; >> > + /* Return 'size' for 0 and very large - kmalloc() may fail. */ >> >> I want to _be certain_ failure happens. If we get here we need to return >> "KMALLOC_MAX_SIZE + 1" > > Why care? Yeah no need for that. > > David > >> >> -Kees >> >> > + return size; >> > + >> > } >> > EXPORT_SYMBOL(kmalloc_size_roundup); >> > >> > -- >> > 2.17.1 >> > >> > - >> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK >> > Registration No: 1397386 (Wales) >> > >> >> -- >> Kees Cook > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
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. > >> > This means it is vitally important that the returned value isn't > >> > less than the argument even if the argument is insane. > >> > In particular if kmalloc_slab() fails or the value is above > >> > (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return > >> > it's single zero-length buffer. > >> > > >> > Fix by returning the input size on error or if the size exceeds > >> > a 'sanity' limit. > >> > kmalloc() will then return NULL is the size really is too big. > >> > > >> > > >> > Signed-off-by: David Laight <david.laight@aculab.com> > >> > Fixes: 05a940656e1eb ("slab: Introduce kmalloc_size_roundup()") > >> > --- > >> > v2: > >> > - Use KMALLOC_MAX_SIZE for upper limit. > >> > (KMALLOC_MAX_SIZE + 1 may give better code on some archs!) > >> > - Invert test for overlarge for consistency. > >> > - Put a likely() on result of kmalloc_slab(). > >> > > >> > mm/slab_common.c | 26 +++++++++++++------------- > >> > 1 file changed, 13 insertions(+), 13 deletions(-) > >> > > >> > diff --git a/mm/slab_common.c b/mm/slab_common.c > >> > index cd71f9581e67..0fb7c7e19bad 100644 > >> > --- a/mm/slab_common.c > >> > +++ b/mm/slab_common.c > >> > @@ -747,22 +747,22 @@ size_t kmalloc_size_roundup(size_t size) > >> > { > >> > struct kmem_cache *c; > >> > > >> > - /* Short-circuit the 0 size case. */ > >> > - if (unlikely(size == 0)) > >> > - return 0; > >> > >> If we want to allow 0, let's just leave this case as-is: the compiler > >> will optimize it against the other tests. > > > > I doubt the compiler will optimise it away - especially with > > the unlikely(). > > Yeah I also think compiler can't do much optimizations except for build-time > constant 0 here. Only relevant if the code were inlined - and it isn't. (and is probably a bit big.) I'm not sure you'd want to expose kmalloc_slab() to the wider kernel. OTOH, it could have an inline version for constants > KMALLOC_CACHE_SIZE. But they may not happen often enough to make any difference. > > > OTOH the explicit checks for (size && size <= LIMIT) do > > get optimised to ((size - 1) <= LIMIT - 1) so become > > a single compare. > > > > Then returning 'size' at the bottom means that zero is returned > > in the arg is zero - which is fine. > > > >> > >> > - /* Short-circuit saturated "too-large" case. */ > >> > - if (unlikely(size == SIZE_MAX)) > >> > - return SIZE_MAX; > >> > + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { > >> > + /* > >> > + * The flags don't matter since size_index is common to all. > >> > + * Neither does the caller for just getting ->object_size. > >> > + */ > >> > + 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. > 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. ... I did have an interesting 'lateral thought' idea. It is all very silly doing all the work twice, what you really want is kmalloc() to return both the pointer and actual size. But returning a 'two word' structure is done by reference and would kill performance/ OTOH a lot of archs can return two word integers in a register pair (dx:ax on x86). Could you have the real function return ((unsigned __int64)size << 64 | (long)ptr) and then extract the size in a wrapper macro? (With different types for 32bit) That will, of course, break the 'it's like malloc' checks the compiler is doing - unless it is taught what is going on. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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. >> >> > This means it is vitally important that the returned value isn't >> >> > less than the argument even if the argument is insane. >> >> > In particular if kmalloc_slab() fails or the value is above >> >> > (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return >> >> > it's single zero-length buffer. >> >> > >> >> > Fix by returning the input size on error or if the size exceeds >> >> > a 'sanity' limit. >> >> > kmalloc() will then return NULL is the size really is too big. >> >> > >> >> > >> >> > Signed-off-by: David Laight <david.laight@aculab.com> >> >> > Fixes: 05a940656e1eb ("slab: Introduce kmalloc_size_roundup()") >> >> > --- >> >> > v2: >> >> > - Use KMALLOC_MAX_SIZE for upper limit. >> >> > (KMALLOC_MAX_SIZE + 1 may give better code on some archs!) >> >> > - Invert test for overlarge for consistency. >> >> > - Put a likely() on result of kmalloc_slab(). >> >> > >> >> > mm/slab_common.c | 26 +++++++++++++------------- >> >> > 1 file changed, 13 insertions(+), 13 deletions(-) >> >> > >> >> > diff --git a/mm/slab_common.c b/mm/slab_common.c >> >> > index cd71f9581e67..0fb7c7e19bad 100644 >> >> > --- a/mm/slab_common.c >> >> > +++ b/mm/slab_common.c >> >> > @@ -747,22 +747,22 @@ size_t kmalloc_size_roundup(size_t size) >> >> > { >> >> > struct kmem_cache *c; >> >> > >> >> > - /* Short-circuit the 0 size case. */ >> >> > - if (unlikely(size == 0)) >> >> > - return 0; >> >> >> >> If we want to allow 0, let's just leave this case as-is: the compiler >> >> will optimize it against the other tests. >> > >> > I doubt the compiler will optimise it away - especially with >> > the unlikely(). >> >> Yeah I also think compiler can't do much optimizations except for build-time >> constant 0 here. > > Only relevant if the code were inlined - and it isn't. Aha, I thought it was, good point. > (and is probably a bit big.) > I'm not sure you'd want to expose kmalloc_slab() to the wider kernel. No, let's keep it that way. > OTOH, it could have an inline version for constants > KMALLOC_CACHE_SIZE. > But they may not happen often enough to make any difference. Yeah, unnecessary. >> >> > OTOH the explicit checks for (size && size <= LIMIT) do >> > get optimised to ((size - 1) <= LIMIT - 1) so become >> > a single compare. >> > >> > Then returning 'size' at the bottom means that zero is returned >> > in the arg is zero - which is fine. >> > >> >> >> >> > - /* Short-circuit saturated "too-large" case. */ >> >> > - if (unlikely(size == SIZE_MAX)) >> >> > - return SIZE_MAX; >> >> > + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { >> >> > + /* >> >> > + * The flags don't matter since size_index is common to all. >> >> > + * Neither does the caller for just getting ->object_size. >> >> > + */ >> >> > + 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. > > ... > > I did have an interesting 'lateral thought' idea. > It is all very silly doing all the work twice, what you really > want is kmalloc() to return both the pointer and actual size. > But returning a 'two word' structure is done by reference and > would kill performance/ > OTOH a lot of archs can return two word integers in a register pair > (dx:ax on x86). > Could you have the real function return ((unsigned __int64)size << 64 | (long)ptr) > and then extract the size in a wrapper macro? > (With different types for 32bit) > > That will, of course, break the 'it's like malloc' checks the > compiler is doing - unless it is taught what is going on. Probably this is something not worth all the trouble. > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
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)
On 9/11/23 18:38, David Laight wrote: >> >> 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. Good point, early crash should be ok. So how about this with my tweaks, looks ok? I'll put it in -next and send with another hotfix to 6.6 next week. ----8<---- From f5de1ee7b35d7ab35c21c79dd13cea49fbe239b7 Mon Sep 17 00:00:00 2001 From: David Laight <david.laight@aculab.com> Date: Thu, 7 Sep 2023 12:42:20 +0000 Subject: [PATCH] Subject: [PATCH v2] slab: kmalloc_size_roundup() must not return 0 for non-zero size The typical use of kmalloc_size_roundup() is: ptr = kmalloc(sz = kmalloc_size_roundup(size), ...); if (!ptr) return -ENOMEM. This means it is vitally important that the returned value isn't less than the argument even if the argument is insane. In particular if kmalloc_slab() fails or the value is above (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return its single zero-length buffer ZERO_SIZE_PTR. Fix this by returning the input size if the size exceeds KMALLOC_MAX_SIZE. kmalloc() will then return NULL as the size really is too big. kmalloc_slab() should not normally return NULL, unless called too early. Again, returning zero is not the correct action as it can be in some usage scenarios stored to a variable and only later cause kmalloc() return ZERO_SIZE_PTR and subsequent crashes on access. Instead we can simply stop checking the kmalloc_slab() result completely, as calling kmalloc_size_roundup() too early would then result in an immediate crash during boot and the developer noticing an issue in their code. [vbabka@suse.cz: remove kmalloc_slab() result check, tweak comments and commit log] Signed-off-by: David Laight <david.laight@aculab.com> Fixes: 05a940656e1e ("slab: Introduce kmalloc_size_roundup()") Signed-off-by: Vlastimil Babka <vbabka@suse.cz> --- mm/slab_common.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/mm/slab_common.c b/mm/slab_common.c index e99e821065c3..1dc108224bd1 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -747,22 +747,25 @@ size_t kmalloc_size_roundup(size_t size) { struct kmem_cache *c; - /* Short-circuit the 0 size case. */ - if (unlikely(size == 0)) - return 0; - /* Short-circuit saturated "too-large" case. */ - if (unlikely(size == SIZE_MAX)) - return SIZE_MAX; + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { + /* + * The flags don't matter since size_index is common to all. + * Neither does the caller for just getting ->object_size. + */ + c = kmalloc_slab(size, GFP_KERNEL, 0); + return c->object_size; + } + /* Above the smaller buckets, size is a multiple of page size. */ - if (size > KMALLOC_MAX_CACHE_SIZE) + if (size && size <= KMALLOC_MAX_SIZE) return PAGE_SIZE << get_order(size); /* - * The flags don't matter since size_index is common to all. - * Neither does the caller for just getting ->object_size. + * Return 'size' for 0 - kmalloc() returns ZERO_SIZE_PTR + * and very large size - kmalloc() may fail. */ - c = kmalloc_slab(size, GFP_KERNEL, 0); - return c ? c->object_size : 0; + return size; + } EXPORT_SYMBOL(kmalloc_size_roundup);
From: Vlastimil Babka > Sent: 20 September 2023 10:58 > > On 9/11/23 18:38, David Laight wrote: > >> >> 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. > > Good point, early crash should be ok. > So how about this with my tweaks, looks ok? Is that just/mainly the change to assume that kmalloc_slab() doesn't fail? You can also remove 'c'. return kmalloc_slab(size, GFP_KERNEL, 0)->object_size; isn't too long. I also did a grep for callers. Nothing in early code, IIRC mainly 'net'. David > I'll put it in -next and > send with another hotfix to 6.6 next week. > ----8<---- > From f5de1ee7b35d7ab35c21c79dd13cea49fbe239b7 Mon Sep 17 00:00:00 2001 > From: David Laight <david.laight@aculab.com> > Date: Thu, 7 Sep 2023 12:42:20 +0000 > Subject: [PATCH] Subject: [PATCH v2] slab: kmalloc_size_roundup() must not > return 0 for non-zero size > > The typical use of kmalloc_size_roundup() is: > > ptr = kmalloc(sz = kmalloc_size_roundup(size), ...); > if (!ptr) return -ENOMEM. > > This means it is vitally important that the returned value isn't less > than the argument even if the argument is insane. > In particular if kmalloc_slab() fails or the value is above > (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return > its single zero-length buffer ZERO_SIZE_PTR. > > Fix this by returning the input size if the size exceeds > KMALLOC_MAX_SIZE. kmalloc() will then return NULL as the size really is > too big. > > kmalloc_slab() should not normally return NULL, unless called too early. > Again, returning zero is not the correct action as it can be in some > usage scenarios stored to a variable and only later cause kmalloc() > return ZERO_SIZE_PTR and subsequent crashes on access. Instead we can > simply stop checking the kmalloc_slab() result completely, as calling > kmalloc_size_roundup() too early would then result in an immediate crash > during boot and the developer noticing an issue in their code. > > [vbabka@suse.cz: remove kmalloc_slab() result check, tweak comments and > commit log] > Signed-off-by: David Laight <david.laight@aculab.com> > Fixes: 05a940656e1e ("slab: Introduce kmalloc_size_roundup()") > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/slab_common.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/mm/slab_common.c b/mm/slab_common.c > index e99e821065c3..1dc108224bd1 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -747,22 +747,25 @@ size_t kmalloc_size_roundup(size_t size) > { > struct kmem_cache *c; > > - /* Short-circuit the 0 size case. */ > - if (unlikely(size == 0)) > - return 0; > - /* Short-circuit saturated "too-large" case. */ > - if (unlikely(size == SIZE_MAX)) > - return SIZE_MAX; > + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { > + /* > + * The flags don't matter since size_index is common to all. > + * Neither does the caller for just getting ->object_size. > + */ > + c = kmalloc_slab(size, GFP_KERNEL, 0); > + return c->object_size; > + } > + > /* Above the smaller buckets, size is a multiple of page size. */ > - if (size > KMALLOC_MAX_CACHE_SIZE) > + if (size && size <= KMALLOC_MAX_SIZE) > return PAGE_SIZE << get_order(size); > > /* > - * The flags don't matter since size_index is common to all. > - * Neither does the caller for just getting ->object_size. > + * Return 'size' for 0 - kmalloc() returns ZERO_SIZE_PTR > + * and very large size - kmalloc() may fail. > */ > - c = kmalloc_slab(size, GFP_KERNEL, 0); > - return c ? c->object_size : 0; > + return size; > + > } > EXPORT_SYMBOL(kmalloc_size_roundup); > > -- > 2.42.0 > > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 9/20/23 12:06, David Laight wrote: > From: Vlastimil Babka >> Sent: 20 September 2023 10:58 >> >> On 9/11/23 18:38, David Laight wrote: >> >> >> 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. >> >> Good point, early crash should be ok. >> So how about this with my tweaks, looks ok? > > Is that just/mainly the change to assume that kmalloc_slab() doesn't fail? Yes. > You can also remove 'c'. > return kmalloc_slab(size, GFP_KERNEL, 0)->object_size; > isn't too long. Right, did that and pushed to -next. Thanks! > I also did a grep for callers. > Nothing in early code, IIRC mainly 'net'. > > David > >> I'll put it in -next and >> send with another hotfix to 6.6 next week. >> ----8<---- >> From f5de1ee7b35d7ab35c21c79dd13cea49fbe239b7 Mon Sep 17 00:00:00 2001 >> From: David Laight <david.laight@aculab.com> >> Date: Thu, 7 Sep 2023 12:42:20 +0000 >> Subject: [PATCH] Subject: [PATCH v2] slab: kmalloc_size_roundup() must not >> return 0 for non-zero size >> >> The typical use of kmalloc_size_roundup() is: >> >> ptr = kmalloc(sz = kmalloc_size_roundup(size), ...); >> if (!ptr) return -ENOMEM. >> >> This means it is vitally important that the returned value isn't less >> than the argument even if the argument is insane. >> In particular if kmalloc_slab() fails or the value is above >> (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return >> its single zero-length buffer ZERO_SIZE_PTR. >> >> Fix this by returning the input size if the size exceeds >> KMALLOC_MAX_SIZE. kmalloc() will then return NULL as the size really is >> too big. >> >> kmalloc_slab() should not normally return NULL, unless called too early. >> Again, returning zero is not the correct action as it can be in some >> usage scenarios stored to a variable and only later cause kmalloc() >> return ZERO_SIZE_PTR and subsequent crashes on access. Instead we can >> simply stop checking the kmalloc_slab() result completely, as calling >> kmalloc_size_roundup() too early would then result in an immediate crash >> during boot and the developer noticing an issue in their code. >> >> [vbabka@suse.cz: remove kmalloc_slab() result check, tweak comments and >> commit log] >> Signed-off-by: David Laight <david.laight@aculab.com> >> Fixes: 05a940656e1e ("slab: Introduce kmalloc_size_roundup()") >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >> --- >> mm/slab_common.c | 25 ++++++++++++++----------- >> 1 file changed, 14 insertions(+), 11 deletions(-) >> >> diff --git a/mm/slab_common.c b/mm/slab_common.c >> index e99e821065c3..1dc108224bd1 100644 >> --- a/mm/slab_common.c >> +++ b/mm/slab_common.c >> @@ -747,22 +747,25 @@ size_t kmalloc_size_roundup(size_t size) >> { >> struct kmem_cache *c; >> >> - /* Short-circuit the 0 size case. */ >> - if (unlikely(size == 0)) >> - return 0; >> - /* Short-circuit saturated "too-large" case. */ >> - if (unlikely(size == SIZE_MAX)) >> - return SIZE_MAX; >> + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { >> + /* >> + * The flags don't matter since size_index is common to all. >> + * Neither does the caller for just getting ->object_size. >> + */ >> + c = kmalloc_slab(size, GFP_KERNEL, 0); >> + return c->object_size; >> + } >> + >> /* Above the smaller buckets, size is a multiple of page size. */ >> - if (size > KMALLOC_MAX_CACHE_SIZE) >> + if (size && size <= KMALLOC_MAX_SIZE) >> return PAGE_SIZE << get_order(size); >> >> /* >> - * The flags don't matter since size_index is common to all. >> - * Neither does the caller for just getting ->object_size. >> + * Return 'size' for 0 - kmalloc() returns ZERO_SIZE_PTR >> + * and very large size - kmalloc() may fail. >> */ >> - c = kmalloc_slab(size, GFP_KERNEL, 0); >> - return c ? c->object_size : 0; >> + return size; >> + >> } >> EXPORT_SYMBOL(kmalloc_size_roundup); >> >> -- >> 2.42.0 >> >> > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
On Wed, Sep 20, 2023 at 9:48 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 9/20/23 12:06, David Laight wrote: > > From: Vlastimil Babka > >> Sent: 20 September 2023 10:58 > >> > >> On 9/11/23 18:38, David Laight wrote: > >> >> >> 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. > >> > >> Good point, early crash should be ok. > >> So how about this with my tweaks, looks ok? > > > > Is that just/mainly the change to assume that kmalloc_slab() doesn't fail? > > Yes. > > > You can also remove 'c'. > > return kmalloc_slab(size, GFP_KERNEL, 0)->object_size; > > isn't too long. > > Right, did that and pushed to -next. Thanks! A bit late, but for the record: Looks good to me, Reviewed-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > > I also did a grep for callers. > > Nothing in early code, IIRC mainly 'net'. > > > > David > > > >> I'll put it in -next and > >> send with another hotfix to 6.6 next week. > >> ----8<---- > >> From f5de1ee7b35d7ab35c21c79dd13cea49fbe239b7 Mon Sep 17 00:00:00 2001 > >> From: David Laight <david.laight@aculab.com> > >> Date: Thu, 7 Sep 2023 12:42:20 +0000 > >> Subject: [PATCH] Subject: [PATCH v2] slab: kmalloc_size_roundup() must not > >> return 0 for non-zero size > >> > >> The typical use of kmalloc_size_roundup() is: > >> > >> ptr = kmalloc(sz = kmalloc_size_roundup(size), ...); > >> if (!ptr) return -ENOMEM. > >> > >> This means it is vitally important that the returned value isn't less > >> than the argument even if the argument is insane. > >> In particular if kmalloc_slab() fails or the value is above > >> (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return > >> its single zero-length buffer ZERO_SIZE_PTR. > >> > >> Fix this by returning the input size if the size exceeds > >> KMALLOC_MAX_SIZE. kmalloc() will then return NULL as the size really is > >> too big. > >> > >> kmalloc_slab() should not normally return NULL, unless called too early. > >> Again, returning zero is not the correct action as it can be in some > >> usage scenarios stored to a variable and only later cause kmalloc() > >> return ZERO_SIZE_PTR and subsequent crashes on access. Instead we can > >> simply stop checking the kmalloc_slab() result completely, as calling > >> kmalloc_size_roundup() too early would then result in an immediate crash > >> during boot and the developer noticing an issue in their code. > >> > >> [vbabka@suse.cz: remove kmalloc_slab() result check, tweak comments and > >> commit log] > >> Signed-off-by: David Laight <david.laight@aculab.com> > >> Fixes: 05a940656e1e ("slab: Introduce kmalloc_size_roundup()") > >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > >> --- > >> mm/slab_common.c | 25 ++++++++++++++----------- > >> 1 file changed, 14 insertions(+), 11 deletions(-) > >> > >> diff --git a/mm/slab_common.c b/mm/slab_common.c > >> index e99e821065c3..1dc108224bd1 100644 > >> --- a/mm/slab_common.c > >> +++ b/mm/slab_common.c > >> @@ -747,22 +747,25 @@ size_t kmalloc_size_roundup(size_t size) > >> { > >> struct kmem_cache *c; > >> > >> - /* Short-circuit the 0 size case. */ > >> - if (unlikely(size == 0)) > >> - return 0; > >> - /* Short-circuit saturated "too-large" case. */ > >> - if (unlikely(size == SIZE_MAX)) > >> - return SIZE_MAX; > >> + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { > >> + /* > >> + * The flags don't matter since size_index is common to all. > >> + * Neither does the caller for just getting ->object_size. > >> + */ > >> + c = kmalloc_slab(size, GFP_KERNEL, 0); > >> + return c->object_size; > >> + } > >> + > >> /* Above the smaller buckets, size is a multiple of page size. */ > >> - if (size > KMALLOC_MAX_CACHE_SIZE) > >> + if (size && size <= KMALLOC_MAX_SIZE) > >> return PAGE_SIZE << get_order(size); > >> > >> /* > >> - * The flags don't matter since size_index is common to all. > >> - * Neither does the caller for just getting ->object_size. > >> + * Return 'size' for 0 - kmalloc() returns ZERO_SIZE_PTR > >> + * and very large size - kmalloc() may fail. > >> */ > >> - c = kmalloc_slab(size, GFP_KERNEL, 0); > >> - return c ? c->object_size : 0; > >> + return size; > >> + > >> } > >> EXPORT_SYMBOL(kmalloc_size_roundup); > >> > >> -- > >> 2.42.0 > >> > >> > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales) >
diff --git a/mm/slab_common.c b/mm/slab_common.c index cd71f9581e67..0fb7c7e19bad 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -747,22 +747,22 @@ size_t kmalloc_size_roundup(size_t size) { struct kmem_cache *c; - /* Short-circuit the 0 size case. */ - if (unlikely(size == 0)) - return 0; - /* Short-circuit saturated "too-large" case. */ - if (unlikely(size == SIZE_MAX)) - return SIZE_MAX; + if (size && size <= KMALLOC_MAX_CACHE_SIZE) { + /* + * The flags don't matter since size_index is common to all. + * Neither does the caller for just getting ->object_size. + */ + c = kmalloc_slab(size, GFP_KERNEL, 0); + return likely(c) ? c->object_size : size; + } + /* Above the smaller buckets, size is a multiple of page size. */ - if (size > KMALLOC_MAX_CACHE_SIZE) + if (size && size <= KMALLOC_MAX_SIZE) return PAGE_SIZE << get_order(size); - /* - * The flags don't matter since size_index is common to all. - * Neither does the caller for just getting ->object_size. - */ - c = kmalloc_slab(size, GFP_KERNEL, 0); - return c ? c->object_size : 0; + /* Return 'size' for 0 and very large - kmalloc() may fail. */ + return size; + } EXPORT_SYMBOL(kmalloc_size_roundup);
The typical use of kmalloc_size_roundup() is: ptr = kmalloc(sz = kmalloc_size_roundup(size), ...); if (!ptr) return -ENOMEM. This means it is vitally important that the returned value isn't less than the argument even if the argument is insane. In particular if kmalloc_slab() fails or the value is above (MAX_ULONG - PAGE_SIZE) zero is returned and kmalloc() will return it's single zero-length buffer. Fix by returning the input size on error or if the size exceeds a 'sanity' limit. kmalloc() will then return NULL is the size really is too big. Signed-off-by: David Laight <david.laight@aculab.com> Fixes: 05a940656e1eb ("slab: Introduce kmalloc_size_roundup()") --- v2: - Use KMALLOC_MAX_SIZE for upper limit. (KMALLOC_MAX_SIZE + 1 may give better code on some archs!) - Invert test for overlarge for consistency. - Put a likely() on result of kmalloc_slab(). mm/slab_common.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)