Re: [PATCH v2 3/7] leds: add driver for LEDs from qnap-mcu devices

["Heiko Stübner" <heiko@sntech.de>]

Hi Florian,

Am Montag, 29. Juli 2024, 08:24:26 CEST schrieb Florian Eckert:
> > +static int qnap_mcu_register_err_led(struct device *dev, struct
> > qnap_mcu *mcu, int num)
> > +{
> > +	struct qnap_mcu_err_led *err_led;
> > +	char tmp_buf[LED_MAX_NAME_SIZE];
> > +	int ret;
> > +
> > +	err_led = devm_kzalloc(dev, sizeof(*err_led), GFP_KERNEL);
> > +	if (!err_led)
> > +		return -ENOMEM;
> > +
> > +	err_led->mcu = mcu;
> > +	err_led->num = num;
> > +	err_led->mode = QNAP_MCU_ERR_LED_OFF;
> > +
> > +	snprintf(tmp_buf, LED_MAX_NAME_SIZE, "hdd%d:red:status", num + 1);
> > +	err_led->cdev.name = tmp_buf;
> 
> Should not the memory have to be allocated here via 'kzalloc' for 
> 'err_led->cdev.name'?
> After leaving the function, tmp_buf is no longer on the stack?

Reading the led_classdev_register thing, cdev->name is used only for
creating the final-name for the LED and thus copied into yet another
temporary buffer [0] .

And cdev->name is not accessed anymore outside the register function.

But thinking more about that, you're still right, because after registering
cdev->name is in a bad state, pointing to something no valid anymore.
So if the LED core changes behaviour in the future, this will cause breakage.

So I'll change that.


Thanks for catching that
Heiko



[0] https://elixir.bootlin.com/linux/v6.10.1/source/drivers/leds/led-class.c#L500
https://elixir.bootlin.com/linux/v6.10.1/source/drivers/leds/led-class.c#L503