From: Vasiliy Kovalev <[email protected]>
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: [email protected]
Signed-off-by: Vasiliy Kovalev <[email protected]>
---
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;
--
2.33.8
On 08. 05. 24, 11:30, [email protected] wrote:
> From: Vasiliy Kovalev <[email protected]>
>
> 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: [email protected]
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,
--
js
suse labs
09.05.2024 09:41, Jiri Slaby wrote:
> On 08. 05. 24, 11:30, [email protected] wrote:
>> From: Vasiliy Kovalev <[email protected]>
>>
>> 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: [email protected]
>
> 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,
--
Regards,
Vasiliy Kovalev