Hopefully, kmalloc() will always succeed, but if it doesn't then....

Hopefully, kmalloc() will always succeed, but if it doesn't then....

[Amit Choudhary]

Hi All,

Hopefully, kmalloc() should always succeed. But if it doesn't then the kernel is going to crash
left and right. But I guess that there's no point in running if there is no memory. Anyways, here
is what I found at many places:

func()
{
   char *a; 
   char *b;

   a = kmalloc(10, GFP_KERNEL);
   if (!a)
       goto error;

   b = kmalloc(10, GFP_KERNEL);
   if (!b)
       goto error;

 error:
  kfree(a);
  kfree(b);

}

So, if memory allocation to 'a' fails, it is going to kfree 'b'. But since 'b' is not initialized,
 kfree may crash (unless DEBUG is defined).

I have seen the same case at many places when allocating in a loop.

Regards,
Amit 


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Hopefully, kmalloc() will always succeed, but if it doesn't then....

[Pekka Enberg]

On 10/22/06, Amit Choudhary <amit2030@yahoo.com> wrote:
> So, if memory allocation to 'a' fails, it is going to kfree 'b'. But since 'b'
> is not initialized, kfree may crash (unless DEBUG is defined).
>
> I have seen the same case at many places when allocating in a loop.

So you found a bug. Why not send a patch to fix it?

Re: Hopefully, kmalloc() will always succeed, but if it doesn't then....

[Jan Engelhardt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 970 bytes --]


>> So, if memory allocation to 'a' fails, it is going to kfree 'b'. But since
>> 'b'
>> is not initialized, kfree may crash (unless DEBUG is defined).

... in which case we will be notified:

$ cat test.c
#include <linux/slab.h>

void func(void) {
    char *a, *b;
    if((a = kmalloc(10, GFP_KERNEL)) == NULL)
        goto err;
    if((b = kmalloc(10, GFP_KERNEL)) == NULL)
        goto err;

 err:
    kfree(a);
    kfree(b);
    return;
}

$ make -C /erk/kernel/linux-2.6.19-rc2 M=$PWD
  CC [M]  /dev/shm/test.o
/dev/shm/test.c: In function ‘func’:
/dev/shm/test.c:4: warning: ‘b’ may be used uninitialized in this 
function


Compared to the whole source tree, the kernel has very few "may be 
uninitialized" spots. And stochastically, it is quite unlikely that all 
of them are caused by a construct like the above.


>> I have seen the same case at many places when allocating in a loop.
>
> So you found a bug. Why not send a patch to fix it?


	-`J'
-- 

Re: Hopefully, kmalloc() will always succeed, but if it doesn't then....

[Amit Choudhary]

--- Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> On 10/22/06, Amit Choudhary <amit2030@yahoo.com> wrote:
> > So, if memory allocation to 'a' fails, it is going to kfree 'b'. But since 'b'
> > is not initialized, kfree may crash (unless DEBUG is defined).
> >
> > I have seen the same case at many places when allocating in a loop.
> 
> So you found a bug. Why not send a patch to fix it?
> 

Yes, I will send it.

Regards,
Amit


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Hopefully, kmalloc() will always succeed, but if it doesn't then....

[Amit Choudhary]

--- Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:

> 
> $ make -C /erk/kernel/linux-2.6.19-rc2 M=$PWD
>   CC [M]  /dev/shm/test.o
> /dev/shm/test.c: In function ‘func’:
> /dev/shm/test.c:4: warning: ‘b’ may be used uninitialized in this 
> function
> 

It would be nice if this warning can be seen in all the cases without doing anything extra. But
sometimes I do not see it.

I compiled sound/pci/mixart/mixart_hwdep.c - did "make modules".

It has the following code but I did not get any warnings.

static int mixart_enum_connectors(struct mixart_mgr *mgr)
{
        u32 k;
        int err;
        struct mixart_msg request;
        struct mixart_enum_connector_resp *connector;
        struct mixart_audio_info_req  *audio_info_req;
        struct mixart_audio_info_resp *audio_info;

        connector = kmalloc(sizeof(*connector), GFP_KERNEL);
        audio_info_req = kmalloc(sizeof(*audio_info_req), GFP_KERNEL);
        audio_info = kmalloc(sizeof(*audio_info), GFP_KERNEL);
        if (! connector || ! audio_info_req || ! audio_info) {
                err = -ENOMEM;
                goto __error;
        }

root@zephyr-7 linux-2.6.19-rc1]# make modules
scripts/kconfig/conf -s arch/i386/Kconfig
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  ...
  CC [M]  sound/pci/mixart/mixart.o
  CC [M]  sound/pci/mixart/mixart_core.o
  CC [M]  sound/pci/mixart/mixart_hwdep.o
  CC [M]  sound/pci/mixart/mixart_mixer.o
  LD [M]  sound/pci/mixart/snd-mixart.o
  Building modules, stage 2.
  MODPOST 44 modules
  CC      sound/core/snd-hwdep.mod.o
  LD [M]  sound/core/snd-hwdep.ko
  ...
[root@zephyr-7 linux-2.6.19-rc1]#

Regards,
Amit



__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: Hopefully, kmalloc() will always succeed, but if it doesn't then....

[Roland Dreier]

 >         struct mixart_enum_connector_resp *connector;
 >         struct mixart_audio_info_req  *audio_info_req;
 >         struct mixart_audio_info_resp *audio_info;
 > 
 >         connector = kmalloc(sizeof(*connector), GFP_KERNEL);
 >         audio_info_req = kmalloc(sizeof(*audio_info_req), GFP_KERNEL);
 >         audio_info = kmalloc(sizeof(*audio_info), GFP_KERNEL);
 >         if (! connector || ! audio_info_req || ! audio_info) {
 >                 err = -ENOMEM;
 >                 goto __error;
 >         }

This is not a bug.  All of the pointers are initialized, and if
kmalloc() fails, then one of them will be set to NULL.  However,
kfree(NULL) is a perfectly fine thing to do (kfree just returns
immediately in this case).

So this is just a way of saving some tests and optimizing for the
common case when all allocations succeed.  In other words, this is
good code -- although the spacing is slightly bogus: it should be

        if (!connector || !audio_info_req || !audio_info) {

and also using __error as a label is slightly silly -- why not just
make it "error"?

 - R.

Re: Hopefully, kmalloc() will always succeed, but if it doesn't then....

[Amit Choudhary]

--- Roland Dreier <rdreier@cisco.com> wrote:

>  >         struct mixart_enum_connector_resp *connector;
>  >         struct mixart_audio_info_req  *audio_info_req;
>  >         struct mixart_audio_info_resp *audio_info;
>  > 
>  >         connector = kmalloc(sizeof(*connector), GFP_KERNEL);
>  >         audio_info_req = kmalloc(sizeof(*audio_info_req), GFP_KERNEL);
>  >         audio_info = kmalloc(sizeof(*audio_info), GFP_KERNEL);
>  >         if (! connector || ! audio_info_req || ! audio_info) {
>  >                 err = -ENOMEM;
>  >                 goto __error;
>  >         }
> 
> This is not a bug.  All of the pointers are initialized, and if

Yes, this is not a bug. Although the case for arrays go unnoticed by gcc. Something like this:

        char *abcd[20];
        int i;

        for (i = 0; i < 20; i++) {
                abcd[i] = kmalloc(10, GFP_KERNEL);
                if (!abcd[i])
                        goto error;
        }
 error:
        for (i = 0; i < 20; i++)
                kfree(abcd[i]);

-Amit

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com