Re: [patch -mm] alsa mixer_oss kfree fix

[young dave]

Hi, Andrew



kfree(NULL) is legal, and is often used.



Apart from the null pointer, IMHO,there's two problem need to be
fixed, I'm not sure.

What's your opinion?

1. the label indent should be removed
2. similar code use different label, some use " __unlock" but others
use "__unalloc", "__unlock" seems to be misspelled.

Regards
dave
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch -mm] alsa mixer_oss kfree fix

[young dave]

Hi,
Just append to my email.

The reason I read mixer_oss.c is that I see oops in 2.6.22-rc1-mm1,
seems oops in mixer_oss.c, but the oops rised only once, I found
rc3-mm1 have no change in this file,
so I read the source , then post this patch.

please find the oops message below:


BUG: unable to handle kernel NULL pointer dereference at virtual
address 0088
printing eip:
c0162bc2
*pde = 
Oops:  [#1]
PREEMPT
Modules linked in: snd_mixer_oss snd_hda_intel snd_pcm snd_timer
snd_page_alloc snd soundcore ipv6 capability commoncap e100 mi
i agpgart pcspkr psmouse
CPU:0
EIP:0060:[]Not tainted VLI
EFLAGS: 00010087   (2.6.22-rc1-mm1 #7)
EIP is at kfree+0x42/0x90
eax:    ebx: c10568e0   ecx: c2b47000   edx: c01588d2
esi: 0287   edi:    ebp: c2864000   esp: c2865f58
ds: 007b   es: 007b   fs:   gs: 0033  ss: 0068
Process modprobe (pid: 2963, ti=c2864000 task=c1ede540 task.ti=c2864000)
Stack: 0011 f8855000 c0520cb4 0001 0001 c1e524a0 c1051bc0 c01588d2
  0805c348 0001 c2864000 f885502b f8840a00  0001 c013caf1
  4b79  000b7f68  b7f44840 08065f88 0805c348 c01040bc
Call Trace:
[] alsa_mixer_oss_init+0x0/0x37 [snd_mixer_oss]
[] __vunmap+0xb2/0xe0
[] alsa_mixer_oss_init+0x2b/0x37 [snd_mixer_oss]
[] sys_init_module+0xa1/0x140
[] syscall_call+0x7/0xb
[] packet_seq_start+0x30/0x60
===
Code: 00 00 00 40 a1 40 6a 59 c0 c1 ea 0c c1 e2 05 01 c2 8b 02 89 d3
25 00 40 02 00 3d 00 40 02 00 74 45 8b 7b 10 8b 54 24 1c 9
c 5e fa <39> 9f 88 00 00 00 75 25 8b 03 a8 02 75 1f 0f b7 53 0a 8b 43 0c
EIP: [] kfree+0x42/0x90 SS:ESP 0068:c2865f58
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch -mm] alsa mixer_oss kfree fix

[young dave]

Hi,


kfree(NULL) is legal, and is often used.


Really?  I don't know this before.

Regards
dave
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [patch -mm] alsa mixer_oss kfree fix

[Andrew Morton]

On Fri, 1 Jun 2007 01:53:39 + "young dave" <[EMAIL PROTECTED]> wrote:

> Fix possible null pointer kfree.

kfree(NULL) is legal, and is often used.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[patch -mm] alsa mixer_oss kfree fix

[young dave]

Hi,
Fix possible null pointer kfree.

Signed-off-by: dave young <[EMAIL PROTECTED]>

mixer_oss.c |120 +-
1 file changed, 74 insertions(+), 46 deletions(-)

diff -purN linux/sound/core/oss/mixer_oss.c linux.new/sound/core/oss/mixer_oss.c
--- linux/sound/core/oss/mixer_oss.c2007-06-01 09:00:12.0 +
+++ linux.new/sound/core/oss/mixer_oss.c2007-06-01 09:36:10.0 
+
@@ -512,23 +512,27 @@ static void snd_mixer_oss_get_volume1_vo
return;
}
uinfo = kzalloc(sizeof(*uinfo), GFP_KERNEL);
+   if (uinfo == NULL)
+   goto out_uinfo;
uctl = kzalloc(sizeof(*uctl), GFP_KERNEL);
-   if (uinfo == NULL || uctl == NULL)
-   goto __unalloc;
+   if (uctl == NULL)
+   goto out_uctl;
if (kctl->info(kctl, uinfo))
-   goto __unalloc;
+   goto out;
if (kctl->get(kctl, uctl))
-   goto __unalloc;
+   goto out;
if (uinfo->type == SNDRV_CTL_ELEM_TYPE_BOOLEAN &&
uinfo->value.integer.min == 0 && uinfo->value.integer.max == 1)
-   goto __unalloc;
+   goto out;
*left = snd_mixer_oss_conv1(uctl->value.integer.value[0],
uinfo->value.integer.min, uinfo->value.integer.max,
&pslot->volume[0]);
if (uinfo->count > 1)
*right = snd_mixer_oss_conv1(uctl->value.integer.value[1],
uinfo->value.integer.min, uinfo->value.integer.max,
&pslot->volume[1]);
-  __unalloc:
+out:
+   kfree(uctl);
+out_uctl:
+   kfree(uinfo);
+out_uinfo:
up_read(&card->controls_rwsem);
-   kfree(uctl);
-   kfree(uinfo);
}

static void snd_mixer_oss_get_volume1_sw(struct snd_mixer_oss_file *fmixer,
@@ -550,13 +554,15 @@ static void snd_mixer_oss_get_volume1_sw
return;
}
uinfo = kzalloc(sizeof(*uinfo), GFP_KERNEL);
+   if (uinfo == NULL)
+   goto out_uinfo;
uctl = kzalloc(sizeof(*uctl), GFP_KERNEL);
-   if (uinfo == NULL || uctl == NULL)
-   goto __unalloc;
+   if (uctl == NULL)
+   goto out_uctl;
if (kctl->info(kctl, uinfo))
-   goto __unalloc;
+   goto out;
if (kctl->get(kctl, uctl))
-   goto __unalloc;
+   goto out;
if (!uctl->value.integer.value[0]) {
*left = 0;
if (uinfo->count == 1)
@@ -564,10 +570,12 @@ static void snd_mixer_oss_get_volume1_sw
}
if (uinfo->count > 1 && !uctl->value.integer.value[route ? 3 : 1])
*right = 0;
-  __unalloc:
-   up_read(&card->controls_rwsem);
-   kfree(uctl);
+out:
+   kfree(uctl);
+out_uctl:
kfree(uinfo);
+out_uinfo:
+   up_read(&card->controls_rwsem);
}

static int snd_mixer_oss_get_volume1(struct snd_mixer_oss_file *fmixer,
@@ -613,25 +621,29 @@ static void snd_mixer_oss_put_volume1_vo
if ((kctl = snd_ctl_find_numid(card, numid)) == NULL)
return;
uinfo = kzalloc(sizeof(*uinfo), GFP_KERNEL);
+   if (uinfo == NULL)
+   goto out_uinfo;
uctl = kzalloc(sizeof(*uctl), GFP_KERNEL);
-   if (uinfo == NULL || uctl == NULL)
-   goto __unalloc;
+   if (uctl == NULL)
+   goto out_uctl;
if (kctl->info(kctl, uinfo))
-   goto __unalloc;
+   goto out;
if (uinfo->type == SNDRV_CTL_ELEM_TYPE_BOOLEAN &&
uinfo->value.integer.min == 0 && uinfo->value.integer.max == 1)
-   goto __unalloc;
+   goto out;
uctl->value.integer.value[0] = snd_mixer_oss_conv2(left,
uinfo->value.integer.min, uinfo->value.integer.max);
if (uinfo->count > 1)
uctl->value.integer.value[1] = snd_mixer_oss_conv2(right,
uinfo->value.integer.min, uinfo->value.integer.max);
if ((res = kctl->put(kctl, uctl)) < 0)
-   goto __unalloc;
+   goto out;
if (res > 0)
snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
-  __unalloc:
-   up_read(&card->controls_rwsem);
-   kfree(uctl);
+out:
+   kfree(uctl);
+out_uctl:
kfree(uinfo);
+out_uinfo:
+   up_read(&card->controls_rwsem);
}

static void snd_mixer_oss_put_volume1_sw(struct snd_mixer_oss_file *fmixer,
@@ -654,11 +666,13 @@ static void snd_mixer_oss_put_volume1_sw
return;
}
uinfo = kzalloc(sizeof(*uinfo), GFP_KERNEL);
+   if (uinfo == NULL)
+   goto out_uinfo;
uctl = kzalloc(sizeof(*uctl), GFP_KERNEL);
-   if (uinfo == NULL || uctl == NULL)
-   goto __unalloc;
+   if (uctl == NULL)
+   goto out_uctl;
if (kctl->info(kctl, uinfo))
-   goto __unalloc;
+   goto out;
if (uinfo->count > 1) {
uctl->value.integer.value[0] = left > 0 ? 1 : 0;
uctl->value.integer.value[route ? 3 : 1] = ri