lkml.org 
[lkml]   [2024]   [May]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] tty: Fix possible deadlock in tty_buffer_flush
From
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

\
 
 \ /
  Last update: 2024-05-09 12:33    [W:0.035 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site