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@xxxxxxxxxxxxx>
> Closes: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2073001
> Tested-by: Kostadin Stoilov <kmstoilov@xxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> 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;
>
