[PATCH] kernel: use kcalloc instead kmalloc/memset

[PATCH] kernel: use kcalloc instead kmalloc/memset

[Pekka Enberg]

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

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 intermodule.c |    3 +--
 params.c      |    4 ++--
 power/pm.c    |    3 +--
 resource.c    |    3 +--
 workqueue.c   |    3 +--
 5 files changed, 6 insertions(+), 10 deletions(-)

Index: 2.6/kernel/resource.c
===================================================================
--- 2.6.orig/kernel/resource.c
+++ 2.6/kernel/resource.c
@@ -430,10 +430,9 @@ EXPORT_SYMBOL(adjust_resource);
  */
 struct resource * __request_region(struct resource *parent, unsigned long start, unsigned long n, const char *name)
 {
-	struct resource *res = kmalloc(sizeof(*res), GFP_KERNEL);
+	struct resource *res = kcalloc(1, sizeof(*res), GFP_KERNEL);
 
 	if (res) {
-		memset(res, 0, sizeof(*res));
 		res->name = name;
 		res->start = start;
 		res->end = start + n - 1;
Index: 2.6/kernel/intermodule.c
===================================================================
--- 2.6.orig/kernel/intermodule.c
+++ 2.6/kernel/intermodule.c
@@ -39,7 +39,7 @@ void inter_module_register(const char *i
 	struct list_head *tmp;
 	struct inter_module_entry *ime, *ime_new;
 
-	if (!(ime_new = kmalloc(sizeof(*ime), GFP_KERNEL))) {
+	if (!(ime_new = kcalloc(1, sizeof(*ime), GFP_KERNEL))) {
 		/* Overloaded kernel, not fatal */
 		printk(KERN_ERR
 			"Aiee, inter_module_register: cannot kmalloc entry for '%s'\n",
@@ -47,7 +47,6 @@ void inter_module_register(const char *i
 		kmalloc_failed = 1;
 		return;
 	}
-	memset(ime_new, 0, sizeof(*ime_new));
 	ime_new->im_name = im_name;
 	ime_new->owner = owner;
 	ime_new->userdata = userdata;
Index: 2.6/kernel/params.c
===================================================================
--- 2.6.orig/kernel/params.c
+++ 2.6/kernel/params.c
@@ -542,8 +542,8 @@ static void __init kernel_param_sysfs_se
 {
 	struct module_kobject *mk;
 
-	mk = kmalloc(sizeof(struct module_kobject), GFP_KERNEL);
-	memset(mk, 0, sizeof(struct module_kobject));
+	mk = kcalloc(1, sizeof(struct module_kobject), GFP_KERNEL);
+	BUG_ON(!mk);
 
 	mk->mod = THIS_MODULE;
 	kobj_set_kset_s(mk, module_subsys);
Index: 2.6/kernel/power/pm.c
===================================================================
--- 2.6.orig/kernel/power/pm.c
+++ 2.6/kernel/power/pm.c
@@ -60,9 +60,8 @@ struct pm_dev *pm_register(pm_dev_t type
 			   unsigned long id,
 			   pm_callback callback)
 {
-	struct pm_dev *dev = kmalloc(sizeof(struct pm_dev), GFP_KERNEL);
+	struct pm_dev *dev = kcalloc(1, sizeof(struct pm_dev), GFP_KERNEL);
 	if (dev) {
-		memset(dev, 0, sizeof(*dev));
 		dev->type = type;
 		dev->id = id;
 		dev->callback = callback;
Index: 2.6/kernel/workqueue.c
===================================================================
--- 2.6.orig/kernel/workqueue.c
+++ 2.6/kernel/workqueue.c
@@ -310,10 +310,9 @@ struct workqueue_struct *__create_workqu
 
 	BUG_ON(strlen(name) > 10);
 
-	wq = kmalloc(sizeof(*wq), GFP_KERNEL);
+	wq = kcalloc(1, sizeof(*wq), GFP_KERNEL);
 	if (!wq)
 		return NULL;
-	memset(wq, 0, sizeof(*wq));
 
 	wq->name = name;
 	/* We don't need the distraction of CPUs appearing and vanishing. */

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Andrew Morton]

Pekka Enberg <penberg@cs.helsinki.fi> wrote:
>
> This patch converts kernel/ to use kcalloc instead of kmalloc/memset.
>

grr.
 
>  -	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?

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Pekka J Enberg]

Hi Andrew,

Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> > This patch converts kernel/ to use kcalloc instead of kmalloc/memset.

On Thu, 4 Aug 2005, Andrew Morton wrote:
> grr.

I am learning grep. Please don't eat me!

> >  -	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?

This was discussed some time ago [1] and here's a patch for you to do 
that.

		Pekka

  1. http://marc.theaimsgroup.com/?l=linux-kernel&m=111279001428661&w=2

[PATCH] use kzalloc instead of kmalloc/memset

This patch introduces a kzalloc wrapper and converts kernel/ to use it.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 include/linux/slab.h |    5 +++++
 kernel/intermodule.c |    3 +--
 kernel/params.c      |    4 ++--
 kernel/power/pm.c    |    3 +--
 kernel/resource.c    |    3 +--
 kernel/workqueue.c   |    3 +--
 6 files changed, 11 insertions(+), 10 deletions(-)

Index: 2.6/kernel/resource.c
===================================================================
--- 2.6.orig/kernel/resource.c
+++ 2.6/kernel/resource.c
@@ -430,10 +430,9 @@ EXPORT_SYMBOL(adjust_resource);
  */
 struct resource * __request_region(struct resource *parent, unsigned long start, unsigned long n, const char *name)
 {
-	struct resource *res = kmalloc(sizeof(*res), GFP_KERNEL);
+	struct resource *res = kzalloc(sizeof(*res), GFP_KERNEL);
 
 	if (res) {
-		memset(res, 0, sizeof(*res));
 		res->name = name;
 		res->start = start;
 		res->end = start + n - 1;
Index: 2.6/kernel/intermodule.c
===================================================================
--- 2.6.orig/kernel/intermodule.c
+++ 2.6/kernel/intermodule.c
@@ -39,7 +39,7 @@ void inter_module_register(const char *i
 	struct list_head *tmp;
 	struct inter_module_entry *ime, *ime_new;
 
-	if (!(ime_new = kmalloc(sizeof(*ime), GFP_KERNEL))) {
+	if (!(ime_new = kzalloc(sizeof(*ime), GFP_KERNEL))) {
 		/* Overloaded kernel, not fatal */
 		printk(KERN_ERR
 			"Aiee, inter_module_register: cannot kmalloc entry for '%s'\n",
@@ -47,7 +47,6 @@ void inter_module_register(const char *i
 		kmalloc_failed = 1;
 		return;
 	}
-	memset(ime_new, 0, sizeof(*ime_new));
 	ime_new->im_name = im_name;
 	ime_new->owner = owner;
 	ime_new->userdata = userdata;
Index: 2.6/kernel/params.c
===================================================================
--- 2.6.orig/kernel/params.c
+++ 2.6/kernel/params.c
@@ -542,8 +542,8 @@ static void __init kernel_param_sysfs_se
 {
 	struct module_kobject *mk;
 
-	mk = kmalloc(sizeof(struct module_kobject), GFP_KERNEL);
-	memset(mk, 0, sizeof(struct module_kobject));
+	mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
+	BUG_ON(!mk);
 
 	mk->mod = THIS_MODULE;
 	kobj_set_kset_s(mk, module_subsys);
Index: 2.6/kernel/power/pm.c
===================================================================
--- 2.6.orig/kernel/power/pm.c
+++ 2.6/kernel/power/pm.c
@@ -60,9 +60,8 @@ struct pm_dev *pm_register(pm_dev_t type
 			   unsigned long id,
 			   pm_callback callback)
 {
-	struct pm_dev *dev = kmalloc(sizeof(struct pm_dev), GFP_KERNEL);
+	struct pm_dev *dev = kzalloc(sizeof(struct pm_dev), GFP_KERNEL);
 	if (dev) {
-		memset(dev, 0, sizeof(*dev));
 		dev->type = type;
 		dev->id = id;
 		dev->callback = callback;
Index: 2.6/kernel/workqueue.c
===================================================================
--- 2.6.orig/kernel/workqueue.c
+++ 2.6/kernel/workqueue.c
@@ -310,10 +310,9 @@ struct workqueue_struct *__create_workqu
 
 	BUG_ON(strlen(name) > 10);
 
-	wq = kmalloc(sizeof(*wq), GFP_KERNEL);
+	wq = kzalloc(sizeof(*wq), GFP_KERNEL);
 	if (!wq)
 		return NULL;
-	memset(wq, 0, sizeof(*wq));
 
 	wq->name = name;
 	/* We don't need the distraction of CPUs appearing and vanishing. */
Index: 2.6/include/linux/slab.h
===================================================================
--- 2.6.orig/include/linux/slab.h
+++ 2.6/include/linux/slab.h
@@ -103,6 +103,11 @@ extern void *kcalloc(size_t, size_t, uns
 extern void kfree(const void *);
 extern unsigned int ksize(const void *);
 
+static inline void *kzalloc(size_t size, unsigned int __nocast flags)
+{
+	return kcalloc(1, size, flags);
+}
+
 #ifdef CONFIG_NUMA
 extern void *kmem_cache_alloc_node(kmem_cache_t *, int flags, int node);
 extern void *kmalloc_node(size_t size, unsigned int __nocast flags, int node);

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Andrew Morton]

Pekka J Enberg <penberg@cs.Helsinki.FI> wrote:
>
> [PATCH] use kzalloc instead of kmalloc/memset
> 

dammit, I was hoping for akpmalloc()

>  
> +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..

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Pekka J Enberg]

On Thu, 4 Aug 2005, Andrew Morton wrote:
> 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..

Yes, much better now. Thanks Andrew.

				Pekka

[PATCH] introduce kzalloc

This patch introduces a kzalloc wrapper and converts kernel/ to use it.

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

 include/linux/slab.h |    1 +
 kernel/intermodule.c |    3 +--
 kernel/params.c      |    4 ++--
 kernel/power/pm.c    |    3 +--
 kernel/resource.c    |    3 +--
 kernel/workqueue.c   |    3 +--
 mm/slab.c            |   19 +++++++++++++++----
 7 files changed, 22 insertions(+), 14 deletions(-)

Index: 2.6/kernel/resource.c
===================================================================
--- 2.6.orig/kernel/resource.c
+++ 2.6/kernel/resource.c
@@ -430,10 +430,9 @@ EXPORT_SYMBOL(adjust_resource);
  */
 struct resource * __request_region(struct resource *parent, unsigned long start, unsigned long n, const char *name)
 {
-	struct resource *res = kmalloc(sizeof(*res), GFP_KERNEL);
+	struct resource *res = kzalloc(sizeof(*res), GFP_KERNEL);
 
 	if (res) {
-		memset(res, 0, sizeof(*res));
 		res->name = name;
 		res->start = start;
 		res->end = start + n - 1;
Index: 2.6/kernel/intermodule.c
===================================================================
--- 2.6.orig/kernel/intermodule.c
+++ 2.6/kernel/intermodule.c
@@ -39,7 +39,7 @@ void inter_module_register(const char *i
 	struct list_head *tmp;
 	struct inter_module_entry *ime, *ime_new;
 
-	if (!(ime_new = kmalloc(sizeof(*ime), GFP_KERNEL))) {
+	if (!(ime_new = kzalloc(sizeof(*ime), GFP_KERNEL))) {
 		/* Overloaded kernel, not fatal */
 		printk(KERN_ERR
 			"Aiee, inter_module_register: cannot kmalloc entry for '%s'\n",
@@ -47,7 +47,6 @@ void inter_module_register(const char *i
 		kmalloc_failed = 1;
 		return;
 	}
-	memset(ime_new, 0, sizeof(*ime_new));
 	ime_new->im_name = im_name;
 	ime_new->owner = owner;
 	ime_new->userdata = userdata;
Index: 2.6/kernel/params.c
===================================================================
--- 2.6.orig/kernel/params.c
+++ 2.6/kernel/params.c
@@ -542,8 +542,8 @@ static void __init kernel_param_sysfs_se
 {
 	struct module_kobject *mk;
 
-	mk = kmalloc(sizeof(struct module_kobject), GFP_KERNEL);
-	memset(mk, 0, sizeof(struct module_kobject));
+	mk = kzalloc(sizeof(struct module_kobject), GFP_KERNEL);
+	BUG_ON(!mk);
 
 	mk->mod = THIS_MODULE;
 	kobj_set_kset_s(mk, module_subsys);
Index: 2.6/kernel/power/pm.c
===================================================================
--- 2.6.orig/kernel/power/pm.c
+++ 2.6/kernel/power/pm.c
@@ -60,9 +60,8 @@ struct pm_dev *pm_register(pm_dev_t type
 			   unsigned long id,
 			   pm_callback callback)
 {
-	struct pm_dev *dev = kmalloc(sizeof(struct pm_dev), GFP_KERNEL);
+	struct pm_dev *dev = kzalloc(sizeof(struct pm_dev), GFP_KERNEL);
 	if (dev) {
-		memset(dev, 0, sizeof(*dev));
 		dev->type = type;
 		dev->id = id;
 		dev->callback = callback;
Index: 2.6/kernel/workqueue.c
===================================================================
--- 2.6.orig/kernel/workqueue.c
+++ 2.6/kernel/workqueue.c
@@ -310,10 +310,9 @@ struct workqueue_struct *__create_workqu
 
 	BUG_ON(strlen(name) > 10);
 
-	wq = kmalloc(sizeof(*wq), GFP_KERNEL);
+	wq = kzalloc(sizeof(*wq), GFP_KERNEL);
 	if (!wq)
 		return NULL;
-	memset(wq, 0, sizeof(*wq));
 
 	wq->name = name;
 	/* We don't need the distraction of CPUs appearing and vanishing. */
Index: 2.6/include/linux/slab.h
===================================================================
--- 2.6.orig/include/linux/slab.h
+++ 2.6/include/linux/slab.h
@@ -100,6 +100,7 @@ found:
 }
 
 extern void *kcalloc(size_t, size_t, unsigned int __nocast);
+extern void *kzalloc(size_t, unsigned int __nocast);
 extern void kfree(const void *);
 extern unsigned int ksize(const void *);
 
Index: 2.6/mm/slab.c
===================================================================
--- 2.6.orig/mm/slab.c
+++ 2.6/mm/slab.c
@@ -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.
@@ -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);
 

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Roman Zippel]

Hi,

On Thu, 4 Aug 2005, Andrew Morton wrote:

> > +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?

bye, Roman

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Arjan van de Ven]

On Fri, 2005-08-05 at 11:37 +0200, Roman Zippel wrote:
> Hi,
> 
> On Thu, 4 Aug 2005, Andrew Morton wrote:
> 
> > > +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?

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. 

Re: kernel: use kcalloc instead kmalloc/memset

[Pekka J Enberg]

On Fri, 2005-08-05 at 11:37 +0200, Roman Zippel wrote:
> > 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?

Arjan van de Ven writes:
> 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. 

Indeed, kcalloc should not be deprecated as it is meant for array 
allocation. I can send patches that replace kcalloc(1,...) with kzalloc if 
there are no other janitors interested... 

                 Pekka 

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Roman Zippel]

Hi,

On Fri, 5 Aug 2005, Arjan van de Ven wrote:

> 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?

bye, Roman

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Arjan van de Ven]

On Fri, 2005-08-05 at 12:07 +0200, Roman Zippel wrote:
> Hi,
> 
> On Fri, 5 Aug 2005, Arjan van de Ven wrote:
> 
> > 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? 

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

> 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.

Re: kernel: use kcalloc instead kmalloc/memset

[Pekka J Enberg]

On Fri, 5 Aug 2005, Arjan van de Ven wrote:
> > 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. 

Roman Zippel writes:
> 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?

Please note that the first version of kcalloc() carried over from ALSA took 
only one size parameter but we decided to make it similar to standard 
calloc(). It is useful as an array allocator which is why I think we should 
keep both and simply get rid of the kcalloc(1,...) form. 

                Pekka 

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Roman Zippel]

Hi,

On Fri, 5 Aug 2005, Arjan van de Ven wrote:

> > 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()?

> > 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.

bye, Roman

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Arjan van de Ven]

On Fri, 2005-08-05 at 12:32 +0200, Roman Zippel wrote:
> Hi,
> 
> On Fri, 5 Aug 2005, Arjan van de Ven wrote:
> 
> > > 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()?

nope kmalloc is not an array allocator
> 
> > > 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.

128Kb max.

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Roman Zippel]

Hi,

On Fri, 5 Aug 2005, Arjan van de Ven wrote:

> > > 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
> 
> > > 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.
> 
> 128Kb max.

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

bye, Roman

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Arjan van de Ven]

On Fri, 2005-08-05 at 12:56 +0200, Roman Zippel wrote:
> Hi,
> 
> On Fri, 5 Aug 2005, Arjan van de Ven wrote:
> 
> > > > 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
> > 
> > > > 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.
> > 
> > 128Kb max.
> 
> You completely missed the point and didn't answer my questions at all... :-(

I found it hard to understand your question.

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.

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Stephen Pollei]

On 8/5/05, Roman Zippel <zippel@linux-m68k.org> wrote:
> On Fri, 5 Aug 2005, Arjan van de Ven wrote:
> > > 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);
static void __bad_kmalloc_safe_zero_size(void);
static void __bad_kmalloc_safe_too_large_size(void);
static void __bad_kmalloc_safe_too_large(void);
static inline void *kmalloc_safe(size_t nmemb, size_t size,int flags) {
        if (!__builtin_constant_p(size))
               __bad_kmalloc_safe_nonconstant_size();
        if ( !size )
                __bad_kmalloc_safe_zero_size();
        if ( size > 0x10000)
                __bad_kmalloc_safe_too_large_size();
        if (__builtin_constant_p(nmemb) && nmemb > 0x20000/size)
                __bad_kmalloc_safe_too_large();
        if (nmemb <= 0x20000/size)
                return kmalloc(nmemb*size,flags);
        else return 0; }


-- 
http://dmoz.org/profiles/pollei.html
http://sourceforge.net/users/stephen_pollei/
http://www.orkut.com/Profile.aspx?uid=2455954990164098214
http://stephen_pollei.home.comcast.net/

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Christoph Lameter]

On Fri, 5 Aug 2005, Stephen Pollei wrote:

> > On Fri, 5 Aug 2005, Arjan van de Ven wrote:
> > > > 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.

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Stephen Pollei]

On 8/5/05, Christoph Lameter <christoph@lameter.com> wrote:

> 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.

Well there is kzalloc and kcalloc. I just thought a safe non-zeroing
version would be nice.
You could warm the cache with prefetch, but you'd need to profile the
diferent cases to see what is worth doing and what isn't.


-- 
http://dmoz.org/profiles/pollei.html
http://sourceforge.net/users/stephen_pollei/
http://www.orkut.com/Profile.aspx?uid=2455954990164098214
http://stephen_pollei.home.comcast.net/

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Roman Zippel]

Hi,

On Fri, 5 Aug 2005, Arjan van de Ven wrote:

> 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.

What prevents a rogue user to call this function a number of times to 
waste resources?
kcalloc() covers only small part of what is needed to make an interface 
secure, once one checked everything else, the safety provided by kcalloc() 
is pretty much redundant.
Relying on the current allocation limits would be rather foolish as these 
can change anytime and hardcoding such assumptions is a really bad idea. 
Kernel coding requires a careful use of the memory resource, so it's the 
job of the driver to define reasonable limits, e.g. such arrays don't make 
much sense if they require too much space and the driver should define a 
limit like (PAGESIZE/sizeof(struct foo)). If the driver writer doesn't 
think about memory usage, then kcalloc() will do pretty much nothing too 
improve the driver, it may avoid a few problem cases, but there will be 
certainly more than enough problems left.
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().

bye, Roman

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Pekka Enberg]

On Sat, 2005-08-06 at 02:40 +0200, Roman Zippel wrote:
> 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().

Every caller of kcalloc() should do proper bounds checking. It's just
that when they forget to do it, we hopefully avoid a buffer overflow
which can be exploited.

			Pekka

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Adrian Bunk]

On Fri, Aug 05, 2005 at 09:52:32AM +0300, Pekka J Enberg wrote:
>...
> --- 2.6.orig/mm/slab.c
> +++ 2.6/mm/slab.c
> @@ -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.
> @@ -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.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Jan Engelhardt]

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

Sorry to jump in, but wasting resources is a different matter than 
overwriting kernel memory.



Jan Engelhardt
-- 

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Takashi Iwai]

At Sat, 6 Aug 2005 17:09:40 +0200,
Adrian Bunk wrote:
> 
> On Fri, Aug 05, 2005 at 09:52:32AM +0300, Pekka J Enberg wrote:
> >...
> > --- 2.6.orig/mm/slab.c
> > +++ 2.6/mm/slab.c
> > @@ -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.
> > @@ -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.

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


Takashi

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Adrian Bunk]

On Mon, Aug 08, 2005 at 12:11:21PM +0200, Takashi Iwai wrote:
> At Sat, 6 Aug 2005 17:09:40 +0200,
> Adrian Bunk wrote:
> > 
> > 
> > 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.
> 
> 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.

> Takashi

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] kernel: use kcalloc instead kmalloc/memset

[Takashi Iwai]

At Mon, 8 Aug 2005 23:05:24 +0200,
Adrian Bunk wrote:
> 
> On Mon, Aug 08, 2005 at 12:11:21PM +0200, Takashi Iwai wrote:
> > At Sat, 6 Aug 2005 17:09:40 +0200,
> > Adrian Bunk wrote:
> > > 
> > > 
> > > 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.
> > 
> > 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.

Argh, forget my post.  It was just lack of caffeine :)


Takashi