| Message ID | 20220513081832.263863-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leds: lm3601x: Don't use mutex after it was destroyed | expand |
Hello Pavel, On Fri, May 13, 2022 at 04:36:57PM +0200, Uwe Kleine-König wrote: > On Fri, May 13, 2022 at 04:02:55PM +0200, Pavel Machek wrote: > > > The mutex might still be in use until the devm cleanup callback > > > devm_led_classdev_flash_release() is called. This only happens some time > > > after lm3601x_remove() completed. > > > > I'm sure lots of "use after free" can be fixed by simply removing the > > resource freeing... > > I agree in general. Mutexes are a bit special here and often are not > destroyed. mutex_destroy() is a no-op usually and with debugging enabled > only does > > lock->magic = NULL; > > which catches use-after-destroy. So IMHO just dropping the mutex_destroy > is fine. > > > but lets fix this properly. > > I don't understand that part. Does that mean you pick up my patch, or > that you create a better fix? You didn't pick up this patch up to now and also I didn't see a better fix. So I wonder what is your plan/vision here. The obvious alternatives are to call mutex_destroy only in a devm callback that is registered before calling lm3601x_register_leds(), or don't used devm to register the LED. Best regards Uwe
Hello Pavel, On Mon, May 23, 2022 at 09:35:10AM +0200, Uwe Kleine-König wrote: > On Fri, May 13, 2022 at 04:36:57PM +0200, Uwe Kleine-König wrote: > > On Fri, May 13, 2022 at 04:02:55PM +0200, Pavel Machek wrote: > > > > The mutex might still be in use until the devm cleanup callback > > > > devm_led_classdev_flash_release() is called. This only happens some time > > > > after lm3601x_remove() completed. > > > > > > I'm sure lots of "use after free" can be fixed by simply removing the > > > resource freeing... > > > > I agree in general. Mutexes are a bit special here and often are not > > destroyed. mutex_destroy() is a no-op usually and with debugging enabled > > only does > > > > lock->magic = NULL; > > > > which catches use-after-destroy. So IMHO just dropping the mutex_destroy > > is fine. > > > > > but lets fix this properly. > > > > I don't understand that part. Does that mean you pick up my patch, or > > that you create a better fix? > > You didn't pick up this patch up to now and also I didn't see a better > fix. > > So I wonder what is your plan/vision here. The obvious alternatives are > to call mutex_destroy only in a devm callback that is registered before > calling lm3601x_register_leds(), or don't used devm to register the LED. Any news on this? If you're waiting for a better fix from me, please tell my your expectations. A devm variant of mutex_init would be an option, but that feels like a very big hammer for a small nail. I still consider my patch fine. Best regards Uwe
diff --git a/drivers/leds/flash/leds-lm3601x.c b/drivers/leds/flash/leds-lm3601x.c index d0e1d4814042..3d1272748201 100644 --- a/drivers/leds/flash/leds-lm3601x.c +++ b/drivers/leds/flash/leds-lm3601x.c @@ -444,8 +444,6 @@ static int lm3601x_remove(struct i2c_client *client) { struct lm3601x_led *led = i2c_get_clientdata(client); - mutex_destroy(&led->lock); - return regmap_update_bits(led->regmap, LM3601X_ENABLE_REG, LM3601X_ENABLE_MASK, LM3601X_MODE_STANDBY);
The mutex might still be in use until the devm cleanup callback devm_led_classdev_flash_release() is called. This only happens some time after lm3601x_remove() completed. Fixes: e63a744871a3 ("leds: lm3601x: Convert class registration to device managed") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/leds/flash/leds-lm3601x.c | 2 -- 1 file changed, 2 deletions(-)