tty: Hold write ldisc sem in tty_reopen()

In case of tty_ldisc_reinit() failure, tty->count should be decremented back, otherwise we will never release_tty().
Tetsuo reported that it fixes noisy warnings on tty release like:
pts pts4033: tty_release: tty->count(10529) != (#fd's(7) + #kopen's(0))

Fixes commit 892d1fa7eaae ("tty: Destroy ldisc instance on hangup")

Quoted 10 lines · Dmitry Safonov

In case of tty_ldisc_reinit() failure, tty->count should be decremented
back, otherwise we will never release_tty().
Tetsuo reported that it fixes noisy warnings on tty release like:
pts pts4033: tty_release: tty->count(10529) != (#fd's(7) +
#kopen's(0))

Fixes commit 892d1fa7eaae ("tty: Destroy ldisc instance on

hangup")

Minor nit, no need to have "commit" in this line, it's implied.

Also, no need to put an extra line here either.

I'll queue this patch up now, have some question about the rest...

ldsem_down_read() will sleep if there is pending writer in the queue. If the writer times out, readers in the queue should be woken up, otherwise they may miss a chance to acquire the semaphore until the last active reader will do ldsem_up_read().

There was a couple of reports where there was one active reader and other readers soft locked up:

  Showing all locks held in the system:
  2 locks held by khungtaskd/17:
   #0:  (rcu_read_lock){......}, at: watchdog+0x124/0x6d1
   #1:  (tasklist_lock){.+.+..}, at: debug_show_all_locks+0x72/0x2d3
  2 locks held by askfirst/123:
   #0:  (&tty->ldisc_sem){.+.+.+}, at: ldsem_down_read+0x46/0x58
   #1:  (&ldata->atomic_read_lock){+.+...}, at: n_tty_read+0x115/0xbe4

Prevent readers wait for active readers to release ldisc semaphore.

Link lkml.kernel.org/r/<[email protected]>

Link lkml.kernel.org/r/<20180907045041.GF1110@shao2-debian>

Cc Greg Kroah-Hartman <[email protected]>

Cc Jiri Slaby <[email protected]>

Cc Peter Zijlstra <[email protected]>

Reported-by kernel test robot <[email protected]>

Signed-off-by Dmitry Safonov <[email protected]>

---

Quoted 23 lines · Dmitry Safonov

ldsem_down_read() will sleep if there is pending writer in the queue.
If the writer times out, readers in the queue should be woken up, otherwise they may miss a chance to acquire the semaphore until the last
active reader will do ldsem_up_read().

There was a couple of reports where there was one active reader and other readers soft locked up:

  Showing all locks held in the system:
  2 locks held by khungtaskd/17:
   #0:  (rcu_read_lock){......}, at: watchdog+0x124/0x6d1
   #1:  (tasklist_lock){.+.+..}, at:
debug_show_all_locks+0x72/0x2d3
  2 locks held by askfirst/123:
   #0:  (&tty->ldisc_sem){.+.+.+}, at: ldsem_down_read+0x46/0x58
   #1:  (&ldata->atomic_read_lock){+.+...}, at:
n_tty_read+0x115/0xbe4

Prevent readers wait for active readers to release ldisc semaphore.

Link: lkml.kernel.org/r/<20171121132855.ajdv4k6swzhvktl6@wfg-t540p. sh.intel.com>

Quoted 7 lines

Link lkml.kernel.org/r/<20180907045041.GF1110@shao2-debian>

Cc Greg Kroah-Hartman <[email protected]>

Cc Jiri Slaby <[email protected]>

Cc Peter Zijlstra <[email protected]>

Reported-by kernel test robot <[email protected]>

Signed-off-by Dmitry Safonov <[email protected]>

---

Why isn't this ok for the stable trees?

tty_ldisc_reinit() doesn't race with neither tty_ldisc_hangup() nor set_ldisc() nor tty_ldisc_release() as they use tty lock. But it races with anyone who expects line discipline to be the same after hoding read semaphore in tty_ldisc_ref().

We've seen the following crash on v4.9.108 stable:

BUG: unable to handle kernel paging request at 0000000000002260
IP: [..] n_tty_receive_buf_common+0x5f/0x86d
Workqueue: events_unbound flush_to_ldisc
Call Trace:
 [..] n_tty_receive_buf2
 [..] tty_ldisc_receive_buf
 [..] flush_to_ldisc
 [..] process_one_work
 [..] worker_thread
 [..] kthread
 [..] ret_from_fork

tty_ldisc_reinit() should be called with ldisc_sem hold for writing, which will protect any reader against line discipline changes.

Backport-first: b027e2298bd5 ("tty: fix data race between tty_init_dev and flush of buf")

Quoted 19 lines · Dmitry Safonov

tty_ldisc_reinit() doesn't race with neither tty_ldisc_hangup() nor set_ldisc() nor tty_ldisc_release() as they use tty lock. But it races with anyone who expects line discipline to be the same after hoding read semaphore in tty_ldisc_ref().

We've seen the following crash on v4.9.108 stable:

BUG: unable to handle kernel paging request at 0000000000002260
IP: [..] n_tty_receive_buf_common+0x5f/0x86d
Workqueue: events_unbound flush_to_ldisc
Call Trace:
 [..] n_tty_receive_buf2
 [..] tty_ldisc_receive_buf
 [..] flush_to_ldisc
 [..] process_one_work
 [..] worker_thread
 [..] kthread
 [..] ret_from_fork

Quoted 7 lines

tty_ldisc_reinit() should be called with ldisc_sem hold for writing,
which will protect any reader against line discipline changes.

Backport-first: b027e2298bd5 ("tty: fix data race between tty_init_dev
and flush of buf")

What does this mean?

Does this require that patch for a stable patch? If so, just do:
Cc: [email protected] # b027e2298bd5 ("tty: fix data race
between tty_init_dev and flush of buf")
in the signed-off-by area. The stable documentation should describe this pretty well. If not, we can modify it to make it more obvious.

can you fix this up for the next resend of this series?

Make sure under CONFIG_LOCKDEP that each change to line discipline is done with held write semaphor.
Otherwise potential reader will have a good time dereferencing incomplete/uninitialized ldisc.

Quoted 4 lines · Dmitry Safonov

Make sure under CONFIG_LOCKDEP that each change to line discipline is done with held write semaphor.
Otherwise potential reader will have a good time dereferencing incomplete/uninitialized ldisc.

Note, I don't see patch 5/7 here, did it get lost? Please fix that up
for the next resend of this series.

Hi all,

Three fixes that worth to have in the @stable, as they were hit by different people, including Arista on v4.9 stable.

And for linux-next - adding lockdep asserts for line discipline changing code, verifying that write ldisc sem will be held forthwith.

The last patch is an optional and probably, timeout can be dropped for read_lock(). I'll do it if everyone agrees.
(Or as per discussion with Peter in v3, just convert ldisc to a regular rwsem).

Thanks,
Dima

Changes since v4:
- back to lock ldisc with (5*HZ) timeout in tty_reopen()
(LKP report link: lkml.kernel.org/r/<[email protected]>)
- reordered 3/7 with 2/7 for LKP robot

Changes since v3:
- Added tested-by Mark Rutland (thanks!)
- Dropped patch with smp_wmb() - wrong idea
- lockdep_assert_held() should be actually lockdep_assert_held_exclusive()
- Described why tty_ldisc_open() can be called without ldisc_sem held for pty slave end (o_tty).
- Added Peter's patch for dropping self-made lockdep annotations
- Fix for a reader(s) of ldisc semaphore waiting for an active reader(s)

Changes since v2:
- Added reviewed-by tags
- Hopefully, fixed reported by 0-day issue.
- Added optional fix for wait_readers decrement

Changes since v1:
- Added tested-by/reported-by tags
- Dropped 3/4 (locking tty pair for lockdep sake),
Because of that - not adding lockdep_assert_held() in tty_ldisc_open()
- Added 4/4 cleanup to inc tty->count only on success of tty_ldisc_reinit()
- lock ldisc without (5*HZ) timeout in tty_reopen()

v1 link: lkml.kernel.org/r/<[email protected]> v2 link: lkml.kernel.org/r/<[email protected]> v3 link: lkml.kernel.org/r/<[email protected]> v4 link: lkml.kernel.org/r/<[email protected]>

Cc: Daniel Axtens <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Michael Neuling <[email protected]>
Cc: Mikulas Patocka <[email protected]>
Cc: Nathan March <[email protected]>
Cc: Pasi Kärkkäinen <[email protected]>
Cc: Peter Hurley <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: "Rong, Chen" <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Cc: Tan Xiaojun <[email protected]>
Cc: Tetsuo Handa <[email protected]> (please, ignore if I Cc'ed you mistakenly)

Dmitry Safonov (6):
tty: Drop tty->count on tty_reopen() failure tty/ldsem: Wake up readers after timed out down_write()
tty: Hold tty_ldisc_lock() during tty_reopen()
tty: Simplify tty->count math in tty_reopen() tty/ldsem: Add lockdep asserts for ldisc_sem tty/ldsem: Decrement wait_readers on timeouted down_read()

Peter Zijlstra (1):
tty/ldsem: Convert to regular lockdep annotations

 drivers/tty/tty_io.c    | 13 ++++++++---
 drivers/tty/tty_ldisc.c |  9 +++++++
 drivers/tty/tty_ldsem.c | 62 ++++++++++++++++++++-----------------------------
 3 files changed, 44 insertions(+), 40 deletions(-)

Signature · 1 line

2.13.6

Quoted 13 lines · Dmitry Safonov

Hi all,

Three fixes that worth to have in the @stable, as they were hit by different people, including Arista on v4.9 stable.

And for linux-next - adding lockdep asserts for line discipline changing code, verifying that write ldisc sem will be held forthwith.

The last patch is an optional and probably, timeout can be dropped for read_lock(). I'll do it if everyone agrees.
(Or as per discussion with Peter in v3, just convert ldisc to a regular rwsem).

Quoted 2 lines

Thanks,
Dima

I confirm that this patch series fixes the crash for me.

Tested-by Mikulas Patocka <[email protected]>

Quoted 7 lines · Mikulas Patocka

Quoted 3 lines · Dmitry Safonov

Hi all,

Three fixes that worth to have in the @stable, as they were hit

by

Quoted 3 lines

Quoted 3 lines

different people, including Arista on v4.9 stable.

And for linux-next - adding lockdep asserts for line discipline

changing

Quoted 3 lines

Quoted 3 lines

code, verifying that write ldisc sem will be held forthwith.

The last patch is an optional and probably, timeout can be

dropped for

Quoted 10 lines

Quoted 6 lines

read_lock(). I'll do it if everyone agrees.
(Or as per discussion with Peter in v3, just convert ldisc to a regular rwsem).

Thanks,
Dima

I confirm that this patch series fixes the crash for me.

Tested-by Mikulas Patocka <[email protected]>

I was too quick to acknowledge this patchset. It doesn't work.

This patchset fixes the crash, but it introduces another bug - when I type
'reboot' on the console, it prints 'INIT: Switching to runlevel: 6' and
then it gets stuck for 80 seconds before proceeding with the reboot. When
I revert this patchset 'reboot' reboots the machine without any delay.
This bug was reproduced on Debian 5 userspace on pa-risc.

Quoted 32 lines · Mikulas Patocka

Quoted 7 lines · Mikulas Patocka

Quoted 3 lines · Dmitry Safonov

Hi all,

Three fixes that worth to have in the @stable, as they were hit

by

Quoted 3 lines

Quoted 3 lines

different people, including Arista on v4.9 stable.

And for linux-next - adding lockdep asserts for line discipline

changing

Quoted 3 lines

Quoted 3 lines

code, verifying that write ldisc sem will be held forthwith.

The last patch is an optional and probably, timeout can be

dropped for

Quoted 10 lines

Quoted 6 lines

read_lock(). I'll do it if everyone agrees.
(Or as per discussion with Peter in v3, just convert ldisc to a regular rwsem).

Thanks,
Dima

I confirm that this patch series fixes the crash for me.

Tested-by Mikulas Patocka <[email protected]>

I was too quick to acknowledge this patchset. It doesn't work.

Quoted 9 lines

This patchset fixes the crash, but it introduces another bug - when I type
'reboot' on the console, it prints 'INIT: Switching to runlevel: 6' and
then it gets stuck for 80 seconds before proceeding with the reboot. When
I revert this patchset 'reboot' reboots the machine without any delay.
This bug was reproduced on Debian 5 userspace on pa-risc.

Thanks much for the testing, Mikulas.
Could you try to bisect which of the patches causes it? The most important are 1,2,3 - probably, one of them caused it.. I'll stare a bit into the code.

Quoted 4 lines · Dmitry Safonov

Thanks much for the testing, Mikulas.
Could you try to bisect which of the patches causes it? The most important are 1,2,3 - probably, one of them caused it.. I'll stare a bit into the code.

The patch 3 causes it.

The hangs during reboot take either 80 seconds, 3 minutes or 3:25.

Quoted 7 lines · Mikulas Patocka

Quoted 4 lines · Dmitry Safonov

Thanks much for the testing, Mikulas.
Could you try to bisect which of the patches causes it? The most important are 1,2,3 - probably, one of them caused it.. I'll stare a bit into the code.

Quoted 3 lines

The patch 3 causes it.

The hangs during reboot take either 80 seconds, 3 minutes or 3:25.

Thanks much for that again.
Any chance you have sysrq enabled and could print: locks held in system and kernel backtraces for applications?

I suppose, userspace hangs in n_tty_receive_buf_common(), which is better of cause than null-ptr dereference, but I believe we can improve it by stopping a reader earlier if there is any writer pending.

Signature · 2 lines

Thanks,
             Dmitry

Quoted 12 lines · Dmitry Safonov

Quoted 10 lines · Mikulas Patocka

Quoted 4 lines · Dmitry Safonov

Thanks much for the testing, Mikulas.
Could you try to bisect which of the patches causes it? The most important are 1,2,3 - probably, one of them caused it.. I'll stare a bit into the code.

The patch 3 causes it.

The hangs during reboot take either 80 seconds, 3 minutes or 3:25.

Quoted 3 lines

Thanks much for that again.
Any chance you have sysrq enabled and could print: locks held in system and kernel backtraces for applications?

Stacktrace doesn't work on parisc. It used to work on 4.18, but the kernel 4.19 reportedly requires patched binutils.

Mikulas

Quoted 8 lines

I suppose, userspace hangs in n_tty_receive_buf_common(), which is better of cause than null-ptr dereference, but I believe we can improve it by stopping a reader earlier if there is any writer pending.

Signature · 2 lines

Thanks,
             Dmitry