HID: asus: Add check for cancelling fn_lock_sync_work

[PATCH] HID: asus: Add check for cancelling fn_lock_sync_work

Posted by Sahil Chandna 1 week ago

syzbot reported a workqueue warning where fn_lock_sync_work is cancelled
during device removal before the work has been initialized. This can
happen when the device is disconnected while initialization is still
in progress.
Fix this by initializing fn_lock_sync_work before marking fn_lock as
enabled, and by using fn_lock as a flag in the remove path. This
ensures cancel_work_sync() is only called for initialized work.

Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
Reported-by: syzbot+13f8286fa2de04a7cd48@syzkaller.appspotmail.com
Signed-off-by: Sahil Chandna <chandna.sahil@gmail.com>
---
 drivers/hid/hid-asus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 1b9793f7c07e..bb85a36de14f 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -960,8 +960,8 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 	}
 
 	if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
-		drvdata->fn_lock = true;
 		INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
+		drvdata->fn_lock = true;
 		asus_kbd_set_fn_lock(hdev, true);
 	}
 
@@ -1343,7 +1343,7 @@ static void asus_remove(struct hid_device *hdev)
 		cancel_work_sync(&drvdata->kbd_backlight->work);
 	}
 
-	if (drvdata->quirks & QUIRK_HID_FN_LOCK)
+	if ((drvdata->quirks & QUIRK_HID_FN_LOCK) && drvdata->fn_lock)
 		cancel_work_sync(&drvdata->fn_lock_sync_work);
 
 	hid_hw_stop(hdev);
-- 
2.50.1

Re: [PATCH] HID: asus: Add check for cancelling fn_lock_sync_work

Posted by Alan Stern 4 days, 9 hours ago

I just noticed this because of a related message in a different thread.

On Fri, Jan 26, 2026 at 09:22:04PM +0530, Sahil Chandna wrote:

> syzbot reported a workqueue warning where fn_lock_sync_work is cancelled
> during device removal before the work has been initialized. This can
> happen when the device is disconnected while initialization is still
> in progress.
> Fix this by initializing fn_lock_sync_work before marking fn_lock as
> enabled, and by using fn_lock as a flag in the remove path. This
> ensures cancel_work_sync() is only called for initialized work.
> 
> Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
> Reported-by: syzbot+13f8286fa2de04a7cd48@syzkaller.appspotmail.com
> Signed-off-by: Sahil Chandna <chandna.sahil@gmail.com>
> ---
>  drivers/hid/hid-asus.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 1b9793f7c07e..bb85a36de14f 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -960,8 +960,8 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  	}
>  
>  	if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
> -		drvdata->fn_lock = true;
>  		INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
> +		drvdata->fn_lock = true;
>  		asus_kbd_set_fn_lock(hdev, true);
>  	}
>  
> @@ -1343,7 +1343,7 @@ static void asus_remove(struct hid_device *hdev)
>  		cancel_work_sync(&drvdata->kbd_backlight->work);
>  	}
>  
> -	if (drvdata->quirks & QUIRK_HID_FN_LOCK)
> +	if ((drvdata->quirks & QUIRK_HID_FN_LOCK) && drvdata->fn_lock)
>  		cancel_work_sync(&drvdata->fn_lock_sync_work);
>  
>  	hid_hw_stop(hdev);

With no synchronization between the two routines, this patch cannot 
possibly be correct.  There's nothing to prevent the CPU running 
asus_input_configured() from executing the assignment to 
drvdata->fn_lock before doing the INIT_WORK() (unless the INIT_WORK() 
call itself contains some synchronization -- but obviously the code 
shouldn't depend on that).

At the very least there should be a pair of memory barriers.  A more 
palatable substitute would be to protect both regions of code with a 
mutex.

Alan Stern

Re: [PATCH] HID: asus: Add check for cancelling fn_lock_sync_work

Posted by Jiri Kosina 3 days, 1 hour ago

On Mon, 2 Feb 2026, Alan Stern wrote:

> With no synchronization between the two routines, this patch cannot 
> possibly be correct.  There's nothing to prevent the CPU running 
> asus_input_configured() from executing the assignment to 
> drvdata->fn_lock before doing the INIT_WORK() (unless the INIT_WORK() 
> call itself contains some synchronization -- but obviously the code 
> shouldn't depend on that).

Ouch, you are of course right, that escaped my attention. Thanks a lot for 
catching this, Alan!

Now dropped from the queue.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: asus: Add check for cancelling fn_lock_sync_work

Posted by Sahil Chandna 4 days ago

On Mon, Feb 02, 2026 at 10:27:33PM -0500, Alan Stern wrote:
>I just noticed this because of a related message in a different thread.
>
>On Fri, Jan 26, 2026 at 09:22:04PM +0530, Sahil Chandna wrote:
>
>> syzbot reported a workqueue warning where fn_lock_sync_work is cancelled
>> during device removal before the work has been initialized. This can
>> happen when the device is disconnected while initialization is still
>> in progress.
>> Fix this by initializing fn_lock_sync_work before marking fn_lock as
>> enabled, and by using fn_lock as a flag in the remove path. This
>> ensures cancel_work_sync() is only called for initialized work.
>>
>> Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
>> Reported-by: syzbot+13f8286fa2de04a7cd48@syzkaller.appspotmail.com
>> Signed-off-by: Sahil Chandna <chandna.sahil@gmail.com>
>> ---
>>  drivers/hid/hid-asus.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index 1b9793f7c07e..bb85a36de14f 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
>> @@ -960,8 +960,8 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>>  	}
>>
>>  	if (drvdata->quirks & QUIRK_HID_FN_LOCK) {
>> -		drvdata->fn_lock = true;
>>  		INIT_WORK(&drvdata->fn_lock_sync_work, asus_sync_fn_lock);
>> +		drvdata->fn_lock = true;
>>  		asus_kbd_set_fn_lock(hdev, true);
>>  	}
>>
>> @@ -1343,7 +1343,7 @@ static void asus_remove(struct hid_device *hdev)
>>  		cancel_work_sync(&drvdata->kbd_backlight->work);
>>  	}
>>
>> -	if (drvdata->quirks & QUIRK_HID_FN_LOCK)
>> +	if ((drvdata->quirks & QUIRK_HID_FN_LOCK) && drvdata->fn_lock)
>>  		cancel_work_sync(&drvdata->fn_lock_sync_work);
>>
>>  	hid_hw_stop(hdev);
>
>With no synchronization between the two routines, this patch cannot
>possibly be correct.  There's nothing to prevent the CPU running
>asus_input_configured() from executing the assignment to
>drvdata->fn_lock before doing the INIT_WORK() (unless the INIT_WORK()
>call itself contains some synchronization -- but obviously the code
>shouldn't depend on that).
>
>At the very least there should be a pair of memory barriers.  A more
>palatable substitute would be to protect both regions of code with a
>mutex.
>
>Alan Stern
Thanks, I will test with mutex between INIT_WORK and cancel_work
and share v2.
Regards,
Sahil

Re: [PATCH] HID: asus: Add check for cancelling fn_lock_sync_work

Posted by Jiri Kosina 4 days, 15 hours ago

On Fri, 30 Jan 2026, Sahil Chandna wrote:

> syzbot reported a workqueue warning where fn_lock_sync_work is cancelled
> during device removal before the work has been initialized. This can
> happen when the device is disconnected while initialization is still
> in progress.
> Fix this by initializing fn_lock_sync_work before marking fn_lock as
> enabled, and by using fn_lock as a flag in the remove path. This
> ensures cancel_work_sync() is only called for initialized work.
> 
> Fixes: f631011e36b8 ("HID: hid-asus: Implement fn lock for Asus ProArt P16")
> Reported-by: syzbot+13f8286fa2de04a7cd48@syzkaller.appspotmail.com
> Signed-off-by: Sahil Chandna <chandna.sahil@gmail.com>

Applied to hid.git#for-6.20/asus, thanks.

-- 
Jiri Kosina
SUSE Labs