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

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 <[email protected]>

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/[email protected]?part=1