Re: [PATCH] tty: Fix possible deadlock in tty_buffer_flush

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



09.05.2024 09:41, Jiri Slaby wrote:
On 08. 05. 24, 11:30, kovalev@xxxxxxxxxxxx wrote:
From: Vasiliy Kovalev <kovalev@xxxxxxxxxxxx>

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@xxxxxxxxxxxxxxx

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




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux