linux-kernel - Re: [PATCH v3] iio: light: cm32181: Unregister second I2C client if present

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] [day] [month] [year] [list]

Message-ID: <1761ee67-5912-56d1-5e27-28135b3c0620@redhat.com>
Date:   Wed, 22 Feb 2023 17:29:44 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Kai-Heng Feng <kai.heng.feng@...onical.com>, jic23@...nel.org,
        lars@...afoo.de
Cc:     Kevin Tsai <ktsai@...ellamicro.com>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] iio: light: cm32181: Unregister second I2C client if
 present

Hi,

On 2/22/23 17:24, Kai-Heng Feng wrote:
> If a second dummy client that talks to the actual I2C address was
> created in probe(), there should be a proper cleanup on driver and
> device removal to avoid leakage.
> 
> So unregister the dummy client via another callback.
> 
> Suggested-by: Hans de Goede <hdegoede@...hat.com>
> Fixes: c1e62062ff54 ("iio: light: cm32181: Handle CM3218 ACPI devices with 2 I2C resources")
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2152281
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
> ---
> v3:
>  - Use devm_add_action_or_reset() in a correct place.
>  - Wording.
> 
> v2:
>  - Use devm_add_action_or_reset() instead of remove() callback to avoid
>    race.
> 
>  drivers/iio/light/cm32181.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index b1674a5bfa368..b3da7a517aaea 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -429,6 +429,14 @@ static const struct iio_info cm32181_info = {
>  	.attrs			= &cm32181_attribute_group,
>  };
>  
> +static void cm32181_unregister_dummy_client(void *data)
> +{
> +	struct i2c_client *client = data;
> +
> +	/* Unregister the dummy client */
> +	i2c_unregister_device(client);
> +}
> +
>  static int cm32181_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -460,6 +468,12 @@ static int cm32181_probe(struct i2c_client *client)
>  		client = i2c_acpi_new_device(dev, 1, &board_info);
>  		if (IS_ERR(client))
>  			return PTR_ERR(client);
> +
> +		ret = devm_add_action_or_reset(dev, cm32181_unregister_dummy_client, client);
> +		if (ret) {
> +			dev_err(dev, "%s: add devres action failed\n", __func__);
> +			return ret;
> +		}

Sorry to be a bit pedantic here, but the only way devm_add_action_or_reset() can fail is
if kmalloc fails and we generally do not log errors for kmalloc failures (because the MM
layer will already complain loudly).

So this can be simplified to just:

		ret = devm_add_action_or_reset(dev, cm32181_unregister_dummy_client, client);
		if (ret)
			return ret;

Can you do a v4 with this changed please ?  With this fixed feel free to add my:

Reviewed-by: Hans de Goede <hdegoede@...hat.com>

to v4.

Regards,

Hans