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(-)
--
2.19.1
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]>
Cc: [email protected]
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Dmitry Safonov <[email protected]>
---
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.19.1
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().
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
Reported-by: Mikulas Patocka <[email protected]>
Signed-off-by: Dmitry Safonov <[email protected]>
---
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(-)
diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
index dabb391909aa..99460af61b77 100644
--- a/drivers/tty/n_hdlc.c
+++ b/drivers/tty/n_hdlc.c
@@ -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;
}
diff --git a/drivers/tty/n_r3964.c b/drivers/tty/n_r3964.c
index 749a608c40b0..f75696f0ee2d 100644
--- a/drivers/tty/n_r3964.c
+++ b/drivers/tty/n_r3964.c
@@ -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;
}
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 3ad460219fd6..5dc9686697cf 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -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;
}
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index fc4c97cae01e..9434d20cf3ca 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -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);
}
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 414db2bce715..80ae5528ef8e 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -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);
--
2.19.1
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]>
Signed-off-by: Dmitry Safonov <[email protected]>
---
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 9434d20cf3ca..45eda69b150c 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -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;
--
2.19.1
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]>
Signed-off-by: Dmitry Safonov <[email protected]>
---
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.19.1
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 <[email protected]>
Cc: Jiri Slaby <[email protected]>
Link: lkml.kernel.org/r/<[email protected]>
Suggested-by: Jiri Slaby <[email protected]>
Reviewed-by: Jiri Slaby <[email protected]>
Tested-by: Mark Rutland <[email protected]>
Signed-off-by: Dmitry Safonov <[email protected]>
---
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 f73d8fa7f02b..57d06eda5b2f 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -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;
}
--
2.19.1
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")
Reviewed-by: Jiri Slaby <[email protected]>
Reported-by: [email protected]
Tested-by: Mark Rutland <[email protected]>
Tested-by: Tetsuo Handa <[email protected]>
Signed-off-by: Dmitry Safonov <[email protected]>
---
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 ee80dfbd5442..f73d8fa7f02b 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -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;
}
--
2.19.1
From: Peter Zijlstra <[email protected]>
For some reason ldsem has its own lockdep wrappers, make them go away.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Jiri Slaby <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Dmitry Safonov <[email protected]>
---
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.19.1
Hi,
On Thu, Nov 01, 2018 at 12:24:47AM +0000, 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.
>
> 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")
> Reviewed-by: Jiri Slaby <[email protected]>
> Reported-by: [email protected]
> Tested-by: Mark Rutland <[email protected]>
> Tested-by: Tetsuo Handa <[email protected]>
Feel free to add
Tested-by: Tycho Andersen <[email protected]>
to this as well. We've recently seen this bug (well, the one that
syzbot reported), and this patch fixes it.
Tycho
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
On Thu, Nov 01, 2018 at 12:24:45AM +0000, Dmitry Safonov wrote:
> 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(-)
>
> --
> 2.19.1
>
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
On Fri, Dec 07, 2018 at 12:24:20PM -0200, Guilherme G. Piccoli wrote:
> 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
Hi Guilherme, Tucho,
On 12/7/18 3:33 PM, Guilherme Piccoli wrote:
> Great news Tycho, thanks for pointing this out - in fact, the entire
> series got merged
> according
> to: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/log/?h=tty-next
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
On 07/12/2018 13:37, Dmitry Safonov wrote:
> [...]
> 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