[PATCH v3] tty: ldisc: fix deadlock between ldisc_sem and rtnl_mutex

[PATCH v3] tty: ldisc: fix deadlock between ldisc_sem and rtnl_mutex

[Yun Zhou]

syzbot reported a circular lock dependency involving tty ldisc_sem and
the networking rtnl_mutex. The full chain is:

  rtnl_mutex --> nft_commit_mutex --> ... --> ep->mtx --> ldisc_sem --> rtnl_mutex

The last edge (ldisc_sem -> rtnl_mutex) is created because tty line
discipline .open() callbacks (slcan, slip) call register_netdev() which
acquires rtnl_mutex, and .open() runs under ldisc_sem write lock in
tty_set_ldisc().

Fix by moving the .open() call outside the ldisc_sem write lock. The
ldisc .open() is initialization of the NEW discipline after the old one
has been closed - there is no need for ldisc_sem protection at this
point since:

 - tty_lock is held throughout, preventing concurrent tty_set_ldisc,
   hangup, or close
 - tty->ldisc is set to NULL during the window. tty_ldisc_ref_wait()
   waits for the transition to complete. tty_ldisc_ref() returns NULL
   which callers already handle.
 - tty buffer data stays queued until the ldisc is installed

The sequence becomes:
  1. Hold ldisc_sem(write): close old ldisc, set tty->ldisc = NULL
  2. Release ldisc_sem(write)
  3. Call new_ldisc->ops->open() without ldisc_sem
  4. Re-acquire ldisc_sem(write): install new ldisc (or restore old)
  5. Release ldisc_sem(write)

Reported-by: syzbot+de610eeef174bd59a8a3@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=de610eeef174bd59a8a3
Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
---
v3:
 - Use a while loop in tty_ldisc_ref_wait() to handle consecutive ldisc
   switches where a reader could see NULL again after being woken up.
v2:
 - Keep user-visible behavior unchanged: tty_ldisc_ref_wait() now waits
   for the ldisc transition to complete instead of returning NULL (which
   would cause spurious EOF/-EIO to concurrent readers).
 - Fix a race between tty_ldisc_ref_wait() and __tty_hangup() where a
   reader could block forever if it observed ldisc==NULL before
   TTY_HUPPED was set. Add wake_up() after set_bit(TTY_HUPPED).

 drivers/tty/tty_io.c    |  7 +++++++
 drivers/tty/tty_ldisc.c | 30 ++++++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 6b283fd03ff8..e2f82e80f397 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -649,6 +649,13 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
 	 */
 	set_bit(TTY_HUPPED, &tty->flags);
 	clear_bit(TTY_HUPPING, &tty->flags);
+
+	/*
+	 * Wake up readers blocked in tty_ldisc_ref_wait() that may have
+	 * seen ldisc == NULL but not yet TTY_HUPPED.
+	 */
+	wake_up(&tty->read_wait);
+
 	tty_unlock(tty);
 
 	if (f)
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index 27fe8236f662..6ec93e6b8498 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -242,6 +242,16 @@ struct tty_ldisc *tty_ldisc_ref_wait(struct tty_struct *tty)
 
 	ldsem_down_read(&tty->ldisc_sem, MAX_SCHEDULE_TIMEOUT);
 	ld = tty->ldisc;
+	while (!ld && !test_bit(TTY_HUPPED, &tty->flags)) {
+		ldsem_up_read(&tty->ldisc_sem);
+
+		/* ldisc may be NULL during a discipline switch; wait and retry */
+		wait_event(tty->read_wait,
+			   READ_ONCE(tty->ldisc) != NULL ||
+			   test_bit(TTY_HUPPED, &tty->flags));
+		ldsem_down_read(&tty->ldisc_sem, MAX_SCHEDULE_TIMEOUT);
+		ld = tty->ldisc;
+	}
 	if (!ld)
 		ldsem_up_read(&tty->ldisc_sem);
 	return ld;
@@ -556,15 +566,28 @@ int tty_set_ldisc(struct tty_struct *tty, int disc)
 	/* Shutdown the old discipline. */
 	tty_ldisc_close(tty, old_ldisc);
 
-	/* Now set up the new line discipline. */
-	tty->ldisc = new_ldisc;
+	/* Clear tty->ldisc so concurrent readers back off during transition */
+	tty->ldisc = NULL;
 	tty_set_termios_ldisc(tty, disc);
+	tty_ldisc_unlock(tty);
 
+	/*
+	 * Open the new discipline outside ldisc_sem. The ldisc .open()
+	 * may acquire locks (e.g., rtnl_mutex) that would create circular
+	 * dependencies if taken under ldisc_sem. tty_lock is still held,
+	 * preventing concurrent ldisc changes and hangup.
+	 */
 	retval = tty_ldisc_open(tty, new_ldisc);
+
+	tty_ldisc_lock(tty, MAX_SCHEDULE_TIMEOUT);
+
 	if (retval < 0) {
 		/* Back to the old one or N_TTY if we can't */
 		tty_ldisc_put(new_ldisc);
 		tty_ldisc_restore(tty, old_ldisc);
+	} else {
+		/* Success - install new ldisc */
+		tty->ldisc = new_ldisc;
 	}
 
 	if (tty->ldisc->ops->num != old_ldisc->ops->num && tty->ops->set_ldisc) {
@@ -584,6 +607,9 @@ int tty_set_ldisc(struct tty_struct *tty, int disc)
 out:
 	tty_ldisc_unlock(tty);
 
+	/* Wake up readers waiting for the ldisc transition to complete */
+	wake_up(&tty->read_wait);
+
 	/*
 	 * Restart the work queue in case no characters kick it off. Safe if
 	 * already running
-- 
2.43.0

Re: [PATCH v3] tty: ldisc: fix deadlock between ldisc_sem and rtnl_mutex

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch replaces a lockdep splat with a silent waitqueue deadlock on the success path.

Pre-existing issues:
- [High] Incomplete fix for circular locking between ldisc_sem and rtnl_mutex, leaving deadlocks on teardown, error, and hangup paths.
--

--- Patch [1]: [PATCH v3] tty: ldisc: fix deadlock between ldisc_sem and rtnl_mutex ---
commit 98cde28b47f83f4a2bf71bb1e35f53d9a7a36294
Author: Yun Zhou <yun.zhou@windriver.com>

tty: ldisc: fix deadlock between ldisc_sem and rtnl_mutex
    
This commit attempts to fix a circular lock dependency between ldisc_sem
and rtnl_mutex reported by syzbot. It moves the tty line discipline .open()
callback outside the ldisc_sem write lock in tty_set_ldisc(). It also adds
logic in tty_ldisc_ref_wait() to wait for the ldisc transition to complete.

Closes: https://syzkaller.appspot.com/bug?extid=de610eeef174bd59a8a3

> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index 27fe8236f6623..6ec93e6b84988 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -242,6 +242,16 @@ struct tty_ldisc *tty_ldisc_ref_wait(struct tty_struct *tty)
>  
>  	ldsem_down_read(&tty->ldisc_sem, MAX_SCHEDULE_TIMEOUT);
>  	ld = tty->ldisc;
> +	while (!ld && !test_bit(TTY_HUPPED, &tty->flags)) {
> +		ldsem_up_read(&tty->ldisc_sem);
> +
> +		/* ldisc may be NULL during a discipline switch; wait and retry */
> +		wait_event(tty->read_wait,
> +			   READ_ONCE(tty->ldisc) != NULL ||
> +			   test_bit(TTY_HUPPED, &tty->flags));

[Severity: High]
Does this wait_event() create a silent waitqueue deadlock that replaces the
lockdep splat?

If thread A switches to a network ldisc like slcan, it will set tty->ldisc
to NULL in tty_set_ldisc() and drop ldisc_sem, but then block waiting for
rtnl_mutex in the .open() callback.

Concurrently, if thread B already holds rtnl_mutex and calls tty_poll(),
it enters tty_ldisc_ref_wait() here. Thread B will observe tty->ldisc as NULL
and block uninterruptibly in this wait_event(), waiting for thread A to
re-assign tty->ldisc.

Since thread A is blocked waiting for thread B to release rtnl_mutex,
will both threads hang forever without a lockdep warning?

> +		ldsem_down_read(&tty->ldisc_sem, MAX_SCHEDULE_TIMEOUT);
> +		ld = tty->ldisc;
> +	}
>  	if (!ld)
>  		ldsem_up_read(&tty->ldisc_sem);

[ ... ]

> @@ -556,15 +566,28 @@ int tty_set_ldisc(struct tty_struct *tty, int disc)
>  	/* Shutdown the old discipline. */
>  	tty_ldisc_close(tty, old_ldisc);

[Severity: High]
This is a pre-existing issue, but does this leave the deadlock intact on
teardown and error paths?

The tty_ldisc_close() function is called here while the ldisc_sem write
lock is held. For network ldiscs like slcan, the ops->close() callback
will acquire rtnl_mutex, which perfectly preserves the original deadlock:

tty_set_ldisc()
  tty_ldisc_lock()       <-- acquires ldisc_sem
  tty_ldisc_close()
    ld->ops->close()     <-- acquires rtnl_mutex

Similarly, on error fallback paths (like tty_ldisc_restore() calling
tty_ldisc_failto()), and during hangup in tty_ldisc_reinit(), both close()
and open() are called while holding the ldisc_sem write lock.

If userspace triggers a fallback to slcan, or a hangup occurs while
slcan is active, won't the deadlock still occur?

> -	/* Now set up the new line discipline. */
> -	tty->ldisc = new_ldisc;
> +	/* Clear tty->ldisc so concurrent readers back off during transition */
> +	tty->ldisc = NULL;
>  	tty_set_termios_ldisc(tty, disc);
> +	tty_ldisc_unlock(tty);
>  
> +	/*
> +	 * Open the new discipline outside ldisc_sem. The ldisc .open()
> +	 * may acquire locks (e.g., rtnl_mutex) that would create circular
> +	 * dependencies if taken under ldisc_sem. tty_lock is still held,
> +	 * preventing concurrent ldisc changes and hangup.
> +	 */
>  	retval = tty_ldisc_open(tty, new_ldisc);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724084648.3879356-1-yun.zhou@windriver.com?part=1