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
On 10/22/06, Amit Choudhary <[email protected]> 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?
>> 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'
--
--- Pekka Enberg <[email protected]> wrote:
> On 10/22/06, Amit Choudhary <[email protected]> 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
--- Jan Engelhardt <[email protected]> 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
> 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.
--- Roland Dreier <[email protected]> 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