kernel: use kcalloc instead kmalloc/memset

This patch converts kernel/ to use kcalloc instead of kmalloc/memset.

- struct resource *res = kmalloc(sizeof(*res), GFP_KERNEL);
+ struct resource *res = kcalloc(1, sizeof(*res), GFP_KERNEL);

Quoted 1 line

This patch converts kernel/ to use kcalloc instead of kmalloc/memset.

grr.

Quoted 2 lines

- struct resource *res = kmalloc(sizeof(*res), GFP_KERNEL);
+ struct resource *res = kcalloc(1, sizeof(*res), GFP_KERNEL);

Notice how every conversion you did passes in `1' in the first argument?

And that's going to happen again and again and again. Each callsite needlessly passing that silly third argument, adding more kernel text.

If we're going to do this, we should add a two-arg helper function first, and use that, no?

[PATCH] use kzalloc instead of kmalloc/memset

That'll generate just as much code as simply using kcalloc(1, ...). This function should be out-of-line and EXPORT_SYMBOL()ed. And kcalloc() can call it too..

include/linux/slab.h extern void *kcalloc(size_t, size_t, uns · line 110

Quoted 5 lines

include/linux/slab.h +5 -0

+static inline void *kzalloc(size_t size, unsigned int __nocast flags)
+{
+	return kcalloc(1, size, flags);
+}
+

That'll generate just as much code as simply using kcalloc(1, ...). This function should be out-of-line and EXPORT_SYMBOL()ed. And kcalloc() can call it too..

Hi,

Quoted 9 lines · Andrew Morton

include/linux/slab.h extern void *kcalloc(size_t, size_t, uns · line 110

Quoted 5 lines

include/linux/slab.h +5 -0

+static inline void *kzalloc(size_t size, unsigned int __nocast flags)
+{
+	return kcalloc(1, size, flags);
+}
+

That'll generate just as much code as simply using kcalloc(1, ...). This function should be out-of-line and EXPORT_SYMBOL()ed. And kcalloc() can call it too..

BTW we already have 420 "kcalloc(1, ...)" user and 41 without the 1 argument, most of them are in sound, which introduced it in first place. Can we please deprecate that function before it spreads any further?

Quoted 2 lines

BTW we already have 420 "kcalloc(1, ...)" user and 41 without the 1 argument, most of them are in sound, which introduced it in first place. Can we please deprecate that function before it spreads any further?

kcalloc does have value, in that it's a nice api to avoid multiplication overflows. Just for "1" it's indeed not the most useful API.

kcalloc does have value, in that it's a nice api to avoid multiplication overflows. Just for "1" it's indeed not the most useful API.

Hi,

Quoted 2 lines · Arjan van de Ven

kcalloc does have value, in that it's a nice api to avoid multiplication overflows. Just for "1" it's indeed not the most useful API.

This would imply a similiar kmalloc() would be useful as well. Second, how relevant is it for the kernel?

Is that really the best place
to check for rogue user parameters?

Quoted 2 lines

kcalloc does have value, in that it's a nice api to avoid multiplication overflows. Just for "1" it's indeed not the most useful API.

This would imply a similiar kmalloc() would be useful as well.
Second, how relevant is it for the kernel? Is that really the best place to check for rogue user parameters?

Quoted 2 lines

This would imply a similiar kmalloc() would be useful as well. Second, how relevant is it for the kernel?

we've had a non-negliable amount of security holes because of this

Quoted 2 lines

Is that really the best place
to check for rogue user parameters?

it makes it easy and safe. Of course you can and should check it in all users. Just that using a safer API is generally better than forcing everyone to do it themselves.

Hi,

Quoted 4 lines · Arjan van de Ven

Quoted 2 lines

This would imply a similiar kmalloc() would be useful as well. Second, how relevant is it for the kernel?

we've had a non-negliable amount of security holes because of this

So why don't we have a similiar kmalloc()?

Quoted 7 lines

Quoted 3 lines

Is that really the best place
to check for rogue user parameters?

it makes it easy and safe. Of course you can and should check it in all users. Just that using a safer API is generally better than forcing everyone to do it themselves.

How exactly does this make it a "safe API"? Even if it checks for this one case, it still makes the user suspectible for allocating big amounts of unswappable memory.

Quoted 3 lines

Quoted 1 line

we've had a non-negliable amount of security holes because of this

So why don't we have a similiar kmalloc()?

nope kmalloc is not an array allocator

Quoted 4 lines

Quoted 3 lines

it makes it easy and safe. Of course you can and should check it in all users. Just that using a safer API is generally better than forcing everyone to do it themselves.

Quoted 3 lines

How exactly does this make it a "safe API"? Even if it checks for this one case, it still makes the user suspectible for allocating big amounts of unswappable memory.

128Kb max.

Hi,

Quoted 10 lines · Arjan van de Ven

Quoted 3 lines

Quoted 1 line

we've had a non-negliable amount of security holes because of this

So why don't we have a similiar kmalloc()?

nope kmalloc is not an array allocator

Quoted 4 lines

Quoted 3 lines

it makes it easy and safe. Of course you can and should check it in all users. Just that using a safer API is generally better than forcing everyone to do it themselves.

Quoted 5 lines

Quoted 3 lines

How exactly does this make it a "safe API"? Even if it checks for this one case, it still makes the user suspectible for allocating big amounts of unswappable memory.

128Kb max.

You completely missed the point and didn't answer my questions at all... :-(

Quoted 4 lines · Arjan van de Ven

Quoted 2 lines

This would imply a similiar kmalloc() would be useful as well. Second, how relevant is it for the kernel?

we've had a non-negliable amount of security holes because of this

So why don't we have a similiar kmalloc()?

Quoted 5 lines · Arjan van de Ven

Quoted 3 lines

Quoted 2 lines

This would imply a similiar kmalloc() would be useful as well. Second, how relevant is it for the kernel?

we've had a non-negliable amount of security holes because of this

So why don't we have a similiar kmalloc()?

You mean something like:
static void __bad_kmalloc_safe_nonconstant_size(void);

Hmm. If we had kcmalloc then we may be able to add a zero bit to the slab allocator. If we would obtain zeroed pages for the slab then we may skip zeroing of individual entries. However, the cache warming effect of the current zeroing is then not occurring. Not sure if this would make sense but this is a possible optimization if we had kcmalloc.

Maybe it helps if I give the basic bug scenario first (pseudo C)

void some_ioctl_func(...)
{
int count, i;
struct foo *ptr;

  copy_from_user(&count,...);

  ptr = kmalloc(sizeof(struct foo) * count);

if (!ptr)

     return -ENOMEM;

  for (i=0; i<count; i++) {
      initialize(ptr+i);

}
}

if the user picks count such that the multiplication overflows, the kmalloc will actually *succeed* in getting a chunk between 0 and 128Kb. The subsequent "fill the array up" will overwrite a LOT of kernel memory though.

Fixing the hole of course involves checking "count" for too high a value. Using kcalloc() will check for this same overflow inside kcalloc and prevent it (eg return NULL) if one of these slips through.

I actually looked at the current kcalloc users and besides a few unchecked module parameters, the arguments were either constant or had to be checked anyway. I didn't find a single example which required the "safety" of kcalloc().

...

mm/slab.c +14 -0

@@ -2555,6 +2555,20 @@ void kmem_cache_free(kmem_cache_t *cache
 EXPORT_SYMBOL(kmem_cache_free);
 
 /**
+ * kzalloc - allocate memory. The memory is set to zero.
+ * @size: how many bytes of memory are required.
+ * @flags: the type of memory to allocate.
+ */
+void *kzalloc(size_t size, unsigned int __nocast flags)
+{
+	void *ret = kmalloc(size, flags);
+	if (ret)
+		memset(ret, 0, size);
+	return ret;
+}
+EXPORT_SYMBOL(kzalloc);
+
+/**
  * kcalloc - allocate memory for an array. The memory is set to zero.
  * @n: number of elements.
  * @size: element size.

What prevents a rogue user to call this function a number of times to waste resources?

Quoted 25 lines · Pekka J Enberg

...

mm/slab.c +14 -0

@@ -2555,6 +2555,20 @@ void kmem_cache_free(kmem_cache_t *cache
 EXPORT_SYMBOL(kmem_cache_free);
 
 /**
+ * kzalloc - allocate memory. The memory is set to zero.
+ * @size: how many bytes of memory are required.
+ * @flags: the type of memory to allocate.
+ */
+void *kzalloc(size_t size, unsigned int __nocast flags)
+{
+	void *ret = kmalloc(size, flags);
+	if (ret)
+		memset(ret, 0, size);
+	return ret;
+}
+EXPORT_SYMBOL(kzalloc);
+
+/**
  * kcalloc - allocate memory for an array. The memory is set to zero.
  * @n: number of elements.
  * @size: element size.

mm/slab.c void *kcalloc(size_t n, size_t size, uns · line 2586

Quoted 10 lines

mm/slab.c +1 -4

@@ -2567,10 +2581,7 @@ void *kcalloc(size_t n, size_t size, uns
 	if (n != 0 && size > INT_MAX / n)
 		return ret;
 
-	ret = kmalloc(n * size, flags);
-	if (ret)
-		memset(ret, 0, n * size);
-	return ret;
+	return kzalloc(n * size, flags);
 }
 EXPORT_SYMBOL(kcalloc);

Looking at how few is left from kcalloc, can't we make it a "static inline" function in slab.h?

This would optimize nicely for all of the users where the first or even the first two parameters are constant at compile-time and shouldn't do much harm for the other users.

As a side effect, the difference between kcalloc(1, ...) and kzalloc() would become a coding style question without any effect on the generated code.

At Sat, 6 Aug 2005 17:09:40 +0200,

Quoted 9 lines · Adrian Bunk

Looking at how few is left from kcalloc, can't we make it a "static inline" function in slab.h?

This would optimize nicely for all of the users where the first or even the first two parameters are constant at compile-time and shouldn't do much harm for the other users.

Quoted 3 lines

As a side effect, the difference between kcalloc(1, ...) and kzalloc() would become a coding style question without any effect on the generated code.

How about to use __builtin_constant_p() like kmalloc? The code readability would be worsen, though...

Takashi

Quoted 15 lines · Takashi Iwai

At Sat, 6 Aug 2005 17:09:40 +0200,

Quoted 12 lines · Adrian Bunk

Looking at how few is left from kcalloc, can't we make it a "static inline" function in slab.h?

This would optimize nicely for all of the users where the first or even the first two parameters are constant at compile-time and shouldn't do much harm for the other users.

As a side effect, the difference between kcalloc(1, ...) and kzalloc() would become a coding style question without any effect on the generated code.

Quoted 2 lines

How about to use __builtin_constant_p() like kmalloc? The code readability would be worsen, though...

Where should this make any difference?

If the function is "static inline", gcc can e.g. always determine at compile-time that 1 >= 0 can never be false and therefore optimize it away.