Received-SPF: pass (google.com: domain of linux-kernel+bounces-134959-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <0a05cadf-562d-4516-86bb-51afacb7e167@redhat.com>
Date: Mon, 8 Apr 2024 09:49:46 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] platform/x86: quickstart: Fix race condition when
 reporting input event
To: Armin Wolf <W_Armin@gmx.de>, ilpo.jarvinen@linux.intel.com
Cc: rafael@kernel.org, linux-acpi@vger.kernel.org,
 platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240327214524.123935-1-W_Armin@gmx.de>
 <f662fab0-0f47-4b71-ab71-2b9492253483@gmx.de>
 <b5d808fd-f9c4-48d5-b888-c4ded4b0cad6@redhat.com>
 <2cbad5c6-8c70-45dd-8281-3bbffb4e8daf@gmx.de>
Content-Language: en-US, nl
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <2cbad5c6-8c70-45dd-8281-3bbffb4e8daf@gmx.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi,

On 4/8/24 8:01 AM, Armin Wolf wrote:
> Am 07.04.24 um 17:32 schrieb Hans de Goede:
> 
>> Hi,
>>
>> On 4/6/24 8:57 PM, Armin Wolf wrote:
>>> Am 27.03.24 um 22:45 schrieb Armin Wolf:
>>>
>>>> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run
>>>> on all CPUs"), the ACPI core allows multiple notify calls to be active
>>>> at the same time. This means that two instances of quickstart_notify()
>>>> running at the same time can mess which each others input sequences.
>>>>
>>>> Fix this by protecting the input sequence with a mutex.
>>>>
>>>> Compile-tested only.
>>> Any thoughts on this?
>> I wonder if we need this at all ?
>>
>> The input_event() / input_report_key() / input_sync() functions
>> which underpin sparse_keymap_report_event() all are safe to be called
>> from multiple threads at the same time AFAIK.
>>
>> The only thing which can then still go "wrong" if we have
>> 2 sparse_keymap_report_event() functions racing for the same
>> quickstart button and thus for the same keycode is that we may
>> end up with:
>>
>> input_report_key(dev, keycode, 1);
>> input_report_key(dev, keycode, 1); /* This is a no-op */
>> input_sync(); /* + another input_sync() somewhere which is a no-op */
>> input_report_key(dev, keycode, 0);
>> input_report_key(dev, keycode, 0); /* This is a no-op */
>> input_sync(); /* + another input_sync() somewhere which is a no-op */
>>
>> IOW if 2 racing notifiers hit the perfect race conditions then
>> only 1 key press is reported, instead of 2 which seems like
>> it is not a problem since arguably if the same event gets
>> reported twice at the exact same time it probably really
>> is only a single button press.
>>
>> Also I think it is highly unlikely we will actually see
>> 2 notifiers for this racing in practice.
>>
>> So I don't think we need this at all. But if others feel strongly
>> about adding this I can still merge it... ?
>>
>> Regards,
>>
>> Hans
> 
> Hi,
> 
> the locking issue was originally brought up by Ilpo Jarvinen during the review of the lenovo-wmi-camera driver.
> Also the race condition can cause an invalid input sequence to be emitted:
> 
> input_report_key(dev, keycode, 1);
> input_sync();
> input_report_key(dev, keycode, 0);    // Possible invalid sequence?
> input_report_key(dev, keycode, 1);
> input_sync();
> input_sync();
> input_report_key(dev, keycode, 0);
> input_sync();
> 
> 
> I think this input sequence would be invalid, so we need the locking.

The:

input_report_key(dev, keycode, 0);    // Possible invalid sequence?
input_report_key(dev, keycode, 1);
input_sync();

Part is equivalent of:

input_report_key(dev, keycode, 1);
input_sync();

Since there is no sync() after the release event it will
never reach userspace.

With that said, I'm still happy to merge this if you
prefer to have the locking in place.

Either way please let me know how you want to proceed.

Regards,

Hans


> 
> Thanks,
> Armin Wolf
> 
>>>> Fixes: afd66f2a739e ("platform/x86: Add ACPI quickstart button (PNP0C32) driver")
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>> ---
>>>> This applies on the branch "review-hans". Maybe we could somehow
>>>> document the concurrency rules for ACPI notify handlers?
>>>> ---
>>>>    drivers/platform/x86/quickstart.c | 17 +++++++++++++++++
>>>>    1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c
>>>> index ba3a7a25dda7..e40f852d42c1 100644
>>>> --- a/drivers/platform/x86/quickstart.c
>>>> +++ b/drivers/platform/x86/quickstart.c
>>>> @@ -18,6 +18,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/sysfs.h>
>>>>    #include <linux/types.h>
>>>> @@ -35,6 +36,7 @@
>>>>
>>>>    struct quickstart_data {
>>>>        struct device *dev;
>>>> +    struct mutex input_lock;    /* Protects input sequence during notify */
>>>>        struct input_dev *input_device;
>>>>        char input_name[32];
>>>>        char phys[32];
>>>> @@ -73,7 +75,10 @@ static void quickstart_notify(acpi_handle handle, u32 event, void *context)
>>>>
>>>>        switch (event) {
>>>>        case QUICKSTART_EVENT_RUNTIME:
>>>> +        mutex_lock(&data->input_lock);
>>>>            sparse_keymap_report_event(data->input_device, 0x1, 1, true);
>>>> +        mutex_unlock(&data->input_lock);
>>>> +
>>>>            acpi_bus_generate_netlink_event(DRIVER_NAME, dev_name(data->dev), event, 0);
>>>>            break;
>>>>        default:
>>>> @@ -147,6 +152,13 @@ static void quickstart_notify_remove(void *context)
>>>>        acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, quickstart_notify);
>>>>    }
>>>>
>>>> +static void quickstart_mutex_destroy(void *data)
>>>> +{
>>>> +    struct mutex *lock = data;
>>>> +
>>>> +    mutex_destroy(lock);
>>>> +}
>>>> +
>>>>    static int quickstart_probe(struct platform_device *pdev)
>>>>    {
>>>>        struct quickstart_data *data;
>>>> @@ -165,6 +177,11 @@ static int quickstart_probe(struct platform_device *pdev)
>>>>        data->dev = &pdev->dev;
>>>>        dev_set_drvdata(&pdev->dev, data);
>>>>
>>>> +    mutex_init(&data->input_lock);
>>>> +    ret = devm_add_action_or_reset(&pdev->dev, quickstart_mutex_destroy, &data->input_lock);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>>        /* We have to initialize the device wakeup before evaluating GHID because
>>>>         * doing so will notify the device if the button was used to wake the machine
>>>>         * from S5.
>>>> -- 
>>>> 2.39.2
>>>>
>>>>
>>
>