* Re: [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on error path
2023-06-12 9:02 [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on error path Michal Wilczynski
@ 2023-06-12 11:21 ` Rafael J. Wysocki
2023-06-12 17:52 ` Pali Rohár
2023-06-13 9:28 ` Ilpo Järvinen
2 siblings, 0 replies; 8+ messages in thread
From: Rafael J. Wysocki @ 2023-06-12 11:21 UTC (permalink / raw)
To: Michal Wilczynski
Cc: linux-acpi, rafael, andriy.shevchenko, ilpo.jarvinen, pali,
hdegoede, markgross, fengguang.wu, dvhart, platform-driver-x86,
linux-kernel
On Mon, Jun 12, 2023 at 11:03 AM Michal Wilczynski
<michal.wilczynski@intel.com> wrote:
>
> Currently rbtn_add() in case of failure is leaking resources. Fix this
> by adding a proper rollback. While at it, remove unnecessary assignment
> of NULL to device->driver_data and unnecessary whitespace, plus add a
> break for the default case in a switch.
>
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Fixes: 817a5cdb40c8 ("dell-rbtn: Dell Airplane Mode Switch driver")
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Rafael J. Wysocki <rafael@kernel.org>
> ---
> drivers/platform/x86/dell/dell-rbtn.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/dell-rbtn.c b/drivers/platform/x86/dell/dell-rbtn.c
> index aa0e6c907494..e9b3f9c3ab7d 100644
> --- a/drivers/platform/x86/dell/dell-rbtn.c
> +++ b/drivers/platform/x86/dell/dell-rbtn.c
> @@ -420,10 +420,12 @@ static int rbtn_add(struct acpi_device *device)
> break;
> default:
> ret = -EINVAL;
> + break;
> }
> + if (ret)
> + rbtn_acquire(device, false);
>
> return ret;
> -
> }
>
> static void rbtn_remove(struct acpi_device *device)
> @@ -442,7 +444,6 @@ static void rbtn_remove(struct acpi_device *device)
> }
>
> rbtn_acquire(device, false);
> - device->driver_data = NULL;
> }
>
> static void rbtn_notify(struct acpi_device *device, u32 event)
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on error path
2023-06-12 9:02 [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on error path Michal Wilczynski
2023-06-12 11:21 ` Rafael J. Wysocki
@ 2023-06-12 17:52 ` Pali Rohár
2023-06-12 20:52 ` Andy Shevchenko
2023-06-13 9:28 ` Ilpo Järvinen
2 siblings, 1 reply; 8+ messages in thread
From: Pali Rohár @ 2023-06-12 17:52 UTC (permalink / raw)
To: Michal Wilczynski
Cc: linux-acpi, rafael, andriy.shevchenko, ilpo.jarvinen, hdegoede,
markgross, fengguang.wu, dvhart, platform-driver-x86,
linux-kernel
On Monday 12 June 2023 12:02:50 Michal Wilczynski wrote:
> Currently rbtn_add() in case of failure is leaking resources. Fix this
> by adding a proper rollback. While at it, remove unnecessary assignment
> of NULL to device->driver_data and unnecessary whitespace, plus add a
> break for the default case in a switch.
>
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Fixes: 817a5cdb40c8 ("dell-rbtn: Dell Airplane Mode Switch driver")
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/platform/x86/dell/dell-rbtn.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/dell-rbtn.c b/drivers/platform/x86/dell/dell-rbtn.c
> index aa0e6c907494..e9b3f9c3ab7d 100644
> --- a/drivers/platform/x86/dell/dell-rbtn.c
> +++ b/drivers/platform/x86/dell/dell-rbtn.c
> @@ -420,10 +420,12 @@ static int rbtn_add(struct acpi_device *device)
> break;
> default:
> ret = -EINVAL;
> + break;
> }
> + if (ret)
> + rbtn_acquire(device, false);
>
> return ret;
> -
> }
Hello! I'm looking at rbtn_add() function and there is also code:
rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL);
if (!rbtn_data)
return -ENOMEM;
which is called after rbtn_acquire(). So it looks like when kzalloc
fails then there is another leak...
>
> static void rbtn_remove(struct acpi_device *device)
> @@ -442,7 +444,6 @@ static void rbtn_remove(struct acpi_device *device)
> }
>
> rbtn_acquire(device, false);
> - device->driver_data = NULL;
> }
>
> static void rbtn_notify(struct acpi_device *device, u32 event)
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on error path
2023-06-12 17:52 ` Pali Rohár
@ 2023-06-12 20:52 ` Andy Shevchenko
2023-06-12 20:58 ` Pali Rohár
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2023-06-12 20:52 UTC (permalink / raw)
To: Pali Rohár
Cc: Michal Wilczynski, linux-acpi, rafael, ilpo.jarvinen, hdegoede,
markgross, fengguang.wu, dvhart, platform-driver-x86,
linux-kernel
On Mon, Jun 12, 2023 at 07:52:05PM +0200, Pali Rohár wrote:
> On Monday 12 June 2023 12:02:50 Michal Wilczynski wrote:
> > Currently rbtn_add() in case of failure is leaking resources. Fix this
> > by adding a proper rollback. While at it, remove unnecessary assignment
> > of NULL to device->driver_data and unnecessary whitespace, plus add a
> > break for the default case in a switch.
...
> Hello! I'm looking at rbtn_add() function and there is also code:
>
> rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL);
> if (!rbtn_data)
> return -ENOMEM;
>
> which is called after rbtn_acquire(). So it looks like when kzalloc
> fails then there is another leak...
Side note: In that case we would need a devm wrapper on acquire call.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on error path
2023-06-12 20:52 ` Andy Shevchenko
@ 2023-06-12 20:58 ` Pali Rohár
2023-06-12 21:02 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: Pali Rohár @ 2023-06-12 20:58 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Michal Wilczynski, linux-acpi, rafael, ilpo.jarvinen, hdegoede,
markgross, fengguang.wu, dvhart, platform-driver-x86,
linux-kernel
On Monday 12 June 2023 23:52:30 Andy Shevchenko wrote:
> On Mon, Jun 12, 2023 at 07:52:05PM +0200, Pali Rohár wrote:
> > On Monday 12 June 2023 12:02:50 Michal Wilczynski wrote:
> > > Currently rbtn_add() in case of failure is leaking resources. Fix this
> > > by adding a proper rollback. While at it, remove unnecessary assignment
> > > of NULL to device->driver_data and unnecessary whitespace, plus add a
> > > break for the default case in a switch.
>
> ...
>
> > Hello! I'm looking at rbtn_add() function and there is also code:
> >
> > rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL);
> > if (!rbtn_data)
> > return -ENOMEM;
> >
> > which is called after rbtn_acquire(). So it looks like when kzalloc
> > fails then there is another leak...
>
> Side note: In that case we would need a devm wrapper on acquire call.
Does it makes sense to invest time and more resources for these fixes?
Driver is not used on new Dell machines, so I would not expect new
users (instead less users, if they start upgrading HW to new Dell
machines).
Simple fix for this issue: Just move devm_kzalloc() call before
rbtn_acquire(true). And call cleanup rbtn_acquire(false) exactly like
Michal did in this patch.
I think that this should be enough, should cover all failure paths and
does not require to introduce new code or new design, which should be
properly tested for no regression.
What do you think?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on error path
2023-06-12 20:58 ` Pali Rohár
@ 2023-06-12 21:02 ` Andy Shevchenko
0 siblings, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2023-06-12 21:02 UTC (permalink / raw)
To: Pali Rohár
Cc: Michal Wilczynski, linux-acpi, rafael, ilpo.jarvinen, hdegoede,
markgross, fengguang.wu, dvhart, platform-driver-x86,
linux-kernel
On Mon, Jun 12, 2023 at 10:58:39PM +0200, Pali Rohár wrote:
> On Monday 12 June 2023 23:52:30 Andy Shevchenko wrote:
> > On Mon, Jun 12, 2023 at 07:52:05PM +0200, Pali Rohár wrote:
> > > On Monday 12 June 2023 12:02:50 Michal Wilczynski wrote:
...
> > > Hello! I'm looking at rbtn_add() function and there is also code:
> > >
> > > rbtn_data = devm_kzalloc(&device->dev, sizeof(*rbtn_data), GFP_KERNEL);
> > > if (!rbtn_data)
> > > return -ENOMEM;
> > >
> > > which is called after rbtn_acquire(). So it looks like when kzalloc
> > > fails then there is another leak...
> >
> > Side note: In that case we would need a devm wrapper on acquire call.
>
> Does it makes sense to invest time and more resources for these fixes?
> Driver is not used on new Dell machines, so I would not expect new
> users (instead less users, if they start upgrading HW to new Dell
> machines).
>
> Simple fix for this issue: Just move devm_kzalloc() call before
> rbtn_acquire(true). And call cleanup rbtn_acquire(false) exactly like
> Michal did in this patch.
>
> I think that this should be enough, should cover all failure paths and
> does not require to introduce new code or new design, which should be
> properly tested for no regression.
>
> What do you think?
Sounds like a good alternative! Thank you for the review.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on error path
2023-06-12 9:02 [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on error path Michal Wilczynski
2023-06-12 11:21 ` Rafael J. Wysocki
2023-06-12 17:52 ` Pali Rohár
@ 2023-06-13 9:28 ` Ilpo Järvinen
2023-06-13 9:57 ` Wilczynski, Michal
2 siblings, 1 reply; 8+ messages in thread
From: Ilpo Järvinen @ 2023-06-13 9:28 UTC (permalink / raw)
To: Michal Wilczynski
Cc: linux-acpi, rafael, andriy.shevchenko, pali, hdegoede, markgross,
fengguang.wu, dvhart, platform-driver-x86, LKML
[-- Attachment #1: Type: text/plain, Size: 1603 bytes --]
On Mon, 12 Jun 2023, Michal Wilczynski wrote:
> Currently rbtn_add() in case of failure is leaking resources. Fix this
> by adding a proper rollback. While at it, remove unnecessary assignment
> of NULL to device->driver_data and unnecessary whitespace, plus add a
> break for the default case in a switch.
>
> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Fixes: 817a5cdb40c8 ("dell-rbtn: Dell Airplane Mode Switch driver")
> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/platform/x86/dell/dell-rbtn.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/dell-rbtn.c b/drivers/platform/x86/dell/dell-rbtn.c
> index aa0e6c907494..e9b3f9c3ab7d 100644
> --- a/drivers/platform/x86/dell/dell-rbtn.c
> +++ b/drivers/platform/x86/dell/dell-rbtn.c
> @@ -420,10 +420,12 @@ static int rbtn_add(struct acpi_device *device)
> break;
> default:
> ret = -EINVAL;
> + break;
> }
> + if (ret)
> + rbtn_acquire(device, false);
>
> return ret;
> -
> }
>
> static void rbtn_remove(struct acpi_device *device)
> @@ -442,7 +444,6 @@ static void rbtn_remove(struct acpi_device *device)
> }
>
> rbtn_acquire(device, false);
> - device->driver_data = NULL;
> }
>
> static void rbtn_notify(struct acpi_device *device, u32 event)
I'm a bit worried the stable people might not like "these while at it
parts". Those changes too are all good but unrelated to the actual fix so
they should appear in their own patches.
--
i.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v1] platform/x86/dell/dell-rbtn: Fix resources leaking on error path
2023-06-13 9:28 ` Ilpo Järvinen
@ 2023-06-13 9:57 ` Wilczynski, Michal
0 siblings, 0 replies; 8+ messages in thread
From: Wilczynski, Michal @ 2023-06-13 9:57 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: linux-acpi, rafael, andriy.shevchenko, pali, hdegoede, markgross,
fengguang.wu, dvhart, platform-driver-x86, LKML
On 6/13/2023 11:28 AM, Ilpo Järvinen wrote:
> On Mon, 12 Jun 2023, Michal Wilczynski wrote:
>
>> Currently rbtn_add() in case of failure is leaking resources. Fix this
>> by adding a proper rollback. While at it, remove unnecessary assignment
>> of NULL to device->driver_data and unnecessary whitespace, plus add a
>> break for the default case in a switch.
>>
>> Suggested-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> Fixes: 817a5cdb40c8 ("dell-rbtn: Dell Airplane Mode Switch driver")
>> Signed-off-by: Michal Wilczynski <michal.wilczynski@intel.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>> drivers/platform/x86/dell/dell-rbtn.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-rbtn.c b/drivers/platform/x86/dell/dell-rbtn.c
>> index aa0e6c907494..e9b3f9c3ab7d 100644
>> --- a/drivers/platform/x86/dell/dell-rbtn.c
>> +++ b/drivers/platform/x86/dell/dell-rbtn.c
>> @@ -420,10 +420,12 @@ static int rbtn_add(struct acpi_device *device)
>> break;
>> default:
>> ret = -EINVAL;
>> + break;
>> }
>> + if (ret)
>> + rbtn_acquire(device, false);
>>
>> return ret;
>> -
>> }
>>
>> static void rbtn_remove(struct acpi_device *device)
>> @@ -442,7 +444,6 @@ static void rbtn_remove(struct acpi_device *device)
>> }
>>
>> rbtn_acquire(device, false);
>> - device->driver_data = NULL;
>> }
>>
>> static void rbtn_notify(struct acpi_device *device, u32 event)
> I'm a bit worried the stable people might not like "these while at it
> parts". Those changes too are all good but unrelated to the actual fix so
> they should appear in their own patches.
Hi, thanks for you input,
Changes seemed really minor and inconsequential, so it seemed to me like it would be an
overkill to create a separate patch for a cosmetic change.
>
^ permalink raw reply [flat|nested] 8+ messages in thread