[RFC PATCH] i2c: acpi: put device when verifying client fails

[RFC PATCH] i2c: acpi: put device when verifying client fails

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

i2c_verify_client() can fail, so we need to put the device when that
happens.

Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

RFC because I don't know if it can be that the returned dev is not an
i2c_client. Yet, since it can happen theoretically, I think we should
have the checks.

 drivers/i2c/i2c-core-acpi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 8f3dbc97a057..8b0ff780919b 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -394,9 +394,17 @@ EXPORT_SYMBOL_GPL(i2c_acpi_find_adapter_by_handle);
 static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
 {
 	struct device *dev;
+	struct i2c_client *client;
 
 	dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
-	return dev ? i2c_verify_client(dev) : NULL;
+	if (!dev)
+		return NULL;
+
+	client = i2c_verify_client(dev);
+	if (!client)
+		put_device(dev);
+
+	return client;
 }
 
 static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
-- 
2.20.1

Re: [RFC PATCH] i2c: acpi: put device when verifying client fails

[Geert Uytterhoeven]

On Thu, Mar 12, 2020 at 2:32 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> i2c_verify_client() can fail, so we need to put the device when that
> happens.
>
> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH] i2c: acpi: put device when verifying client fails

[Andy Shevchenko]

On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> i2c_verify_client() can fail, so we need to put the device when that
> happens.

NAK, this will do double put and messing up with reference counters.
Besides the fact, that device may disappear after looking up which leads us to
even more problems.

See how i2c_acpi_find_client_by_adev() is used in callers.

> 
> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> RFC because I don't know if it can be that the returned dev is not an
> i2c_client. Yet, since it can happen theoretically, I think we should
> have the checks.
> 
>  drivers/i2c/i2c-core-acpi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 8f3dbc97a057..8b0ff780919b 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -394,9 +394,17 @@ EXPORT_SYMBOL_GPL(i2c_acpi_find_adapter_by_handle);
>  static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
>  {
>  	struct device *dev;
> +	struct i2c_client *client;
>  
>  	dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> -	return dev ? i2c_verify_client(dev) : NULL;
> +	if (!dev)
> +		return NULL;
> +
> +	client = i2c_verify_client(dev);
> +	if (!client)
> +		put_device(dev);
> +
> +	return client;
>  }
>  
>  static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH] i2c: acpi: put device when verifying client fails

[Andy Shevchenko]

On Thu, Mar 12, 2020 at 04:47:39PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > 
> > i2c_verify_client() can fail, so we need to put the device when that
> > happens.
> 
> NAK, this will do double put and messing up with reference counters.
> Besides the fact, that device may disappear after looking up which leads us to
> even more problems.
> 
> See how i2c_acpi_find_client_by_adev() is used in callers.

Perhaps proper "fix" is to add the explanation to a comment in the code to
prevent false positive reports in the future?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH] i2c: acpi: put device when verifying client fails

[Andy Shevchenko]

On Thu, Mar 12, 2020 at 04:49:08PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 12, 2020 at 04:47:39PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> > > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > 
> > > i2c_verify_client() can fail, so we need to put the device when that
> > > happens.
> > 
> > NAK, this will do double put and messing up with reference counters.
> > Besides the fact, that device may disappear after looking up which leads us to
> > even more problems.
> > 
> > See how i2c_acpi_find_client_by_adev() is used in callers.
> 
> Perhaps proper "fix" is to add the explanation to a comment in the code to
> prevent false positive reports in the future?

Ah, sorry, I need to read it more carefully...


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH] i2c: acpi: put device when verifying client fails

[Andy Shevchenko]

On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> i2c_verify_client() can fail, so we need to put the device when that
> happens.

I hope it's not a CoVID-19 makes me mistakenly commented in the first place. :-)

So, theoretically below is possible, but practically it's doubtful.

The I2CSerialBusV2() ACPI resource can be present solely in I²C slave device
nodes according to the specification. However, we might have two possible cases
a) screwed up ACPI table;
b) I²C master which in turn is I²C slave.

While a) has been so far unseen, b) case sounds like plasible for I²C muxes IIUC.

So, I agree with the patch, and sorry for the first reaction.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> RFC because I don't know if it can be that the returned dev is not an
> i2c_client. Yet, since it can happen theoretically, I think we should
> have the checks.
> 
>  drivers/i2c/i2c-core-acpi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 8f3dbc97a057..8b0ff780919b 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -394,9 +394,17 @@ EXPORT_SYMBOL_GPL(i2c_acpi_find_adapter_by_handle);
>  static struct i2c_client *i2c_acpi_find_client_by_adev(struct acpi_device *adev)
>  {
>  	struct device *dev;
> +	struct i2c_client *client;
>  
>  	dev = bus_find_device_by_acpi_dev(&i2c_bus_type, adev);
> -	return dev ? i2c_verify_client(dev) : NULL;
> +	if (!dev)
> +		return NULL;
> +
> +	client = i2c_verify_client(dev);
> +	if (!client)
> +		put_device(dev);
> +
> +	return client;
>  }
>  
>  static int i2c_acpi_notify(struct notifier_block *nb, unsigned long value,
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH] i2c: acpi: put device when verifying client fails

[Mika Westerberg]

On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> i2c_verify_client() can fail, so we need to put the device when that
> happens.
> 
> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> RFC because I don't know if it can be that the returned dev is not an
> i2c_client. Yet, since it can happen theoretically, I think we should
> have the checks.

I agree,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [RFC PATCH] i2c: acpi: put device when verifying client fails

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 462 bytes --]

On Thu, Mar 12, 2020 at 02:32:44PM +0100, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> i2c_verify_client() can fail, so we need to put the device when that
> happens.
> 
> Fixes: 525e6fabeae2 ("i2c / ACPI: add support for ACPI reconfigure notifications")
> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]