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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>]

On Fri, Nov 18, 2022 at 10:22:36AM +0800, Zeng Heng wrote:
> Here is a backtrace 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

You missed my comment here about extra space.

> 			(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().

...

> +static void gpiochip_print_register_fail(struct gpio_chip *gc,
> +					 struct gpio_device *gdev,
> +					 const char *func, int ret)
> +{
> +	/* failures here can mean systems won't boot... */
> +	if (ret != -EPROBE_DEFER) {

Wouldn't the following work better?

	if (ret == -EPROBE_DEFER)
		return;

	/* failures here can mean systems won't boot... */
	pr_err(...);

> +		pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", func,
> +			gdev->base, gdev->base + gdev->ngpio - 1,
> +			gc->label ? : "generic", ret);
> +	}
> +}

...

>  err_free_gpiochip_mask:
>  	gpiochip_remove_pin_ranges(gc);
>  	gpiochip_free_valid_mask(gc);
> +	if (gdev->dev.release) {

> +		/* release() has been registered by gpiochip_setup_dev() */

This comment is most probably in a wrong line and should be one line below.

> +		gpiochip_print_register_fail(gc, gdev, __func__, ret);
> +		put_device(&gdev->dev);
> +		return ret;
> +	}

...

>  err_free_gdev:
> -	/* failures here can mean systems won't boot... */
> -	if (ret != -EPROBE_DEFER) {
> -		pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__,
> -		       gdev->base, gdev->base + gdev->ngpio - 1,
> -		       gc->label ? : "generic", ret);
> -	}
> +	gpiochip_print_register_fail(gc, gdev, __func__, ret);
>  	kfree(gdev);
>  	return ret;

Now it looks cleaner, but why you can't use the same return point with the
message? What you need is to neep the range on the stack (which is almost there).

...

Okay, let's leave this for a while, I will think how it can be improved and then
I come up with particular suggestions.

-- 
With Best Regards,
Andy Shevchenko