Re: [PATCH v2] 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:
- [Medium] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `tty_ldisc_ref_wait` can cause spurious EOF or -EIO errors for concurrent readers.

Pre-existing issues:
- [High] The circular lock dependency (ldisc_sem -> rtnl_mutex) is not fully resolved in error fallback, hangup, and teardown paths.
--

--- Patch [1]: [PATCH v2] tty: ldisc: fix deadlock between ldisc_sem and rtnl_mutex ---
commit aa2b45f5a2e0cd2e80f19ce89583432e4e6eb9e4
Author: Yun Zhou <[email protected]>

tty: ldisc: fix deadlock between ldisc_sem and rtnl_mutex

syzbot reported a circular lock dependency involving tty ldisc_sem and
the networking rtnl_mutex. Fix by moving the .open() call outside the
ldisc_sem write lock.

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

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

[Severity: Medium]
Could this if statement lead to a Time-of-Check to Time-of-Use (TOCTOU)
race condition returning spurious EOF or -EIO to concurrent readers?

If a reader thread waits here for a discipline switch to finish, the writer
finishes, wakes up the reader, and drops tty_lock. Before the reader can
acquire ldisc_sem(read) again, a third thread could initiate another line
discipline switch, acquiring tty_lock and setting tty->ldisc = NULL.

Since this uses an if statement instead of a while loop, wouldn't the reader
see NULL again and incorrectly return NULL without retrying?

> +			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 +568,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 doesn't the circular lock dependency
still exist in the teardown, hangup, and error fallback paths?

Switching away from a network line discipline (like slcan or slip) calls
.close() here while ldisc_sem is held for write. The .close() callback calls
unregister_netdev(), acquiring rtnl_mutex.

Similarly, if opening a new discipline fails, the fallback tty_ldisc_failto()
calls .open() under ldisc_sem, recreating the exact same deadlock. The same
happens in tty_ldisc_reinit().

> -	/* Now set up the new line discipline. */
> -	tty->ldisc = new_ldisc;
> +	/* Clear tty->ldisc so concurrent readers back off during transition */
> +	tty->ldisc = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1