[lvc-project] [PATCH] tty: Fix possible deadlock in tty_buffer_flush

[Vasiliy Kovalev]

09.05.2024 09:41, Jiri Slaby wrote:
> On 08. 05. 24, 11:30, kovalev at altlinux.org wrote:
>> From: Vasiliy Kovalev <kovalev at 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 at 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