linux-kernel - Re: [PATCH] platform: mellanox: Fix a resource leak in an error handling path in mlxplat

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 3 Oct 2023 15:05:45 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
cc:     Vadim Pasternak <vadimp@...dia.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Michael Shych <michaelsh@...dia.com>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] platform: mellanox: Fix a resource leak in an error
 handling path in mlxplat_probe()

On Sun, 1 Oct 2023, Christophe JAILLET wrote:

> If an error occurs after a successful mlxplat_i2c_main_init(),
> mlxplat_i2c_main_exit() should be called to free some resources.
> 
> Add the missing call, as already done in the remove function.
> 
> Fixes: 158cd8320776 ("platform: mellanox: Split logic in init and exit flow")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> ---
> This patch is based on comparison between functions called in the remove
> function and the error handling path of the probe.
> 
> For some reason, the way the code is written and function names are
> puzzling to me.

Indeed, pre/post are mixed up for init/exit variants which makes 
everything very confusing and the call to mlxplat_post_init() is buried 
deep into the call chain.

These would seem better names for the init/deinit with proper pairing:

- ..._logicdev_init/deinit for FPGA/CPLD init.
- ..._mainbus_init/deinit
- perhaps the rest could be just ..._platdevs_reg/unreg

Those alone would make it much easier to follow.

In addition,
- mlxplat_i2c_mux_complition_notify() looks just useless call chain level
  and should be removed (it also has a typo in its name).
- Maybe ..._platdev_reg() (currently mlxplat_post_init()) should be called 
  directly from mainbus_init() or even from .probe() and not from the
  mux topo init.

> So Review with care!

It does not look complete as also mlxplat_i2c_main_init() lacks rollback 
it should do it mlxplat_i2c_mux_topology_init() failed.

Since currently mlxplat_i2c_main_init() is what ultimately calls 
mlxplat_post_init() deep down in the call chain (unless the call to it 
gets moved out from there), it would be appropriate for 
mlxplat_i2c_main_exit() to do/call the cleanup.  And neither .probe() nor 
.remove() should call mlxplat_pre_exit() directly in that case.

So two alternative ways forward for the fix before all the renaming:

1) Move mlxplat_post_init() call out of its current place into .probe() 
   and make the rollback path there to match that.
2) Do cleanup properly in mlxplat_i2c_main_init() and 
   mlxplat_i2c_main_exit().

I'd prefer 1) because it's much simpler to follow the init logic when the 
init components are not hidden deep into the call chain.

-- 
 i.

> ---
>  drivers/platform/x86/mlx-platform.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/platform/x86/mlx-platform.c b/drivers/platform/x86/mlx-platform.c
> index 3d96dbf79a72..64701b63336e 100644
> --- a/drivers/platform/x86/mlx-platform.c
> +++ b/drivers/platform/x86/mlx-platform.c
> @@ -6598,6 +6598,7 @@ static int mlxplat_probe(struct platform_device *pdev)
>  fail_register_reboot_notifier:
>  fail_regcache_sync:
>  	mlxplat_pre_exit(priv);
> +	mlxplat_i2c_main_exit(priv);
>  fail_mlxplat_i2c_main_init:
>  fail_regmap_write:
>  fail_alloc:
>