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@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.
Good point, I've just send a v2 switching to guard(mutex)().
Regards,
Hans
>
