Messages in this thread | | | Date | Mon, 30 Jan 2023 15:00:32 -0800 | Subject | Re: [RFC PATCH v2 12/22] sound: usb: card: Introduce USB SND platform op callbacks | From | Wesley Cheng <> |
| |
Hi Pierre,
On 1/26/2023 7:50 AM, Pierre-Louis Bossart wrote: > > > >> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops) >> +{ >> + if (platform_ops) >> + return -EEXIST; >> + >> + platform_ops = ops; >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops); >> + >> +int snd_usb_unregister_platform_ops(void) >> +{ >> + platform_ops = NULL; >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops); > > I find this super-racy. > > If the this function is called just before ... > >> >> /* >> * disconnect streams >> @@ -910,6 +928,10 @@ static int usb_audio_probe(struct usb_interface *intf, >> usb_set_intfdata(intf, chip); >> atomic_dec(&chip->active); >> mutex_unlock(®ister_mutex); >> + >> + if (platform_ops->connect_cb) >> + platform_ops->connect_cb(intf, chip); >> + > > ... this, then you have a risk of using a dandling pointer. > > You also didn't test that the platform_ops != NULL, so there's a risk of > dereferencing a NULL pointer. > > Not so good, eh? > > It's a classic (I've had the same sort of issues with SoundWire), when > you export ops from one driver than can be removed, then additional > protection is needed when using those callbacks. > >
Yep, will take a look at this a bit more to improve it.
Thanks Wesley Cheng
| |