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. However, you should probably use a separate mutex for the tty_buffer_flush() function to exclude such a situation.
Found by Syzkaller: ====================================================== WARNING: possible circular locking dependency detected 5.10.213-std-def-alt1 #1 Not tainted ------------------------------------------------------ kworker/u6:8/428 is trying to acquire lock: ffff88810c3498b8 (&buf->lock){+.+.}-{3:3}, at: tty_buffer_flush+0x7b/0x2b0 drivers/tty/tty_buffer.c:228
but task is already holding lock: ffff888114dca2e8 (&o_tty->termios_rwsem/1){++++}-{3:3}, at: isig+0xef/0x440 drivers/tty/n_tty.c:1127
which lock already depends on the new lock.
Chain exists of: &buf->lock --> &port->buf.lock/1 --> &o_tty->termios_rwsem/1
Possible unsafe locking scenario:
CPU0 CPU1 ---- ---- lock(&o_tty->termios_rwsem/1); lock(&port->buf.lock/1); lock(&o_tty->termios_rwsem/1); lock(&buf->lock);
stack backtrace: CPU: 0 PID: 428 Comm: kworker/u6:8 Not tainted 5.10.213-std-def-alt1 #1 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.0-alt1 04/01/2014 Workqueue: events_unbound flush_to_ldisc Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x19b/0x203 lib/dump_stack.c:118 print_circular_bug.cold+0x162/0x171 kernel/locking/lockdep.c:2002 check_noncircular+0x263/0x2e0 kernel/locking/lockdep.c:2123 check_prev_add kernel/locking/lockdep.c:2988 [inline] check_prevs_add kernel/locking/lockdep.c:3113 [inline] validate_chain kernel/locking/lockdep.c:3729 [inline] __lock_acquire+0x298f/0x5500 kernel/locking/lockdep.c:4955 lock_acquire kernel/locking/lockdep.c:5566 [inline] lock_acquire+0x1fe/0x550 kernel/locking/lockdep.c:5531 __mutex_lock_common kernel/locking/mutex.c:968 [inline] __mutex_lock+0x142/0x10c0 kernel/locking/mutex.c:1109 mutex_lock_nested+0x17/0x20 kernel/locking/mutex.c:1124 tty_buffer_flush+0x7b/0x2b0 drivers/tty/tty_buffer.c:228 pty_flush_buffer+0x4e/0x170 drivers/tty/pty.c:222 tty_driver_flush_buffer+0x65/0x80 drivers/tty/tty_ioctl.c:96 isig+0x1e4/0x440 drivers/tty/n_tty.c:1138 n_tty_receive_signal_char+0x24/0x160 drivers/tty/n_tty.c:1239 n_tty_receive_char_special+0x1261/0x2a70 drivers/tty/n_tty.c:1285 n_tty_receive_buf_fast drivers/tty/n_tty.c:1606 [inline] __receive_buf drivers/tty/n_tty.c:1640 [inline] n_tty_receive_buf_common+0x1e76/0x2b60 drivers/tty/n_tty.c:1738 n_tty_receive_buf2+0x34/0x40 drivers/tty/n_tty.c:1773 tty_ldisc_receive_buf+0xb1/0x1a0 drivers/tty/tty_buffer.c:441 tty_port_default_receive_buf+0x73/0xa0 drivers/tty/tty_port.c:39 receive_buf drivers/tty/tty_buffer.c:461 [inline] flush_to_ldisc+0x21c/0x400 drivers/tty/tty_buffer.c:513 process_one_work+0x9ae/0x14b0 kernel/workqueue.c:2282 worker_thread+0x622/0x1320 kernel/workqueue.c:2428 kthread+0x396/0x470 kernel/kthread.c:313 ret_from_fork+0x22/0x30 arch/x86/entry/entry_64.S:299
Cc: stable@vger.kernel.org Signed-off-by: Vasiliy Kovalev kovalev@altlinux.org --- drivers/tty/tty_buffer.c | 5 +++-- include/linux/tty_buffer.h | 1 + 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c index 79f0ff94ce00da..e777bd5f3a2fca 100644 --- 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); /* paired w/ release in __tty_buffer_request_room; ensures there are * no pending memory accesses to the freed buffer */ @@ -241,7 +241,7 @@ void tty_buffer_flush(struct tty_struct *tty, struct tty_ldisc *ld) ld->ops->flush_buffer(tty);
atomic_dec(&buf->priority); - mutex_unlock(&buf->lock); + mutex_unlock(&buf->flush_mtx); }
/** @@ -577,6 +577,7 @@ void tty_buffer_init(struct tty_port *port) { struct tty_bufhead *buf = &port->buf;
+ mutex_init(&buf->flush_mtx); mutex_init(&buf->lock); tty_buffer_reset(&buf->sentinel, 0); buf->head = &buf->sentinel; diff --git a/include/linux/tty_buffer.h b/include/linux/tty_buffer.h index 31125e3be3c55e..cea4eacc3b70d3 100644 --- a/include/linux/tty_buffer.h +++ b/include/linux/tty_buffer.h @@ -35,6 +35,7 @@ static inline u8 *flag_buf_ptr(struct tty_buffer *b, unsigned int ofs) struct tty_bufhead { struct tty_buffer *head; /* Queue head */ struct work_struct work; + struct mutex flush_mtx; /* For use in tty_buffer_flush() */ struct mutex lock; atomic_t priority; struct tty_buffer sentinel;
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)?
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?
--- 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?
/* paired w/ release in __tty_buffer_request_room; ensures there are * no pending memory accesses to the freed buffer */
thanks,
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,
linux-stable-mirror@lists.linaro.org
-
Jiri Slaby -
kovalev＠altlinux.org -
Vasiliy Kovalev