[PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Pekka J Enberg]

Introduce kcalloc() in linux/slab.h that is used to replace snd_calloc() and
snd_magic_calloc().

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

 include/linux/slab.h |    1 +
 mm/slab.c            |   15 +++++++++++++++
 2 files changed, 16 insertions(+)

--- linux-2.6.6/include/linux/slab.h	2004-06-08 23:54:41.382519192 +0300
+++ alsa-2.6.6/include/linux/slab.h	2004-06-08 21:36:12.951592680 +0300
@@ -95,6 +95,7 @@ found:
 	return __kmalloc(size, flags);
 }
 
+extern void *kcalloc(size_t, int);
 extern void kfree(const void *);
 extern unsigned int ksize(const void *);
 
--- linux-2.6.6/mm/slab.c	2004-06-08 23:57:30.713776928 +0300
+++ alsa-2.6.6/mm/slab.c	2004-06-08 21:42:18.000000000 +0300
@@ -2332,6 +2332,21 @@ void kmem_cache_free (kmem_cache_t *cach
 EXPORT_SYMBOL(kmem_cache_free);
 
 /**
+ * kcalloc - allocate memory. The memory is set to zero.
+ * @size: how many bytes of memory are required.
+ * @flags: the type of memory to allocate.
+ */
+void *kcalloc(size_t size, int flags)
+{
+	void *ret = kmalloc(size, flags);
+	if (ret)
+		memset(ret, 0, size);
+	return ret;
+}
+
+EXPORT_SYMBOL(kcalloc);
+
+/**
  * kfree - free previously allocated memory
  * @objp: pointer returned by kmalloc.
  *

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Chris Wright]

* Pekka J Enberg (penberg@cs.helsinki.fi) wrote:
>  /**
> + * kcalloc - allocate memory. The memory is set to zero.
> + * @size: how many bytes of memory are required.
> + * @flags: the type of memory to allocate.
> + */
> +void *kcalloc(size_t size, int flags)
> +{
> +	void *ret = kmalloc(size, flags);
> +	if (ret)
> +		memset(ret, 0, size);
> +	return ret;
> +}

This looks more like the kzalloc() that pops up every now and then.
Wouldn't it make more sense to give kcalloc() a true calloc() style
interface?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Pekka Enberg]

On Wed, 2004-06-09 at 21:34, Chris Wright wrote:
> This looks more like the kzalloc() that pops up every now and then.
> Wouldn't it make more sense to give kcalloc() a true calloc() style
> interface?

I can go either way. Takashi, do you want me to update the ALSA patches
to use this version?

		Pekka

diff -urN linux-2.6.6/include/linux/slab.h kcalloc-2.6.6/include/linux/slab.h
--- linux-2.6.6/include/linux/slab.h	2004-06-09 22:56:11.874249056 +0300
+++ kcalloc-2.6.6/include/linux/slab.h	2004-06-09 23:03:10.597593432 +0300
@@ -95,6 +95,7 @@
 	return __kmalloc(size, flags);
 }
 
+extern void *kcalloc(size_t, size_t, int);
 extern void kfree(const void *);
 extern unsigned int ksize(const void *);
 
diff -urN linux-2.6.6/mm/slab.c kcalloc-2.6.6/mm/slab.c
--- linux-2.6.6/mm/slab.c	2004-06-09 22:59:13.081701336 +0300
+++ kcalloc-2.6.6/mm/slab.c	2004-06-09 23:07:51.262925816 +0300
@@ -2332,6 +2332,22 @@
 EXPORT_SYMBOL(kmem_cache_free);
 
 /**
+ * kcalloc - allocate memory for an array. The memory is set to zero.
+ * @n: number of elements.
+ * @size: element size.
+ * @flags: the type of memory to allocate.
+ */
+void *kcalloc(size_t n, size_t size, int flags)
+{
+	void *ret = kmalloc(n * size, flags);
+	if (ret)
+		memset(ret, 0, n * size);
+	return ret;
+}
+
+EXPORT_SYMBOL(kcalloc);
+
+/**
  * kfree - free previously allocated memory
  * @objp: pointer returned by kmalloc.
  *

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]


> + */
> +void *kcalloc(size_t n, size_t size, int flags)
> +{
> +	void *ret = kmalloc(n * size, flags);

how about making sure n*size doesn't overflow an int in this function?
We had a few security holes due to that happening a while ago; might as
well prevent it from happening entirely

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 923 bytes --]

On Wed, 09 Jun 2004 22:21:26 +0200, Arjan van de Ven said:

> > + */
> > +void *kcalloc(size_t n, size_t size, int flags)
> > +{
> > +	void *ret = kmalloc(n * size, flags);
> 
> how about making sure n*size doesn't overflow an int in this function?
> We had a few security holes due to that happening a while ago; might as
> well prevent it from happening entirely

Do we want 'int', or is some other value (size_t? u32?) a better bet? (I see on
some of the 64-bit boxes a compat_size_t for 32-bits as well, which hints at
the problems here....

I'm worried that 'int' will Do The Wrong Thing when it runs into stuff like
this from asm-i386/types.h:

/* DMA addresses come in generic and 64-bit flavours.  */

#ifdef CONFIG_HIGHMEM64G
typedef u64 dma_addr_t;
#else
typedef u32 dma_addr_t;
#endif
typedef u64 dma64_addr_t;

Are there any platforms where 'int' and 'max reasonable kmalloc size' have the
same number of bits?

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Chris Wright]

* Arjan van de Ven (arjanv@redhat.com) wrote:
> 
> > + */
> > +void *kcalloc(size_t n, size_t size, int flags)
> > +{
> > +	void *ret = kmalloc(n * size, flags);
> 
> how about making sure n*size doesn't overflow an int in this function?
> We had a few security holes due to that happening a while ago; might as
> well prevent it from happening entirely

Nice point.  Too bad all we can return is NULL, it'd be nice to know the
overflow was the reason.  Do we plop a WARN_ON() in there for a while?
Something like below (w/out any WARN_ON right now)?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

diff -urN linux-2.6.6/include/linux/slab.h kcalloc-2.6.6/include/linux/slab.h
--- linux-2.6.6/include/linux/slab.h	2004-06-09 22:56:11.874249056 +0300
+++ kcalloc-2.6.6/include/linux/slab.h	2004-06-09 23:03:10.597593432 +0300
@@ -95,6 +95,7 @@
 	return __kmalloc(size, flags);
 }
 
+extern void *kcalloc(size_t, size_t, int);
 extern void kfree(const void *);
 extern unsigned int ksize(const void *);
 
diff -urN linux-2.6.6/mm/slab.c kcalloc-2.6.6/mm/slab.c
--- linux-2.6.6/mm/slab.c	2004-06-09 22:59:13.081701336 +0300
+++ kcalloc-2.6.6/mm/slab.c	2004-06-09 23:07:51.262925816 +0300
@@ -2332,6 +2332,28 @@
 EXPORT_SYMBOL(kmem_cache_free);
 
 /**
+ * kcalloc - allocate memory for an array. The memory is set to zero.
+ * @n: number of elements.
+ * @size: element size.
+ * @flags: the type of memory to allocate.
+ */
+void *kcalloc(size_t n, size_t size, int flags)
+{
+	void *ret = NULL;
+
+	/* check for overflow */
+	if (n > UINT_MAX/size)
+		return ret;
+
+	ret = kmalloc(n * size, flags);
+	if (ret)
+		memset(ret, 0, n * size);
+	return ret;
+}
+
+EXPORT_SYMBOL(kcalloc);
+
+/**
  * kfree - free previously allocated memory
  * @objp: pointer returned by kmalloc.
  *

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Pekka Enberg]

On Wed, 2004-06-09 at 23:21, Arjan van de Ven wrote:
> how about making sure n*size doesn't overflow an int in this function?
> We had a few security holes due to that happening a while ago; might as
> well prevent it from happening entirely

Sure.

		Pekka

diff -urN linux-2.6.6/include/linux/slab.h kcalloc-2.6.6/include/linux/slab.h
--- linux-2.6.6/include/linux/slab.h	2004-06-09 22:56:11.874249056 +0300
+++ kcalloc-2.6.6/include/linux/slab.h	2004-06-09 23:03:10.597593432 +0300
@@ -95,6 +95,7 @@
 	return __kmalloc(size, flags);
 }
 
+extern void *kcalloc(size_t, size_t, int);
 extern void kfree(const void *);
 extern unsigned int ksize(const void *);
 
diff -urN linux-2.6.6/mm/slab.c kcalloc-2.6.6/mm/slab.c
--- linux-2.6.6/mm/slab.c	2004-06-09 22:59:13.081701336 +0300
+++ kcalloc-2.6.6/mm/slab.c	2004-06-09 23:50:06.592497136 +0300
@@ -2332,6 +2332,25 @@
 EXPORT_SYMBOL(kmem_cache_free);
 
 /**
+ * kcalloc - allocate memory for an array. The memory is set to zero.
+ * @n: number of elements.
+ * @size: element size.
+ * @flags: the type of memory to allocate.
+ */
+void *kcalloc(size_t n, size_t size, int flags)
+{
+	if (n != 0 && size > INT_MAX / n)
+		return NULL;
+
+	void *ret = kmalloc(n * size, flags);
+	if (ret)
+		memset(ret, 0, n * size);
+	return ret;
+}
+
+EXPORT_SYMBOL(kcalloc);
+
+/**
  * kfree - free previously allocated memory
  * @objp: pointer returned by kmalloc.
  *

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 1533 bytes --]

On Wed, Jun 09, 2004 at 11:57:43PM +0300, Pekka Enberg wrote:
> On Wed, 2004-06-09 at 23:21, Arjan van de Ven wrote:
> > how about making sure n*size doesn't overflow an int in this function?
> > We had a few security holes due to that happening a while ago; might as
> > well prevent it from happening entirely
> 
> Sure.
> 
> 		Pekka
> 
> diff -urN linux-2.6.6/include/linux/slab.h kcalloc-2.6.6/include/linux/slab.h
> --- linux-2.6.6/include/linux/slab.h	2004-06-09 22:56:11.874249056 +0300
> +++ kcalloc-2.6.6/include/linux/slab.h	2004-06-09 23:03:10.597593432 +0300
> @@ -95,6 +95,7 @@
>  	return __kmalloc(size, flags);
>  }
>  
> +extern void *kcalloc(size_t, size_t, int);
>  extern void kfree(const void *);
>  extern unsigned int ksize(const void *);
>  
> diff -urN linux-2.6.6/mm/slab.c kcalloc-2.6.6/mm/slab.c
> --- linux-2.6.6/mm/slab.c	2004-06-09 22:59:13.081701336 +0300
> +++ kcalloc-2.6.6/mm/slab.c	2004-06-09 23:50:06.592497136 +0300
> @@ -2332,6 +2332,25 @@
>  EXPORT_SYMBOL(kmem_cache_free);
>  
>  /**
> + * kcalloc - allocate memory for an array. The memory is set to zero.
> + * @n: number of elements.
> + * @size: element size.
> + * @flags: the type of memory to allocate.
> + */
> +void *kcalloc(size_t n, size_t size, int flags)
> +{
> +	if (n != 0 && size > INT_MAX / n)
> +		return NULL;
> +
> +	void *ret = kmalloc(n * size, flags);
> +	if (ret)
> +		memset(ret, 0, n * size);
> +	return ret;
> +}

ok I like it ;)

only question is what n==0 means, might as well short-circuit that but it's
optional 

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Chris Wright]

* Pekka Enberg (penberg@cs.helsinki.fi) wrote:
> +void *kcalloc(size_t n, size_t size, int flags)
> +{
> +	if (n != 0 && size > INT_MAX / n)
> +		return NULL;

Yup, I forgot to add divide by zero check.

> +	void *ret = kmalloc(n * size, flags);

This is only C99, and will make some compilers spit up warning.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Pekka Enberg]

On Wed, Jun 09, 2004 at 11:57:43PM +0300, Pekka Enberg wrote:
> > +void *kcalloc(size_t n, size_t size, int flags)
> > +{
> > +	if (n != 0 && size > INT_MAX / n)
> > +		return NULL;
> > +
> > +	void *ret = kmalloc(n * size, flags);
> > +	if (ret)
> > +		memset(ret, 0, n * size);
> > +	return ret;
> > +}

On Wed, 2004-06-09 at 23:59, Arjan van de Ven wrote:
> ok I like it ;)
> 
> only question is what n==0 means, might as well short-circuit that but it's
> optional 

Nah, I don't see the point. Now if I can only convince the ALSA guys to
switch to this... =)

		Pekka

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Pekka Enberg]

On Thu, 2004-06-10 at 00:00, Chris Wright wrote:
> > +	void *ret = kmalloc(n * size, flags);
> 
> This is only C99, and will make some compilers spit up warning.

Indeed, I'd better fix that as well then ;)

		Pekka

diff -urN linux-2.6.6/include/linux/slab.h kcalloc-2.6.6/include/linux/slab.h
--- linux-2.6.6/include/linux/slab.h	2004-06-09 22:56:11.874249056 +0300
+++ kcalloc-2.6.6/include/linux/slab.h	2004-06-09 23:03:10.597593432 +0300
@@ -95,6 +95,7 @@
 	return __kmalloc(size, flags);
 }
 
+extern void *kcalloc(size_t, size_t, int);
 extern void kfree(const void *);
 extern unsigned int ksize(const void *);
 
diff -urN linux-2.6.6/mm/slab.c kcalloc-2.6.6/mm/slab.c
--- linux-2.6.6/mm/slab.c	2004-06-09 22:59:13.081701336 +0300
+++ kcalloc-2.6.6/mm/slab.c	2004-06-10 00:08:44.004624672 +0300
@@ -2332,6 +2332,27 @@
 EXPORT_SYMBOL(kmem_cache_free);
 
 /**
+ * kcalloc - allocate memory for an array. The memory is set to zero.
+ * @n: number of elements.
+ * @size: element size.
+ * @flags: the type of memory to allocate.
+ */
+void *kcalloc(size_t n, size_t size, int flags)
+{
+	void *ret = NULL;
+
+	if (n != 0 && size > INT_MAX / n)
+		return ret;
+
+	ret = kmalloc(n * size, flags);
+	if (ret)
+		memset(ret, 0, n * size);
+	return ret;
+}
+
+EXPORT_SYMBOL(kcalloc);
+
+/**
  * kfree - free previously allocated memory
  * @objp: pointer returned by kmalloc.
  *

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[dean gaudet]

On Wed, 9 Jun 2004, Pekka Enberg wrote:

> +void *kcalloc(size_t n, size_t size, int flags)
> +{
> +	if (n != 0 && size > INT_MAX / n)
> +		return NULL;

division isn't very efficient :)  it's typically a 40+ cycle operation.

there's almost certainly more efficient ways to do this with per-target
assembly... but you should probably just crib the code from glibc which
avoids division for small allocations:

  /* size_t is unsigned so the behavior on overflow is defined.  */
  bytes = n * elem_size;
#define HALF_INTERNAL_SIZE_T \
  (((INTERNAL_SIZE_T) 1) << (8 * sizeof (INTERNAL_SIZE_T) / 2))
  if (__builtin_expect ((n | elem_size) >= HALF_INTERNAL_SIZE_T, 0)) {
    if (elem_size != 0 && bytes / elem_size != n) {
      MALLOC_FAILURE_ACTION;
      return 0;
    }
  }

-dean

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Dave Jones]

On Wed, Jun 09, 2004 at 10:08:34PM -0700, dean gaudet wrote:

 > > +void *kcalloc(size_t n, size_t size, int flags)
 > > +{
 > > +	if (n != 0 && size > INT_MAX / n)
 > > +		return NULL;
 > 
 > division isn't very efficient :)  it's typically a 40+ cycle operation.
 >
 > there's almost certainly more efficient ways to do this with per-target
 > assembly... but you should probably just crib the code from glibc which
 > avoids division for small allocations:
 > 
 >   /* size_t is unsigned so the behavior on overflow is defined.  */
 >   bytes = n * elem_size;
 > #define HALF_INTERNAL_SIZE_T \
 >   (((INTERNAL_SIZE_T) 1) << (8 * sizeof (INTERNAL_SIZE_T) / 2))
 >   if (__builtin_expect ((n | elem_size) >= HALF_INTERNAL_SIZE_T, 0)) {
 >     if (elem_size != 0 && bytes / elem_size != n) {
 >       MALLOC_FAILURE_ACTION;
 >       return 0;
 >     }
 >   }

Considering this isn't exactly a cycle-critical piece of code,
I'd choose readability over saving 40 cycles which is probably
lost in the noise of every other operation we do in that allocation.

		Dave

Re: [PATCH] ALSA: Remove subsystem-specific malloc (1/8)

[Takashi Iwai]

At Thu, 10 Jun 2004 00:07:49 +0300,
Pekka Enberg wrote:
> 
> On Wed, Jun 09, 2004 at 11:57:43PM +0300, Pekka Enberg wrote:
> > > +void *kcalloc(size_t n, size_t size, int flags)
> > > +{
> > > +	if (n != 0 && size > INT_MAX / n)
> > > +		return NULL;
> > > +
> > > +	void *ret = kmalloc(n * size, flags);
> > > +	if (ret)
> > > +		memset(ret, 0, n * size);
> > > +	return ret;
> > > +}
> 
> On Wed, 2004-06-09 at 23:59, Arjan van de Ven wrote:
> > ok I like it ;)
> > 
> > only question is what n==0 means, might as well short-circuit that but it's
> > optional 
> 
> Nah, I don't see the point. Now if I can only convince the ALSA guys to
> switch to this... =)

I don't think the shortcut is necessary, too.
By calling kmalloc with 0, the same behavior as kmalloc is
guaranteed.

BTW, I agree with conversion to the standard calloc() style :)


thanks,

Takashi

Re: ALSA: Remove subsystem-specific malloc (1/8)

[Pekka J Enberg]

Hi, 

On 6/11/2004, "Takashi Iwai" <tiwai@suse.de> wrote:
> I don't think the shortcut is necessary, too.
> By calling kmalloc with 0, the same behavior as kmalloc is
> guaranteed.

Agreed. I'm wondering if the overflow case should set size to zero and call 
kmalloc() as well. 

		Pekka 

Re: ALSA: Remove subsystem-specific malloc (1/8)

[Pekka J Enberg]

Pekka J Enberg writes:
> Agreed. I'm wondering if the overflow case should set size to zero and 
> call kmalloc() as well. 

Ehh... obviously not. The overflow case should _always_ fail whereas 
zero-order allocation can do whatever it wants. Sorry for the noise. 

       Pekka