* [PATCH] platform/x86: intel-vbtn: Protect ACPI notify handler against recursion
@ 2024-07-29 11:00 Hans de Goede
2024-07-29 11:12 ` Ilpo Järvinen
0 siblings, 1 reply; 3+ messages in thread
From: Hans de Goede @ 2024-07-29 11:00 UTC (permalink / raw)
To: Ilpo Järvinen, Andy Shevchenko
Cc: Hans de Goede, platform-driver-x86, En-Wei Wu, Kostadin Stoilov
Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on
all CPUs") ACPI notify handlers like the intel-vbtn notify_handler() may
run on multiple CPU cores racing with themselves.
This race gets hit on Dell Venue 7140 tablets when undocking from
the keyboard, causing the handler to try and register priv->switches_dev
twice, as can be seen from the dev_info() message getting logged twice:
[ 83.861800] intel-vbtn INT33D6:00: Registering Intel Virtual Switches input-dev after receiving a switch event
[ 83.861858] input: Intel Virtual Switches as /devices/pci0000:00/0000:00:1f.0/PNP0C09:00/INT33D6:00/input/input17
[ 83.861865] intel-vbtn INT33D6:00: Registering Intel Virtual Switches input-dev after receiving a switch event
After which things go seriously wrong:
[ 83.861872] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:1f.0/PNP0C09:00/INT33D6:00/input/input17'
...
[ 83.861967] kobject: kobject_add_internal failed for input17 with -EEXIST, don't try to register things with the same name in the same directory.
[ 83.877338] BUG: kernel NULL pointer dereference, address: 0000000000000018
...
Protect intel-vbtn notify_handler() from racing with itself with a mutex
to fix this.
Fixes: e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on all CPUs")
Reported-by: En-Wei Wu <en-wei.wu@canonical.com>
Closes: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2073001
Tested-by: Kostadin Stoilov <kmstoilov@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/platform/x86/intel/vbtn.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
index 9b7ce03ba085..93deda7daac4 100644
--- a/drivers/platform/x86/intel/vbtn.c
+++ b/drivers/platform/x86/intel/vbtn.c
@@ -12,6 +12,7 @@
#include <linux/input/sparse-keymap.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/platform_device.h>
#include <linux/suspend.h>
#include "../dual_accel_detect.h"
@@ -66,6 +67,7 @@ static const struct key_entry intel_vbtn_switchmap[] = {
};
struct intel_vbtn_priv {
+ struct mutex mutex; /* Avoid notify_handler() racing with itself */
struct input_dev *buttons_dev;
struct input_dev *switches_dev;
bool dual_accel;
@@ -155,30 +157,32 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
bool autorelease;
int ret;
+ mutex_lock(&priv->mutex);
+
if ((ke = sparse_keymap_entry_from_scancode(priv->buttons_dev, event))) {
if (!priv->has_buttons) {
dev_warn(&device->dev, "Warning: received 0x%02x button event on a device without buttons, please report this.\n",
event);
- return;
+ goto out_unlock;
}
input_dev = priv->buttons_dev;
} else if ((ke = sparse_keymap_entry_from_scancode(priv->switches_dev, event))) {
if (!priv->has_switches) {
/* See dual_accel_detect.h for more info */
if (priv->dual_accel)
- return;
+ goto out_unlock;
dev_info(&device->dev, "Registering Intel Virtual Switches input-dev after receiving a switch event\n");
ret = input_register_device(priv->switches_dev);
if (ret)
- return;
+ goto out_unlock;
priv->has_switches = true;
}
input_dev = priv->switches_dev;
} else {
dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
- return;
+ goto out_unlock;
}
if (priv->wakeup_mode) {
@@ -189,7 +193,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
* mirroring how the drivers/acpi/button.c code skips this too.
*/
if (ke->type == KE_KEY)
- return;
+ goto out_unlock;
}
/*
@@ -200,6 +204,9 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
autorelease = val && (!ke_rel || ke_rel->type == KE_IGNORE);
sparse_keymap_report_event(input_dev, event, val, autorelease);
+
+out_unlock:
+ mutex_unlock(&priv->mutex);
}
/*
@@ -290,6 +297,10 @@ static int intel_vbtn_probe(struct platform_device *device)
return -ENOMEM;
dev_set_drvdata(&device->dev, priv);
+ err = devm_mutex_init(&device->dev, &priv->mutex);
+ if (err)
+ return err;
+
priv->dual_accel = dual_accel;
priv->has_buttons = has_buttons;
priv->has_switches = has_switches;
--
2.45.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] platform/x86: intel-vbtn: Protect ACPI notify handler against recursion
2024-07-29 11:00 [PATCH] platform/x86: intel-vbtn: Protect ACPI notify handler against recursion Hans de Goede
@ 2024-07-29 11:12 ` Ilpo Järvinen
2024-07-29 12:06 ` Hans de Goede
0 siblings, 1 reply; 3+ messages in thread
From: Ilpo Järvinen @ 2024-07-29 11:12 UTC (permalink / raw)
To: Hans de Goede
Cc: Andy Shevchenko, platform-driver-x86, En-Wei Wu, Kostadin Stoilov
On Mon, 29 Jul 2024, Hans de Goede wrote:
> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on
> all CPUs") ACPI notify handlers like the intel-vbtn notify_handler() may
> run on multiple CPU cores racing with themselves.
>
> This race gets hit on Dell Venue 7140 tablets when undocking from
> the keyboard, causing the handler to try and register priv->switches_dev
> twice, as can be seen from the dev_info() message getting logged twice:
>
> [ 83.861800] intel-vbtn INT33D6:00: Registering Intel Virtual Switches input-dev after receiving a switch event
> [ 83.861858] input: Intel Virtual Switches as /devices/pci0000:00/0000:00:1f.0/PNP0C09:00/INT33D6:00/input/input17
> [ 83.861865] intel-vbtn INT33D6:00: Registering Intel Virtual Switches input-dev after receiving a switch event
>
> After which things go seriously wrong:
> [ 83.861872] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:1f.0/PNP0C09:00/INT33D6:00/input/input17'
> ...
> [ 83.861967] kobject: kobject_add_internal failed for input17 with -EEXIST, don't try to register things with the same name in the same directory.
> [ 83.877338] BUG: kernel NULL pointer dereference, address: 0000000000000018
> ...
>
> Protect intel-vbtn notify_handler() from racing with itself with a mutex
> to fix this.
>
> Fixes: e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on all CPUs")
> Reported-by: En-Wei Wu <en-wei.wu@canonical.com>
> Closes: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2073001
> Tested-by: Kostadin Stoilov <kmstoilov@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/platform/x86/intel/vbtn.c | 21 ++++++++++++++++-----
> 1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
> index 9b7ce03ba085..93deda7daac4 100644
> --- a/drivers/platform/x86/intel/vbtn.c
> +++ b/drivers/platform/x86/intel/vbtn.c
> @@ -12,6 +12,7 @@
> #include <linux/input/sparse-keymap.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/mutex.h>
> #include <linux/platform_device.h>
> #include <linux/suspend.h>
> #include "../dual_accel_detect.h"
> @@ -66,6 +67,7 @@ static const struct key_entry intel_vbtn_switchmap[] = {
> };
>
> struct intel_vbtn_priv {
> + struct mutex mutex; /* Avoid notify_handler() racing with itself */
> struct input_dev *buttons_dev;
> struct input_dev *switches_dev;
> bool dual_accel;
> @@ -155,30 +157,32 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
> bool autorelease;
> int ret;
>
> + mutex_lock(&priv->mutex);
> +
> if ((ke = sparse_keymap_entry_from_scancode(priv->buttons_dev, event))) {
> if (!priv->has_buttons) {
> dev_warn(&device->dev, "Warning: received 0x%02x button event on a device without buttons, please report this.\n",
> event);
> - return;
> + goto out_unlock;
> }
> input_dev = priv->buttons_dev;
> } else if ((ke = sparse_keymap_entry_from_scancode(priv->switches_dev, event))) {
> if (!priv->has_switches) {
> /* See dual_accel_detect.h for more info */
> if (priv->dual_accel)
> - return;
> + goto out_unlock;
>
> dev_info(&device->dev, "Registering Intel Virtual Switches input-dev after receiving a switch event\n");
> ret = input_register_device(priv->switches_dev);
> if (ret)
> - return;
> + goto out_unlock;
>
> priv->has_switches = true;
> }
> input_dev = priv->switches_dev;
> } else {
> dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
> - return;
> + goto out_unlock;
> }
>
> if (priv->wakeup_mode) {
> @@ -189,7 +193,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
> * mirroring how the drivers/acpi/button.c code skips this too.
> */
> if (ke->type == KE_KEY)
> - return;
> + goto out_unlock;
> }
>
> /*
> @@ -200,6 +204,9 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
> autorelease = val && (!ke_rel || ke_rel->type == KE_IGNORE);
>
> sparse_keymap_report_event(input_dev, event, val, autorelease);
> +
> +out_unlock:
> + mutex_unlock(&priv->mutex);
Please use guard() and keep the return statements as is + add cleanup.h
include if not yet there.
--
i.
> }
>
> /*
> @@ -290,6 +297,10 @@ static int intel_vbtn_probe(struct platform_device *device)
> return -ENOMEM;
> dev_set_drvdata(&device->dev, priv);
>
> + err = devm_mutex_init(&device->dev, &priv->mutex);
> + if (err)
> + return err;
> +
> priv->dual_accel = dual_accel;
> priv->has_buttons = has_buttons;
> priv->has_switches = has_switches;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] platform/x86: intel-vbtn: Protect ACPI notify handler against recursion
2024-07-29 11:12 ` Ilpo Järvinen
@ 2024-07-29 12:06 ` Hans de Goede
0 siblings, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2024-07-29 12:06 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Andy Shevchenko, platform-driver-x86, En-Wei Wu, Kostadin Stoilov
Hi Ilpo,
On 7/29/24 1:12 PM, Ilpo Järvinen wrote:
> On Mon, 29 Jul 2024, Hans de Goede wrote:
>
>> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on
>> all CPUs") ACPI notify handlers like the intel-vbtn notify_handler() may
>> run on multiple CPU cores racing with themselves.
>>
>> This race gets hit on Dell Venue 7140 tablets when undocking from
>> the keyboard, causing the handler to try and register priv->switches_dev
>> twice, as can be seen from the dev_info() message getting logged twice:
>>
>> [ 83.861800] intel-vbtn INT33D6:00: Registering Intel Virtual Switches input-dev after receiving a switch event
>> [ 83.861858] input: Intel Virtual Switches as /devices/pci0000:00/0000:00:1f.0/PNP0C09:00/INT33D6:00/input/input17
>> [ 83.861865] intel-vbtn INT33D6:00: Registering Intel Virtual Switches input-dev after receiving a switch event
>>
>> After which things go seriously wrong:
>> [ 83.861872] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:1f.0/PNP0C09:00/INT33D6:00/input/input17'
>> ...
>> [ 83.861967] kobject: kobject_add_internal failed for input17 with -EEXIST, don't try to register things with the same name in the same directory.
>> [ 83.877338] BUG: kernel NULL pointer dereference, address: 0000000000000018
>> ...
>>
>> Protect intel-vbtn notify_handler() from racing with itself with a mutex
>> to fix this.
>>
>> Fixes: e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on all CPUs")
>> Reported-by: En-Wei Wu <en-wei.wu@canonical.com>
>> Closes: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2073001
>> Tested-by: Kostadin Stoilov <kmstoilov@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/platform/x86/intel/vbtn.c | 21 ++++++++++++++++-----
>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
>> index 9b7ce03ba085..93deda7daac4 100644
>> --- a/drivers/platform/x86/intel/vbtn.c
>> +++ b/drivers/platform/x86/intel/vbtn.c
>> @@ -12,6 +12,7 @@
>> #include <linux/input/sparse-keymap.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> +#include <linux/mutex.h>
>> #include <linux/platform_device.h>
>> #include <linux/suspend.h>
>> #include "../dual_accel_detect.h"
>> @@ -66,6 +67,7 @@ static const struct key_entry intel_vbtn_switchmap[] = {
>> };
>>
>> struct intel_vbtn_priv {
>> + struct mutex mutex; /* Avoid notify_handler() racing with itself */
>> struct input_dev *buttons_dev;
>> struct input_dev *switches_dev;
>> bool dual_accel;
>> @@ -155,30 +157,32 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>> bool autorelease;
>> int ret;
>>
>> + mutex_lock(&priv->mutex);
>> +
>> if ((ke = sparse_keymap_entry_from_scancode(priv->buttons_dev, event))) {
>> if (!priv->has_buttons) {
>> dev_warn(&device->dev, "Warning: received 0x%02x button event on a device without buttons, please report this.\n",
>> event);
>> - return;
>> + goto out_unlock;
>> }
>> input_dev = priv->buttons_dev;
>> } else if ((ke = sparse_keymap_entry_from_scancode(priv->switches_dev, event))) {
>> if (!priv->has_switches) {
>> /* See dual_accel_detect.h for more info */
>> if (priv->dual_accel)
>> - return;
>> + goto out_unlock;
>>
>> dev_info(&device->dev, "Registering Intel Virtual Switches input-dev after receiving a switch event\n");
>> ret = input_register_device(priv->switches_dev);
>> if (ret)
>> - return;
>> + goto out_unlock;
>>
>> priv->has_switches = true;
>> }
>> input_dev = priv->switches_dev;
>> } else {
>> dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
>> - return;
>> + goto out_unlock;
>> }
>>
>> if (priv->wakeup_mode) {
>> @@ -189,7 +193,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>> * mirroring how the drivers/acpi/button.c code skips this too.
>> */
>> if (ke->type == KE_KEY)
>> - return;
>> + goto out_unlock;
>> }
>>
>> /*
>> @@ -200,6 +204,9 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>> autorelease = val && (!ke_rel || ke_rel->type == KE_IGNORE);
>>
>> sparse_keymap_report_event(input_dev, event, val, autorelease);
>> +
>> +out_unlock:
>> + mutex_unlock(&priv->mutex);
>
> Please use guard() and keep the return statements as is + add cleanup.h
> include if not yet there.
Good point, I've just send a v2 switching to guard(mutex)().
Regards,
Hans
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-07-29 12:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-29 11:00 [PATCH] platform/x86: intel-vbtn: Protect ACPI notify handler against recursion Hans de Goede
2024-07-29 11:12 ` Ilpo Järvinen
2024-07-29 12:06 ` Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox