09.05.2024 09:41, Jiri Slaby wrote:
On 08. 05. 24, 11:30, kovalev@altlinux.org wrote:
From: Vasiliy Kovalev kovalev@altlinux.org
A possible scenario in which a deadlock may occur is as follows:
flush_to_ldisc() {
mutex_lock(&buf->lock);
tty_port_default_receive_buf() { tty_ldisc_receive_buf() { n_tty_receive_buf2() { n_tty_receive_buf_common() { n_tty_receive_char_special() { isig() { tty_driver_flush_buffer() { pty_flush_buffer() { tty_buffer_flush() {
mutex_lock(&buf->lock); (DEADLOCK)
flush_to_ldisc() and tty_buffer_flush() functions they use the same mutex (&buf->lock), but not necessarily the same struct tty_bufhead object.
"not necessarily" -- so does it mean that it actually can happen (and we should fix it) or not at all (and we should annotate the mutex)?
During debugging, when running the reproducer multiple times, I failed to catch a situation where these mutexes have the same address in memory in the above call scenario, so I'm not sure that such a situation is possible. But earlier, a thread is triggered that accesses the same structure (and mutex), so LOCKDEP tools throw a warning:
thread 0: flush_to_ldisc() {
mutex_lock(&buf->lock) // Address mutex == 0xA
n_tty_receive_buf_common();
mutex_unlock(&buf->lock) // Address mutex == 0xA }
thread 1: flush_to_ldisc() {
mutex_lock(&buf->lock) // Address mutex == 0xB
n_tty_receive_buf_common() { isig() { tty_driver_flush_buffer() { pty_flush_buffer() { tty_buffer_flush() {
mutex_lock(&buf->lock) // Address mutex == 0xA -> throw Warning // successful continuation ... }
However, you should probably use a separate mutex for the tty_buffer_flush() function to exclude such a situation.
...
Cc: stable@vger.kernel.org
What commit does this fix?
I will assume that the commit of introducing mutexes in these functions: e9975fdec013 ("tty: Ensure single-threaded flip buffer consumer with mutex")
--- a/drivers/tty/tty_buffer.c +++ b/drivers/tty/tty_buffer.c @@ -226,7 +226,7 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld) atomic_inc(&buf->priority); - mutex_lock(&buf->lock); + mutex_lock(&buf->flush_mtx);
Hmm, how does this protect against concurrent buf pickup. We free it here and the racing thread can start using it, or?
Yes, assuming that such a scenario is possible..
Otherwise, if such a scenario is not possible and the patch is inappropriate, then you need to mark this mutex in some way to tell lockdep tools to ignore this place..
/* paired w/ release in __tty_buffer_request_room; ensures there are * no pending memory accesses to the freed buffer */
thanks,