Skip to contentSkip to conversation

tty: Hold write ldisc sem in tty_reopen()

PATCH In review Dmitry Safonov 4 participants 14 messages Nov 1, 2018 – Dec 7, 2018 1 message not in the archive

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v6 cover tty: Hold write ldisc sem in tty_reopen()

Hi all,

Here are 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.

Mikulas, can you add your tested-by on this patches set again, please? I tried to reproduce reboot issue on qemu-hppa and even built cross-compiler for pa-risc.. but was unlucky in reproducing.

Thanks,
Dima

Changes since v5:
- Better commit tags
- Hopefully fixed issue with reboot on pa-risc with Debian 5

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/ldsem: Wake up readers after timed out down_write()
tty: Hold tty_ldisc_lock() during tty_reopen()
tty: Don't block on IO when ldisc change is pending
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/n_hdlc.c    |  4 +--
 drivers/tty/n_r3964.c   |  2 +-
 drivers/tty/n_tty.c     |  8 +++---
 drivers/tty/tty_io.c    | 14 ++++++----
 drivers/tty/tty_ldisc.c | 16 +++++++++++
 drivers/tty/tty_ldsem.c | 62 +++++++++++++++++------------------------
 include/linux/tty.h     |  7 +++++
 7 files changed, 63 insertions(+), 50 deletions(-)
Signature · 1 line
2.19.1
#1 lore raw

PATCH v6 1/7 tty/ldsem: Wake up readers after timed out down_write()

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]>
---
 drivers/tty/tty_ldsem.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/tty/tty_ldsem.c +10 -0
index 0c98d88f795a..b989ca26fc78 100644
@@ -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);
Signature · 1 line
2.19.1
#2 lore raw

PATCH v6 2/7 tty: Hold tty_ldisc_lock() during tty_reopen()

In reply to 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.

Cc Greg Kroah-Hartman <[email protected]>
Cc Jiri Slaby <[email protected]>
Cc [email protected] # b027e2298bd5 ("tty: fix data race between tty_init_dev and flush of buf")
Reported-by [email protected]
---
 drivers/tty/tty_io.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/tty/tty_io.c +7 -2
index ee80dfbd5442..f73d8fa7f02b 100644
@@ -1268,15 +1268,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;
 }
 
Signature · 1 line
2.19.1
Tested-by on v6 2/7
#3 lore raw

Re: [PATCHv6 2/7] tty: Hold tty_ldisc_lock() during tty_reopen()

Hi,

Quoted 22 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

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

Quoted 7 lines
Cc Greg Kroah-Hartman <[email protected]>
Cc Jiri Slaby <[email protected]>
Cc [email protected] # b027e2298bd5 ("tty: fix data race between tty_init_dev and flush of buf")
Reported-by [email protected]

Feel free to add

to this as well. We've recently seen this bug (well, the one that syzbot reported), and this patch fixes it.

Tycho

#4 lore raw

PATCH v6 3/7 tty: Don't block on IO when ldisc change is pending

In reply to Dmitry Safonov

There might be situations where tty_ldisc_lock() has blocked, but there is already IO on tty and it prevents line discipline changes. It might theoretically turn into dead-lock.

Basically, provide more priority to pending tty_ldisc_lock() than to servicing reads/writes over tty.

User-visible issue was reported by Mikulas where on pa-risc with Debian 5 reboot took either 80 seconds, 3 minutes or 3:25 after proper locking in tty_reopen().

---
 drivers/tty/n_hdlc.c    | 4 ++--
 drivers/tty/n_r3964.c   | 2 +-
 drivers/tty/n_tty.c     | 8 ++++----
 drivers/tty/tty_ldisc.c | 7 +++++++
 include/linux/tty.h     | 7 +++++++
 5 files changed, 21 insertions(+), 7 deletions(-)
drivers/tty/n_hdlc.c +2 -2
index dabb391909aa..99460af61b77 100644
@@ -612,7 +612,7 @@ static ssize_t n_hdlc_tty_read(struct tty_struct *tty, struct file *file,
 		}
 			
 		/* no data */
-		if (file->f_flags & O_NONBLOCK) {
+		if (tty_io_nonblock(tty, file)) {
 			ret = -EAGAIN;
 			break;
 		}
@@ -679,7 +679,7 @@ static ssize_t n_hdlc_tty_write(struct tty_struct *tty, struct file *file,
 		if (tbuf)
 			break;
 
-		if (file->f_flags & O_NONBLOCK) {
+		if (tty_io_nonblock(tty, file)) {
 			error = -EAGAIN;
 			break;
 		}
drivers/tty/n_r3964.c +1 -1
index 749a608c40b0..f75696f0ee2d 100644
@@ -1085,7 +1085,7 @@ static ssize_t r3964_read(struct tty_struct *tty, struct file *file,
 		pMsg = remove_msg(pInfo, pClient);
 		if (pMsg == NULL) {
 			/* no messages available. */
-			if (file->f_flags & O_NONBLOCK) {
+			if (tty_io_nonblock(tty, file)) {
 				ret = -EAGAIN;
 				goto unlock;
 			}
drivers/tty/n_tty.c +4 -4
index 3ad460219fd6..5dc9686697cf 100644
@@ -1702,7 +1702,7 @@ n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
 
 	down_read(&tty->termios_rwsem);
 
-	while (1) {
+	do {
 		/*
 		 * When PARMRK is set, each input char may take up to 3 chars
 		 * in the read buf; reduce the buffer space avail by 3x
@@ -1744,7 +1744,7 @@ n_tty_receive_buf_common(struct tty_struct *tty, const unsigned char *cp,
 			fp += n;
 		count -= n;
 		rcvd += n;
-	}
+	} while (!test_bit(TTY_LDISC_CHANGING, &tty->flags));
 
 	tty->receive_room = room;
 
@@ -2211,7 +2211,7 @@ static ssize_t n_tty_read(struct tty_struct *tty, struct file *file,
 					break;
 				if (!timeout)
 					break;
-				if (file->f_flags & O_NONBLOCK) {
+				if (tty_io_nonblock(tty, file)) {
 					retval = -EAGAIN;
 					break;
 				}
@@ -2365,7 +2365,7 @@ static ssize_t n_tty_write(struct tty_struct *tty, struct file *file,
 		}
 		if (!nr)
 			break;
-		if (file->f_flags & O_NONBLOCK) {
+		if (tty_io_nonblock(tty, file)) {
 			retval = -EAGAIN;
 			break;
 		}
drivers/tty/tty_ldisc.c +7 -0
index fc4c97cae01e..9434d20cf3ca 100644
@@ -327,6 +327,11 @@ int tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
 {
 	int ret;
 
+	/* Kindly asking blocked readers to release the read side */
+	set_bit(TTY_LDISC_CHANGING, &tty->flags);
+	wake_up_interruptible_all(&tty->read_wait);
+	wake_up_interruptible_all(&tty->write_wait);
+
 	ret = __tty_ldisc_lock(tty, timeout);
 	if (!ret)
 		return -EBUSY;
@@ -337,6 +342,8 @@ int tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout)
 void tty_ldisc_unlock(struct tty_struct *tty)
 {
 	clear_bit(TTY_LDISC_HALTED, &tty->flags);
+	/* Can be cleared here - ldisc_unlock will wake up writers firstly */
+	clear_bit(TTY_LDISC_CHANGING, &tty->flags);
 	__tty_ldisc_unlock(tty);
 }
 
include/linux/tty.h +7 -0
index 414db2bce715..80ae5528ef8e 100644
@@ -366,6 +366,7 @@ struct tty_file_private {
 #define TTY_NO_WRITE_SPLIT 	17	/* Preserve write boundaries to driver */
 #define TTY_HUPPED 		18	/* Post driver->hangup() */
 #define TTY_HUPPING		19	/* Hangup in progress */
+#define TTY_LDISC_CHANGING	20	/* Change pending - non-block IO */
 #define TTY_LDISC_HALTED	22	/* Line discipline is halted */
 
 /* Values for tty->flow_change */
@@ -383,6 +384,12 @@ static inline void tty_set_flow_change(struct tty_struct *tty, int val)
 	smp_mb();
 }
 
+static inline bool tty_io_nonblock(struct tty_struct *tty, struct file *file)
+{
+	return file->f_flags & O_NONBLOCK ||
+		test_bit(TTY_LDISC_CHANGING, &tty->flags);
+}
+
 static inline bool tty_io_error(struct tty_struct *tty)
 {
 	return test_bit(TTY_IO_ERROR, &tty->flags);
Signature · 1 line
2.19.1
#5 lore raw

PATCH v6 4/7 tty: Simplify tty->count math in tty_reopen()

In reply to Dmitry Safonov

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

---
 drivers/tty/tty_io.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/tty/tty_io.c +5 -8
index f73d8fa7f02b..57d06eda5b2f 100644
@@ -1272,16 +1272,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;
 }
 
Signature · 1 line
2.19.1
#6 lore raw

PATCH v6 5/7 tty/ldsem: Convert to regular lockdep annotations

In reply to Dmitry Safonov

From: Peter Zijlstra <[email protected]>

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

---
 drivers/tty/tty_ldsem.c | 51 +++++++++++------------------------------
 1 file changed, 14 insertions(+), 37 deletions(-)
drivers/tty/tty_ldsem.c +14 -37
index b989ca26fc78..d4d0dbf4a6d9 100644
@@ -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)
Signature · 1 line
2.19.1
#7 lore raw

PATCH v6 6/7 tty/ldsem: Add lockdep asserts for ldisc_sem

In reply to 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 <[email protected]>
Cc Jiri Slaby <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
 drivers/tty/tty_ldisc.c | 9 +++++++++
 1 file changed, 9 insertions(+)
drivers/tty/tty_ldisc.c +9 -0
index 9434d20cf3ca..45eda69b150c 100644
@@ -478,6 +478,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)
@@ -499,6 +500,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;
@@ -622,6 +624,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;
 	/*
@@ -669,6 +672,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);
@@ -767,6 +771,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);
@@ -832,6 +840,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;
Signature · 1 line
2.19.1
#8 lore raw

PATCH v6 7/7 tty/ldsem: Decrement wait_readers on timeouted down_read()

In reply to 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 <[email protected]>
Cc Jiri Slaby <[email protected]>
---
 drivers/tty/tty_ldsem.c | 1 +
 1 file changed, 1 insertion(+)
drivers/tty/tty_ldsem.c +1 -0
index d4d0dbf4a6d9..717292c1c0df 100644
@@ -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);
Signature · 1 line
2.19.1
#9 lore raw
In reply to Dmitry Safonov

Hello Mikulas,

Any chance you could re-test these v6 patches? It seems you're able to relatively easily trigger the bug on your (pa-risc) environment.. I'd be good to have this series finally acked/merged!

Thanks a lot,

-- Pasi

Quoted 89 lines · Dmitry Safonov

Hi all,

Here are 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.

Mikulas, can you add your tested-by on this patches set again, please? I tried to reproduce reboot issue on qemu-hppa and even built cross-compiler for pa-risc.. but was unlucky in reproducing.

Thanks,
Dima

Changes since v5:
- Better commit tags
- Hopefully fixed issue with reboot on pa-risc with Debian 5

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/ldsem: Wake up readers after timed out down_write()
tty: Hold tty_ldisc_lock() during tty_reopen()
tty: Don't block on IO when ldisc change is pending
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/n_hdlc.c    |  4 +--
 drivers/tty/n_r3964.c   |  2 +-
 drivers/tty/n_tty.c     |  8 +++---
 drivers/tty/tty_io.c    | 14 ++++++----
 drivers/tty/tty_ldisc.c | 16 +++++++++++
 drivers/tty/tty_ldsem.c | 62 +++++++++++++++++------------------------
 include/linux/tty.h     |  7 +++++
 7 files changed, 63 insertions(+), 50 deletions(-)
Signature · 1 line
2.19.1
#10 lore raw

Hi, thanks Dmitry for the re-spin - hopefully now the pa-risc issues are fixed.

BTW, any news on the pa-risc testing? We're just waiting on this to get the patchset merged?

Thanks in advance,

Guilherme

#11 lore raw
Quoted 5 lines · Guilherme G. Piccoli

Hi, thanks Dmitry for the re-spin - hopefully now the pa-risc issues are fixed.

BTW, any news on the pa-risc testing? We're just waiting on this to get the patchset merged?

As far as I know it has been, I got a mail from Greg yesterday about the patch I tested and it looks to be in tty-next,

https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=83d817f41070c48bc3eb7ec18e43000a548fca5c

So maybe any fixes in this series need to go on top?

Tycho

A message that is not in the archive (sent off-list or not yet received): CAHD1Q_yPbhTYcKwk2PUkoabD+dTqegkVEab7nAvcrgAN10otSQ@mail.gmail.com

#14 lore raw
Quoted 9 lines · Dmitry Safonov

[...]
Yes, Greg kindly took the patches set into -next to increase coverage. So far, there is no standing issues known to me.

Ping me, if you have any, please.

Thanks,
Dmitry

Thanks Dmitry, hopefully this time the series gets merged in Linus tree and we have the fix for this long-standing issue. Cheers,

Guilherme