Messages in this thread Patch in this message | | | Date | Thu, 9 May 2024 13:32:43 +0300 | Subject | Re: [PATCH] tty: Fix possible deadlock in tty_buffer_flush | From | Vasiliy Kovalev <> |
| |
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,
-- Regards, Vasiliy Kovalev
| |