Re: [PATCH v3] gpiolib: fix memory leak in gpiochip_setup_dev()

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>]

On Thu, Nov 17, 2022 at 05:02:47PM +0800, Zeng Heng wrote:
> Here is a report about memory leak detected in gpiochip_setup_dev():
> 
> unreferenced object 0xffff88810b406400 (size 512):
>   comm "python3", pid 1682, jiffies 4295346908 (age 24.090s)
>   backtrace:
>     kmalloc_trace
>     device_add 		device_private_init at drivers/base/core.c:3361

Seems like unneeded space after device_add. Also note, we refer to
the functions as func().

> 			(inlined by) device_add at drivers/base/core.c:3411
>     cdev_device_add
>     gpiolib_cdev_register
>     gpiochip_setup_dev
>     gpiochip_add_data_with_key
> 
> gcdev_register() & gcdev_unregister() would call device_add() &
> device_del() (no matter CONFIG_GPIO_CDEV is enabled or not) to
> register/unregister device.
> 
> However, if device_add() succeeds, some resource (like
> struct device_private allocated by device_private_init())
> is not released by device_del().
> 
> Therefore, after device_add() succeeds by gcdev_register(), it
> needs to call put_device() to release resource in the error handle
> path.
> 
> Here we move forward the register of release function, and let it
> release every piece of resource by put_device() instead of kfree().
> 
> Fixes: 159f3cd92f17 ("gpiolib: Defer gpio device setup until after gpiolib initialization")
> Signed-off-by: Zeng Heng <zengheng4@xxxxxxxxxx>
> ---

Where is changelog since we see this as v3?

...

>  err_free_gpiochip_mask:
>  	gpiochip_remove_pin_ranges(gc);
>  	gpiochip_free_valid_mask(gc);
> +	/*
> +	 * If gdev->dev.release has been registered by
> +	 * gpiochip_setup_dev(), print err msg and
> +	 * call put_device() to release all.
> +	 */
> +	if (gdev->dev.release)
> +		goto err_free_gdev;

(1)

>  err_remove_from_list:
>  	spin_lock_irqsave(&gpio_lock, flags);
>  	list_del(&gdev->list);

...

> -	kfree(gdev);

> +	if (gdev->dev.release)
> +		put_device(&gdev->dev);

Why you can't do this above at (1)?
Is there any other hidden way to get here with release set?

> +	else
> +		kfree(gdev);
>  	return ret;

-- 
With Best Regards,
Andy Shevchenko