[PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen()

[PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen()

[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).

Thanks,
Dima

Changes since v4:
- back to lock ldisc with (5*HZ) timeout in tty_reopen()
  (LKP report link: lkml.kernel.org/r/<1536940609.3185.29.camel@arista.com>)
- 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/<20180829022353.23568-1-dima@arista.com>
v2 link: lkml.kernel.org/r/<20180903165257.29227-1-dima@arista.com>
v3 link: lkml.kernel.org/r/<20180911014821.26286-1-dima@arista.com>
v4 link: lkml.kernel.org/r/<20180912001702.18522-1-dima@arista.com>

Cc: Daniel Axtens <dja@axtens.net>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Michael Neuling <mikey@neuling.org>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Nathan March <nathan@gt.net>
Cc: Pasi Kärkkäinen <pasik@iki.fi>
Cc: Peter Hurley <peter@hurleysoftware.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Rong, Chen" <rong.a.chen@intel.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Tan Xiaojun <tanxiaojun@huawei.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
(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(-)

-- 
2.13.6

[PATCHv5 1/7] tty: Drop tty->count on tty_reopen() failure

[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")

Cc: stable@vger.kernel.org # v4.6+
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Reviewed-by: Jiri Slaby <jslaby@suse.cz>
Tested-by: Jiri Slaby <jslaby@suse.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/tty_io.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 32bc3e3fe4d3..5e5da9acaf0a 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1255,6 +1255,7 @@ static void tty_driver_remove_tty(struct tty_driver *driver, struct tty_struct *
 static int tty_reopen(struct tty_struct *tty)
 {
 	struct tty_driver *driver = tty->driver;
+	int retval;
 
 	if (driver->type == TTY_DRIVER_TYPE_PTY &&
 	    driver->subtype == PTY_TYPE_MASTER)
@@ -1268,10 +1269,14 @@ static int tty_reopen(struct tty_struct *tty)
 
 	tty->count++;
 
-	if (!tty->ldisc)
-		return tty_ldisc_reinit(tty, tty->termios.c_line);
+	if (tty->ldisc)
+		return 0;
 
-	return 0;
+	retval = tty_ldisc_reinit(tty, tty->termios.c_line);
+	if (retval)
+		tty->count--;
+
+	return retval;
 }
 
 /**
-- 
2.13.6

Re: [PATCHv5 1/7] tty: Drop tty->count on tty_reopen() failure

[Greg Kroah-Hartman]

On Tue, Sep 18, 2018 at 12:52:52AM +0100, Dmitry Safonov wrote:
> 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...

thanks,

greg k-h

Re: [PATCHv5 1/7] tty: Drop tty->count on tty_reopen() failure

[Dmitry Safonov]

On Tue, 2018-09-18 at 15:41 +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 18, 2018 at 12:52:52AM +0100, Dmitry Safonov wrote:
> > 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...

Thanks, Greg, will have in mind further.

-- 
            Dima

[PATCHv5 2/7] tty/ldsem: Wake up readers after timed out down_write()

[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>
Link: lkml.kernel.org/r/<20180907045041.GF1110@shao2-debian>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Reported-by: kernel test robot <rong.a.chen@intel.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/tty_ldsem.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/tty_ldsem.c b/drivers/tty/tty_ldsem.c
index 0c98d88f795a..b989ca26fc78 100644
--- a/drivers/tty/tty_ldsem.c
+++ b/drivers/tty/tty_ldsem.c
@@ -293,6 +293,16 @@ down_write_failed(struct ld_semaphore *sem, long count, long timeout)
 	if (!locked)
 		atomic_long_add_return(-LDSEM_WAIT_BIAS, &sem->count);
 	list_del(&waiter.list);
+
+	/*
+	 * In case of timeout, wake up every reader who gave the right of way
+	 * to writer. Prevent separation readers into two groups:
+	 * one that helds semaphore and another that sleeps.
+	 * (in case of no contention with a writer)
+	 */
+	if (!locked && list_empty(&sem->write_wait))
+		__ldsem_wake_readers(sem);
+
 	raw_spin_unlock_irq(&sem->wait_lock);
 
 	__set_current_state(TASK_RUNNING);
-- 
2.13.6

Re: [PATCHv5 2/7] tty/ldsem: Wake up readers after timed out down_write()

[Greg Kroah-Hartman]

On Tue, Sep 18, 2018 at 12:52:53AM +0100, Dmitry Safonov wrote:
> 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>
> Link: lkml.kernel.org/r/<20180907045041.GF1110@shao2-debian>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Reported-by: kernel test robot <rong.a.chen@intel.com>
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---

Why isn't this ok for the stable trees?

thanks,

greg k-h

Re: [PATCHv5 2/7] tty/ldsem: Wake up readers after timed out down_write()

[Dmitry Safonov]

Hi Greg,

On Tue, 2018-09-18 at 15:43 +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 18, 2018 at 12:52:53AM +0100, Dmitry Safonov wrote:
> > 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>
> > Link: lkml.kernel.org/r/<20180907045041.GF1110@shao2-debian>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Reported-by: kernel test robot <rong.a.chen@intel.com>
> > Signed-off-by: Dmitry Safonov <dima@arista.com>
> > ---
> 
> Why isn't this ok for the stable trees?

I guess it is ok for stables, but I would prefer to have an ack from
Peter before that.

Peter, could you look if my analysis and fix are proper?
I'll fill more confident about it going to stables if it had your ack.

-- 
Thanks,
             Dmitry

[PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()

[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

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")
Cc: stable@vger.kernel.org

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Reviewed-by: Jiri Slaby <jslaby@suse.cz>
Reported-by: syzbot+3aa9784721dfb90e984d@syzkaller.appspotmail.com
Tested-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/tty_io.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 5e5da9acaf0a..3ef8b977b167 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1267,15 +1267,20 @@ static int tty_reopen(struct tty_struct *tty)
 	if (test_bit(TTY_EXCLUSIVE, &tty->flags) && !capable(CAP_SYS_ADMIN))
 		return -EBUSY;
 
-	tty->count++;
+	retval = tty_ldisc_lock(tty, 5 * HZ);
+	if (retval)
+		return retval;
 
+	tty->count++;
 	if (tty->ldisc)
-		return 0;
+		goto out_unlock;
 
 	retval = tty_ldisc_reinit(tty, tty->termios.c_line);
 	if (retval)
 		tty->count--;
 
+out_unlock:
+	tty_ldisc_unlock(tty);
 	return retval;
 }
 
-- 
2.13.6

Re: [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()

[Greg Kroah-Hartman]

On Tue, Sep 18, 2018 at 12:52:54AM +0100, Dmitry Safonov wrote:
> 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")

What does this mean?

Does this require that patch for a stable patch?  If so, just do:
 Cc: stable@vger.kernel.org  # 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?

thanks,

greg k-h

Re: [PATCHv5 3/7] tty: Hold tty_ldisc_lock() during tty_reopen()

[Dmitry Safonov]

On Tue, 2018-09-18 at 15:47 +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 18, 2018 at 12:52:54AM +0100, Dmitry Safonov wrote:
> > 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")
> 
> What does this mean?
> 
> Does this require that patch for a stable patch?  If so, just do:
>  Cc: stable@vger.kernel.org  # 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?

Sure, sorry about that non-obvious tag.

-- 
Thanks,
             Dmitry

[PATCHv5 4/7] tty: Simplify tty->count math in tty_reopen()

[Dmitry Safonov]

As notted by Jiri, tty_ldisc_reinit() shouldn't rely on tty counter.
Simplify math by increasing the counter after reinit success.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Link: lkml.kernel.org/r/<20180829022353.23568-2-dima@arista.com>
Suggested-by: Jiri Slaby <jslaby@suse.com>
Reviewed-by: Jiri Slaby <jslaby@suse.cz>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/tty_io.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 3ef8b977b167..44bf22b8d746 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1271,16 +1271,13 @@ static int tty_reopen(struct tty_struct *tty)
 	if (retval)
 		return retval;
 
-	tty->count++;
-	if (tty->ldisc)
-		goto out_unlock;
+	if (!tty->ldisc)
+		retval = tty_ldisc_reinit(tty, tty->termios.c_line);
+	tty_ldisc_unlock(tty);
 
-	retval = tty_ldisc_reinit(tty, tty->termios.c_line);
-	if (retval)
-		tty->count--;
+	if (retval == 0)
+		tty->count++;
 
-out_unlock:
-	tty_ldisc_unlock(tty);
 	return retval;
 }
 
-- 
2.13.6

[PATCHv5 5/7] tty/ldsem: Convert to regular lockdep annotations

[Dmitry Safonov]

From: Peter Zijlstra <peterz@infradead.org>

For some reason ldsem has its own lockdep wrappers, make them go away.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/tty_ldsem.c | 51 ++++++++++++++-----------------------------------
 1 file changed, 14 insertions(+), 37 deletions(-)

diff --git a/drivers/tty/tty_ldsem.c b/drivers/tty/tty_ldsem.c
index b989ca26fc78..d4d0dbf4a6d9 100644
--- a/drivers/tty/tty_ldsem.c
+++ b/drivers/tty/tty_ldsem.c
@@ -34,29 +34,6 @@
 #include <linux/sched/task.h>
 
 
-#ifdef CONFIG_DEBUG_LOCK_ALLOC
-# define __acq(l, s, t, r, c, n, i)		\
-				lock_acquire(&(l)->dep_map, s, t, r, c, n, i)
-# define __rel(l, n, i)				\
-				lock_release(&(l)->dep_map, n, i)
-#define lockdep_acquire(l, s, t, i)		__acq(l, s, t, 0, 1, NULL, i)
-#define lockdep_acquire_nest(l, s, t, n, i)	__acq(l, s, t, 0, 1, n, i)
-#define lockdep_acquire_read(l, s, t, i)	__acq(l, s, t, 1, 1, NULL, i)
-#define lockdep_release(l, n, i)		__rel(l, n, i)
-#else
-# define lockdep_acquire(l, s, t, i)		do { } while (0)
-# define lockdep_acquire_nest(l, s, t, n, i)	do { } while (0)
-# define lockdep_acquire_read(l, s, t, i)	do { } while (0)
-# define lockdep_release(l, n, i)		do { } while (0)
-#endif
-
-#ifdef CONFIG_LOCK_STAT
-# define lock_stat(_lock, stat)		lock_##stat(&(_lock)->dep_map, _RET_IP_)
-#else
-# define lock_stat(_lock, stat)		do { } while (0)
-#endif
-
-
 #if BITS_PER_LONG == 64
 # define LDSEM_ACTIVE_MASK	0xffffffffL
 #else
@@ -320,17 +297,17 @@ static int __ldsem_down_read_nested(struct ld_semaphore *sem,
 {
 	long count;
 
-	lockdep_acquire_read(sem, subclass, 0, _RET_IP_);
+	rwsem_acquire_read(&sem->dep_map, subclass, 0, _RET_IP_);
 
 	count = atomic_long_add_return(LDSEM_READ_BIAS, &sem->count);
 	if (count <= 0) {
-		lock_stat(sem, contended);
+		lock_contended(&sem->dep_map, _RET_IP_);
 		if (!down_read_failed(sem, count, timeout)) {
-			lockdep_release(sem, 1, _RET_IP_);
+			rwsem_release(&sem->dep_map, 1, _RET_IP_);
 			return 0;
 		}
 	}
-	lock_stat(sem, acquired);
+	lock_acquired(&sem->dep_map, _RET_IP_);
 	return 1;
 }
 
@@ -339,17 +316,17 @@ static int __ldsem_down_write_nested(struct ld_semaphore *sem,
 {
 	long count;
 
-	lockdep_acquire(sem, subclass, 0, _RET_IP_);
+	rwsem_acquire(&sem->dep_map, subclass, 0, _RET_IP_);
 
 	count = atomic_long_add_return(LDSEM_WRITE_BIAS, &sem->count);
 	if ((count & LDSEM_ACTIVE_MASK) != LDSEM_ACTIVE_BIAS) {
-		lock_stat(sem, contended);
+		lock_contended(&sem->dep_map, _RET_IP_);
 		if (!down_write_failed(sem, count, timeout)) {
-			lockdep_release(sem, 1, _RET_IP_);
+			rwsem_release(&sem->dep_map, 1, _RET_IP_);
 			return 0;
 		}
 	}
-	lock_stat(sem, acquired);
+	lock_acquired(&sem->dep_map, _RET_IP_);
 	return 1;
 }
 
@@ -372,8 +349,8 @@ int ldsem_down_read_trylock(struct ld_semaphore *sem)
 
 	while (count >= 0) {
 		if (atomic_long_try_cmpxchg(&sem->count, &count, count + LDSEM_READ_BIAS)) {
-			lockdep_acquire_read(sem, 0, 1, _RET_IP_);
-			lock_stat(sem, acquired);
+			rwsem_acquire_read(&sem->dep_map, 0, 1, _RET_IP_);
+			lock_acquired(&sem->dep_map, _RET_IP_);
 			return 1;
 		}
 	}
@@ -398,8 +375,8 @@ int ldsem_down_write_trylock(struct ld_semaphore *sem)
 
 	while ((count & LDSEM_ACTIVE_MASK) == 0) {
 		if (atomic_long_try_cmpxchg(&sem->count, &count, count + LDSEM_WRITE_BIAS)) {
-			lockdep_acquire(sem, 0, 1, _RET_IP_);
-			lock_stat(sem, acquired);
+			rwsem_acquire(&sem->dep_map, 0, 1, _RET_IP_);
+			lock_acquired(&sem->dep_map, _RET_IP_);
 			return 1;
 		}
 	}
@@ -413,7 +390,7 @@ void ldsem_up_read(struct ld_semaphore *sem)
 {
 	long count;
 
-	lockdep_release(sem, 1, _RET_IP_);
+	rwsem_release(&sem->dep_map, 1, _RET_IP_);
 
 	count = atomic_long_add_return(-LDSEM_READ_BIAS, &sem->count);
 	if (count < 0 && (count & LDSEM_ACTIVE_MASK) == 0)
@@ -427,7 +404,7 @@ void ldsem_up_write(struct ld_semaphore *sem)
 {
 	long count;
 
-	lockdep_release(sem, 1, _RET_IP_);
+	rwsem_release(&sem->dep_map, 1, _RET_IP_);
 
 	count = atomic_long_add_return(-LDSEM_WRITE_BIAS, &sem->count);
 	if (count < 0)
-- 
2.13.6

[PATCHv5 6/7] tty/ldsem: Add lockdep asserts for ldisc_sem

[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.

An exception here is tty_ldisc_open(), as it's called without ldisc_sem
locked by tty_init_dev() => tty_ldisc_setup() for the tty->link.

It seem valid as tty_init_dev() will call tty_driver_install_tty()
which will find ops->install(). Install will establish tty->link in
pty_common_install(), just after allocation of slave tty with
alloc_tty_struct(). So, no one should have a reference to slave pty yet.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/tty_ldisc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index fc4c97cae01e..bc0171f984a1 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -471,6 +471,7 @@ static int tty_ldisc_open(struct tty_struct *tty, struct tty_ldisc *ld)
 
 static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
 {
+	lockdep_assert_held_exclusive(&tty->ldisc_sem);
 	WARN_ON(!test_bit(TTY_LDISC_OPEN, &tty->flags));
 	clear_bit(TTY_LDISC_OPEN, &tty->flags);
 	if (ld->ops->close)
@@ -492,6 +493,7 @@ static int tty_ldisc_failto(struct tty_struct *tty, int ld)
 	struct tty_ldisc *disc = tty_ldisc_get(tty, ld);
 	int r;
 
+	lockdep_assert_held_exclusive(&tty->ldisc_sem);
 	if (IS_ERR(disc))
 		return PTR_ERR(disc);
 	tty->ldisc = disc;
@@ -615,6 +617,7 @@ EXPORT_SYMBOL_GPL(tty_set_ldisc);
  */
 static void tty_ldisc_kill(struct tty_struct *tty)
 {
+	lockdep_assert_held_exclusive(&tty->ldisc_sem);
 	if (!tty->ldisc)
 		return;
 	/*
@@ -662,6 +665,7 @@ int tty_ldisc_reinit(struct tty_struct *tty, int disc)
 	struct tty_ldisc *ld;
 	int retval;
 
+	lockdep_assert_held_exclusive(&tty->ldisc_sem);
 	ld = tty_ldisc_get(tty, disc);
 	if (IS_ERR(ld)) {
 		BUG_ON(disc == N_TTY);
@@ -760,6 +764,10 @@ int tty_ldisc_setup(struct tty_struct *tty, struct tty_struct *o_tty)
 		return retval;
 
 	if (o_tty) {
+		/*
+		 * Called without o_tty->ldisc_sem held, as o_tty has been
+		 * just allocated and no one has a reference to it.
+		 */
 		retval = tty_ldisc_open(o_tty, o_tty->ldisc);
 		if (retval) {
 			tty_ldisc_close(tty, tty->ldisc);
@@ -825,6 +833,7 @@ int tty_ldisc_init(struct tty_struct *tty)
  */
 void tty_ldisc_deinit(struct tty_struct *tty)
 {
+	/* no ldisc_sem, tty is being destroyed */
 	if (tty->ldisc)
 		tty_ldisc_put(tty->ldisc);
 	tty->ldisc = NULL;
-- 
2.13.6

Re: [PATCHv5 6/7] tty/ldsem: Add lockdep asserts for ldisc_sem

[Greg Kroah-Hartman]

On Tue, Sep 18, 2018 at 12:52:57AM +0100, Dmitry Safonov wrote:
> 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.

thanks,

greg k-h

Re: [PATCHv5 6/7] tty/ldsem: Add lockdep asserts for ldisc_sem

[Dmitry Safonov]

On Tue, 2018-09-18 at 15:49 +0200, Greg Kroah-Hartman wrote:
> On Tue, Sep 18, 2018 at 12:52:57AM +0100, Dmitry Safonov wrote:
> > 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.

Oh yeah, sorry, I've screwed up the Cc tags for that patch:
https://lore.kernel.org/lkml/20180917235258.5719-6-dima@arista.com/

Will resend.

-- 
Thanks,
             Dmitry

[PATCHv5 7/7] tty/ldsem: Decrement wait_readers on timeouted down_read()

[Dmitry Safonov]

It seems like when ldsem_down_read() fails with timeout, it misses
update for sem->wait_readers. By that reason, when writer finally
releases write end of the semaphore __ldsem_wake_readers() does adjust
sem->count with wrong value:
sem->wait_readers * (LDSEM_ACTIVE_BIAS - LDSEM_WAIT_BIAS)

I.e, if update comes with 1 missed wait_readers decrement, sem->count
will be 0x100000001 which means that there is active reader and it'll
make any further writer to fail in acquiring the semaphore.

It looks like, this is a dead-code, because ldsem_down_read() is never
called with timeout different than MAX_SCHEDULE_TIMEOUT, so it might be
worth to delete timeout parameter and error path fall-back..

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/tty_ldsem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/tty_ldsem.c b/drivers/tty/tty_ldsem.c
index d4d0dbf4a6d9..717292c1c0df 100644
--- a/drivers/tty/tty_ldsem.c
+++ b/drivers/tty/tty_ldsem.c
@@ -212,6 +212,7 @@ down_read_failed(struct ld_semaphore *sem, long count, long timeout)
 		raw_spin_lock_irq(&sem->wait_lock);
 		if (waiter.task) {
 			atomic_long_add_return(-LDSEM_WAIT_BIAS, &sem->count);
+			sem->wait_readers--;
 			list_del(&waiter.list);
 			raw_spin_unlock_irq(&sem->wait_lock);
 			put_task_struct(waiter.task);
-- 
2.13.6

Re: [PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen()

[Mikulas Patocka]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3333 bytes --]



On Tue, 18 Sep 2018, Dmitry Safonov wrote:

> 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

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

Tested-by: Mikulas Patocka <mpatocka@redhat.com>


> Changes since v4:
> - back to lock ldisc with (5*HZ) timeout in tty_reopen()
>   (LKP report link: lkml.kernel.org/r/<1536940609.3185.29.camel@arista.com>)
> - 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/<20180829022353.23568-1-dima@arista.com>
> v2 link: lkml.kernel.org/r/<20180903165257.29227-1-dima@arista.com>
> v3 link: lkml.kernel.org/r/<20180911014821.26286-1-dima@arista.com>
> v4 link: lkml.kernel.org/r/<20180912001702.18522-1-dima@arista.com>
> 
> Cc: Daniel Axtens <dja@axtens.net>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Michael Neuling <mikey@neuling.org>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: Nathan March <nathan@gt.net>
> Cc: Pasi Kärkkäinen <pasik@iki.fi>
> Cc: Peter Hurley <peter@hurleysoftware.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: "Rong, Chen" <rong.a.chen@intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Cc: Tan Xiaojun <tanxiaojun@huawei.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> (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(-)
> 
> -- 
> 2.13.6
> 

Re: [PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen()

[Mikulas Patocka]

On Wed, 19 Sep 2018, Mikulas Patocka wrote:

> 
> 
> On Tue, 18 Sep 2018, Dmitry Safonov wrote:
> 
> > 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
> 
> I confirm that this patch series fixes the crash for me.
> 
> Tested-by: Mikulas Patocka <mpatocka@redhat.com>

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.

Mikulas

Re: [PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen()

[Dmitry Safonov]

On Wed, 2018-09-19 at 13:35 -0400, Mikulas Patocka wrote:
> 
> On Wed, 19 Sep 2018, Mikulas Patocka wrote:
> 
> > 
> > 
> > On Tue, 18 Sep 2018, Dmitry Safonov wrote:
> > 
> > > 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
> > 
> > I confirm that this patch series fixes the crash for me.
> > 
> > Tested-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> 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.

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.

-- 
Thanks,
             Dmitry

Re: [PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen()

[Mikulas Patocka]

On Wed, 19 Sep 2018, Dmitry Safonov wrote:

> On Wed, 2018-09-19 at 13:35 -0400, Mikulas Patocka wrote:
> > 
> > On Wed, 19 Sep 2018, Mikulas Patocka wrote:
> > 
> > > 
> > > 
> > > On Tue, 18 Sep 2018, Dmitry Safonov wrote:
> > > 
> > > > 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
> > > 
> > > I confirm that this patch series fixes the crash for me.
> > > 
> > > Tested-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > 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.
> 
> 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.

Mikulas

Re: [PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen()

[Dmitry Safonov]

On Wed, 2018-09-19 at 16:03 -0400, Mikulas Patocka wrote:
> 
> On Wed, 19 Sep 2018, Dmitry Safonov wrote:
> > 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.

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.

-- 
Thanks,
             Dmitry

Re: [PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen()

[Mikulas Patocka]

On Wed, 19 Sep 2018, Dmitry Safonov wrote:

> On Wed, 2018-09-19 at 16:03 -0400, Mikulas Patocka wrote:
> > 
> > On Wed, 19 Sep 2018, Dmitry Safonov wrote:
> > > 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.
> 
> 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

> 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.
> 
> -- 
> Thanks,
>              Dmitry
> 

Re: [PATCHv5 0/7] tty: Hold write ldisc sem in tty_reopen()

[Pasi Kärkkäinen]

Hi,

On Thu, Sep 20, 2018 at 10:25:01AM -0400, Mikulas Patocka wrote:
> 
> 
> On Wed, 19 Sep 2018, Dmitry Safonov wrote:
> 
> > On Wed, 2018-09-19 at 16:03 -0400, Mikulas Patocka wrote:
> > > 
> > > On Wed, 19 Sep 2018, Dmitry Safonov wrote:
> > > > 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.
> > 
> > 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
> 
> > 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.
> > 
> > -- 
> > Thanks,
> >              Dmitry
> > 

Any progress here? Or is more info/stacktrace needed about the hang?


Thanks,

-- Pasi