Re: [PATCH] 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] Concurrent read/write/poll system calls spuriously return EOF or EIO during line discipline transitions, breaking UAPI.

Pre-existing issues:
- [High] The circular lock dependency between ldisc_sem and rtnl_mutex is not fully resolved, remaining on the error handling and hangup paths.
--

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

tty: ldisc: fix deadlock between ldisc_sem and rtnl_mutex

This commit aims to fix a circular lock dependency between the tty ldisc_sem
and the networking rtnl_mutex. It does this by modifying tty_set_ldisc() to
call the line discipline open() callback outside of 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..248a6995cc53a 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -556,15 +556,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);

[Severity: High]
Does unlocking the semaphore while the ldisc pointer is NULL introduce a UAPI
break for concurrent operations?

If a concurrent process calls read(), write(), or poll() during this unlocked
window, it can enter tty_ldisc_ref_wait() in drivers/tty/tty_io.c. Because the
semaphore was unlocked here, tty_ldisc_ref_wait() will successfully acquire
the read lock but observe tty->ldisc as NULL.

This causes the reader to immediately return EOF or -EIO, potentially
aborting userspace applications unexpectedly during a line discipline
transition.

> +
> +	/*
> +	 * 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);

[Severity: High]
This is a pre-existing issue, but does this fully resolve the circular lock
dependency?

If tty_ldisc_open() fails above, the error path calls tty_ldisc_restore(),
which eventually calls tty_ldisc_failto(). Inside tty_ldisc_failto(),
tty_ldisc_open() is called while the ldisc_sem write lock is held.

Similarly, during hangup processing, tty_ldisc_reinit() calls tty_ldisc_open()
while holding the ldisc_sem write lock.

If the restored or reinitialized discipline acquires rtnl_mutex on these paths,
it appears the lock inversion and potential deadlock would still occur.

> +	} else {
> +		/* Success - install new ldisc */
> +		tty->ldisc = new_ldisc;
>  	}

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