[PATCH v2] Input: serio - don't sleep on serio_mutex from drvctl_store()

[PATCH v2] Input: serio - don't sleep on serio_mutex from drvctl_store()

[Nguyen Ngoc Thang]

Writing to the drvctl attribute takes serio_mutex while holding the
attribute's kernfs active reference. serio_unregister_port() does the
opposite: it holds serio_mutex and device_del() then waits for active
references to drain in kernfs_drain(). If a drvctl write is in flight
when the port is unregistered, the writer waits for serio_mutex and the
unregistering task waits for the writer, and neither makes progress.
Interruptibility of the lock does not help, as nothing signals the writer.

lockdep reports it as:

  WARNING: possible circular locking dependency detected
  repro/4908 is trying to acquire lock:
   (kn->active){++++}-{0:0}, at: __kernfs_remove+0x34c/0xb90
  but task is already holding lock:
   (serio_mutex){+.+.}-{4:4}, at: serio_unregister_port+0x1b/0x40
   drvctl_store
   sysfs_kf_write
   ...
   kernfs_drain
   device_del
   serio_destroy_port
   serio_unregister_port
   userio_char_release

Use mutex_trylock() and restart the syscall when the mutex is busy, so
the active reference is dropped before waiting. Once the port is being
removed the retried write fails with -ENODEV.

Reported-by: syzbot+1075f6dc93f398c1857e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1075f6dc93f398c1857e
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
 drivers/input/serio/serio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index 54dd26249b02..5b650421e066 100644
--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -357,7 +357,8 @@ static ssize_t drvctl_store(struct device *dev, struct device_attribute *attr, c
 	struct device_driver *drv;
 	int error;
 
-	scoped_cond_guard(mutex_intr, return -EINTR, &serio_mutex) {
+	/* Port removal holds serio_mutex and drains us: don't wait for it. */
+	scoped_cond_guard(mutex_try, return -EAGAIN, &serio_mutex) {
 		if (!strncmp(buf, "none", count)) {
 			serio_disconnect_port(serio);
 		} else if (!strncmp(buf, "reconnect", count)) {

base-commit: daae2ab46e0cb612f50ca1d86cf50e5962461ae5
-- 
2.43.0

Re: [PATCH v2] Input: serio - don't sleep on serio_mutex from drvctl_store()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sysfs: write returning -EAGAIN breaks userspace ABI
--

commit 587df4e4a0dc7a2a601093d20c02612cf36f0cbd
Author: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>

Input: serio - don't sleep on serio_mutex from drvctl_store()

Writing to the drvctl attribute takes serio_mutex while holding the
attribute's kernfs active reference. serio_unregister_port() does the
opposite: it holds serio_mutex and device_del() then waits for active
references to drain in kernfs_drain(). If a drvctl write is in flight
when the port is unregistered, the writer waits for serio_mutex and the
unregistering task waits for the writer, and neither makes progress.

> diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
> index 54dd26249b027..5b650421e0663 100644
> --- a/drivers/input/serio/serio.c
> +++ b/drivers/input/serio/serio.c
> @@ -357,7 +357,8 @@ static ssize_t drvctl_store(struct device *dev, struct device_attribute *attr, c
>  	struct device_driver *drv;
>  	int error;
>  
> -	scoped_cond_guard(mutex_intr, return -EINTR, &serio_mutex) {
> +	/* Port removal holds serio_mutex and drains us: don't wait for it. */
> +	scoped_cond_guard(mutex_try, return -EAGAIN, &serio_mutex) {

[Severity: High]
Does returning -EAGAIN directly to the VFS here break the userspace ABI?

Standard userspace tools (like echo) performing blocking writes do not
automatically retry on -EAGAIN. This causes them to fail outright with
'Resource temporarily unavailable' when the trylock fails in drvctl_store()
during concurrent port removal.

Could this be resolved by using sysfs_break_active_protection() or returning
restart_syscall() (which sets TIF_SIGPENDING and returns -ERESTARTNOINTR)
instead, to allow transparent restarting?

>  		if (!strncmp(buf, "none", count)) {
>  			serio_disconnect_port(serio);
>  		} else if (!strncmp(buf, "reconnect", count)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924040430.711439-1-ngocthang2710.1999@gmail.com?part=1