[bug report] ALSA: hda - Don't register a cb func if it is registered already
Hello Hui Wang,
The patch f4794c6064a8: "ALSA: hda - Don't register a cb func if it is registered already" from Sep 30, 2020, leads to the following static checker warning:
sound/pci/hda/patch_sigmatel.c:3075 stac92hd71bxx_fixup_hp_m4() warn: 'jack' can also be NULL
sound/pci/hda/patch_sigmatel.c 3069 /* Enable VREF power saving on GPIO1 detect */ 3070 snd_hda_codec_write_cache(codec, codec->core.afg, 0, 3071 AC_VERB_SET_GPIO_UNSOLICITED_RSP_MASK, 0x02); 3072 jack = snd_hda_jack_detect_enable_callback(codec, codec->core.afg, 3073 stac_vref_event);
Originally snd_hda_jack_detect_enable_callback() would not return NULL here.
3074 if (!IS_ERR(jack)) 3075 jack->private_data = 0x02; 3076 3077 spec->gpio_mask |= 0x02;
But now we have this:
sound/pci/hda/hda_jack.c 301 struct hda_jack_callback * 302 snd_hda_jack_detect_enable_callback_mst(struct hda_codec *codec, hda_nid_t nid, 303 int dev_id, hda_jack_callback_fn func) 304 { 305 struct hda_jack_tbl *jack; 306 struct hda_jack_callback *callback = NULL; 307 int err; 308 309 jack = snd_hda_jack_tbl_new(codec, nid, dev_id); 310 if (!jack) 311 return ERR_PTR(-ENOMEM); 312 if (func && !func_is_already_in_callback_list(jack, func)) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ We only allocate callback if there isn't one already.
313 callback = kzalloc(sizeof(*callback), GFP_KERNEL); 314 if (!callback) 315 return ERR_PTR(-ENOMEM); 316 callback->func = func; 317 callback->nid = jack->nid; 318 callback->dev_id = jack->dev_id; 319 callback->next = jack->callback; 320 jack->callback = callback; 321 } 322 323 if (jack->jack_detect) 324 return callback; /* already registered */ ^^^^^^^^^^^^^^^ So presumably this should be jack->callback
325 jack->jack_detect = 1; 326 if (codec->jackpoll_interval > 0) 327 return callback; /* No unsol if we're polling instead */ ^^^^^^^^^^^^^^^^
328 err = snd_hda_codec_write_cache(codec, nid, 0, 329 AC_VERB_SET_UNSOLICITED_ENABLE, 330 AC_USRSP_EN | jack->tag); 331 if (err < 0) 332 return ERR_PTR(err); 333 return callback; ^^^^^^^^^^^^^^^^ And these as well.
334 }
regards, dan carpenter
On 10/21/20 8:19 PM, Dan Carpenter wrote:
Hello Hui Wang,
The patch f4794c6064a8: "ALSA: hda - Don't register a cb func if it is registered already" from Sep 30, 2020, leads to the following static checker warning:
sound/pci/hda/patch_sigmatel.c:3075 stac92hd71bxx_fixup_hp_m4() warn: 'jack' can also be NULL
sound/pci/hda/patch_sigmatel.c 3069 /* Enable VREF power saving on GPIO1 detect */ 3070 snd_hda_codec_write_cache(codec, codec->core.afg, 0, 3071 AC_VERB_SET_GPIO_UNSOLICITED_RSP_MASK, 0x02); 3072 jack = snd_hda_jack_detect_enable_callback(codec, codec->core.afg, 3073 stac_vref_event);
Originally snd_hda_jack_detect_enable_callback() would not return NULL here.
3074 if (!IS_ERR(jack)) 3075 jack->private_data = 0x02; 3076 3077 spec->gpio_mask |= 0x02;
But now we have this:
sound/pci/hda/hda_jack.c 301 struct hda_jack_callback * 302 snd_hda_jack_detect_enable_callback_mst(struct hda_codec *codec, hda_nid_t nid, 303 int dev_id, hda_jack_callback_fn func) 304 { 305 struct hda_jack_tbl *jack; 306 struct hda_jack_callback *callback = NULL; 307 int err; 308 309 jack = snd_hda_jack_tbl_new(codec, nid, dev_id); 310 if (!jack) 311 return ERR_PTR(-ENOMEM); 312 if (func && !func_is_already_in_callback_list(jack, func)) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ We only allocate callback if there isn't one already.
Yes, indeed, this is a problem.
313 callback = kzalloc(sizeof(*callback), GFP_KERNEL); 314 if (!callback) 315 return ERR_PTR(-ENOMEM); 316 callback->func = func; 317 callback->nid = jack->nid; 318 callback->dev_id = jack->dev_id; 319 callback->next = jack->callback; 320 jack->callback = callback; 321 } 322 323 if (jack->jack_detect) 324 return callback; /* already registered */ ^^^^^^^^^^^^^^^So presumably this should be jack->callback
Looks like it is also not correct to return the jack->callback. Need to take some time to write a fix for it.
Thanks for reporting the issue.
325 jack->jack_detect = 1; 326 if (codec->jackpoll_interval > 0) 327 return callback; /* No unsol if we're polling instead */ ^^^^^^^^^^^^^^^^ 328 err = snd_hda_codec_write_cache(codec, nid, 0, 329 AC_VERB_SET_UNSOLICITED_ENABLE, 330 AC_USRSP_EN | jack->tag); 331 if (err < 0) 332 return ERR_PTR(err); 333 return callback; ^^^^^^^^^^^^^^^^And these as well.
334 }regards, dan carpenter
Looks like this will not bring problem on patch_sigmatel.c, NULL and valid kernel pointer are same for IS_ERR(), they will not make IS_ERR() come true.
On 10/21/20 10:21 PM, Hui Wang wrote:
On 10/21/20 8:19 PM, Dan Carpenter wrote:
Hello Hui Wang,
The patch f4794c6064a8: "ALSA: hda - Don't register a cb func if it is registered already" from Sep 30, 2020, leads to the following static checker warning:
sound/pci/hda/patch_sigmatel.c:3075 stac92hd71bxx_fixup_hp_m4() warn: 'jack' can also be NULL
sound/pci/hda/patch_sigmatel.c 3069 /* Enable VREF power saving on GPIO1 detect */ 3070 snd_hda_codec_write_cache(codec, codec->core.afg, 0, 3071 AC_VERB_SET_GPIO_UNSOLICITED_RSP_MASK, 0x02); 3072 jack = snd_hda_jack_detect_enable_callback(codec, codec->core.afg, 3073 stac_vref_event);
Originally snd_hda_jack_detect_enable_callback() would not return NULL here.
3074 if (!IS_ERR(jack)) 3075 jack->private_data = 0x02; 3076 3077 spec->gpio_mask |= 0x02;
But now we have this:
sound/pci/hda/hda_jack.c 301 struct hda_jack_callback * 302 snd_hda_jack_detect_enable_callback_mst(struct hda_codec *codec, hda_nid_t nid, 303 int dev_id, hda_jack_callback_fn func) 304 { 305 struct hda_jack_tbl *jack; 306 struct hda_jack_callback *callback = NULL; 307 int err; 308 309 jack = snd_hda_jack_tbl_new(codec, nid, dev_id); 310 if (!jack) 311 return ERR_PTR(-ENOMEM); 312 if (func && !func_is_already_in_callback_list(jack, func)) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ We only allocate callback if there isn't one already.
Yes, indeed, this is a problem.
313 callback = kzalloc(sizeof(*callback), GFP_KERNEL); 314 if (!callback) 315 return ERR_PTR(-ENOMEM); 316 callback->func = func; 317 callback->nid = jack->nid; 318 callback->dev_id = jack->dev_id; 319 callback->next = jack->callback; 320 jack->callback = callback; 321 } 322 323 if (jack->jack_detect) 324 return callback; /* already registered */ ^^^^^^^^^^^^^^^ So presumably this should be jack->callback
Looks like it is also not correct to return the jack->callback. Need to take some time to write a fix for it.
Thanks for reporting the issue.
325 jack->jack_detect = 1; 326 if (codec->jackpoll_interval > 0) 327 return callback; /* No unsol if we're polling instead */ ^^^^^^^^^^^^^^^^
328 err = snd_hda_codec_write_cache(codec, nid, 0, 329 AC_VERB_SET_UNSOLICITED_ENABLE, 330 AC_USRSP_EN | jack->tag); 331 if (err < 0) 332 return ERR_PTR(err); 333 return callback; ^^^^^^^^^^^^^^^^ And these as well.
334 }
regards, dan carpenter
On Wed, 21 Oct 2020 17:03:11 +0200, Hui Wang wrote:
Looks like this will not bring problem on patch_sigmatel.c, NULL and valid kernel pointer are same for IS_ERR(), they will not make IS_ERR() come true.
Right, and it's called only once, so the bug doesn't hit.
But we should address the potential bug nevertheless, of course :)
thanks,
Takashi
On 10/21/20 11:59 PM, Takashi Iwai wrote:
On Wed, 21 Oct 2020 17:03:11 +0200, Hui Wang wrote:
Looks like this will not bring problem on patch_sigmatel.c, NULL and valid kernel pointer are same for IS_ERR(), they will not make IS_ERR() come true.
Right, and it's called only once, so the bug doesn't hit.
But we should address the potential bug nevertheless, of course :)
OK, will fix it.
Thanks.
thanks,
Takashi
On Wed, Oct 21, 2020 at 11:03:11PM +0800, Hui Wang wrote:
Looks like this will not bring problem on patch_sigmatel.c, NULL and valid kernel pointer are same for IS_ERR(), they will not make IS_ERR() come true.
Correct, but that is the problem. You can dereference a valid pointer but you can't dereference a NULL.
On 10/21/20 10:21 PM, Hui Wang wrote:
On 10/21/20 8:19 PM, Dan Carpenter wrote:
Hello Hui Wang,
The patch f4794c6064a8: "ALSA: hda - Don't register a cb func if it is registered already" from Sep 30, 2020, leads to the following static checker warning:
sound/pci/hda/patch_sigmatel.c:3075 stac92hd71bxx_fixup_hp_m4() warn: 'jack' can also be NULL
sound/pci/hda/patch_sigmatel.c 3069 /* Enable VREF power saving on GPIO1 detect */ 3070 snd_hda_codec_write_cache(codec, codec->core.afg, 0, 3071 AC_VERB_SET_GPIO_UNSOLICITED_RSP_MASK, 0x02); 3072 jack = snd_hda_jack_detect_enable_callback(codec, codec->core.afg, 3073 stac_vref_event);
Originally snd_hda_jack_detect_enable_callback() would not return NULL here.
3074 if (!IS_ERR(jack)) 3075 jack->private_data = 0x02;
So if "jack" is NULL then it will say it's not an error pointer so it will try to assign jack->private_data = 0x02; and oops.
regards, dan carpenter
On 10/22/20 1:27 AM, Dan Carpenter wrote:
On Wed, Oct 21, 2020 at 11:03:11PM +0800, Hui Wang wrote:
Looks like this will not bring problem on patch_sigmatel.c, NULL and valid kernel pointer are same for IS_ERR(), they will not make IS_ERR() come true.
Correct, but that is the problem. You can dereference a valid pointer but you can't dereference a NULL.
On 10/21/20 10:21 PM, Hui Wang wrote:
On 10/21/20 8:19 PM, Dan Carpenter wrote:
Hello Hui Wang,
The patch f4794c6064a8: "ALSA: hda - Don't register a cb func if it is registered already" from Sep 30, 2020, leads to the following static checker warning:
sound/pci/hda/patch_sigmatel.c:3075 stac92hd71bxx_fixup_hp_m4() warn: 'jack' can also be NULL
sound/pci/hda/patch_sigmatel.c 3069 /* Enable VREF power saving on GPIO1 detect */ 3070 snd_hda_codec_write_cache(codec, codec->core.afg, 0, 3071 AC_VERB_SET_GPIO_UNSOLICITED_RSP_MASK, 0x02); 3072 jack = snd_hda_jack_detect_enable_callback(codec, codec->core.afg, 3073 stac_vref_event);
Originally snd_hda_jack_detect_enable_callback() would not return NULL here.
3074 if (!IS_ERR(jack)) 3075 jack->private_data = 0x02;
So if "jack" is NULL then it will say it's not an error pointer so it will try to assign jack->private_data = 0x02; and oops.
Right. Will fix it.
Thanks
regards, dan carpenter
participants (3)
-
Dan Carpenter -
Hui Wang -
Takashi Iwai