tty: Hold write ldisc sem in tty_reopen()
Subsystems (2)
v3 cover tty: Hold write ldisc sem in tty_reopen()
Hi all,
Three fixes that worth to have in the @stable, as we've hit them 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 optional and probably, timeout can be dropped for read_lock(). I'll do it if everyone agrees.
Rong Chen, could you kindly re-run this version to see if the lockup from v1 still happens? I wasn't able to reproduce it..
Thanks,
Dima
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]>
Huuge cc list:
Cc: Daniel Axtens <[email protected]>
Cc: Dmitry Vyukov <[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: "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: Update waiter->task before waking up reader
tty: Hold tty_ldisc_lock() during tty_reopen() tty/lockdep: Add ldisc_sem asserts
tty: Simplify tty->count math in tty_reopen() tty/ldsem: Decrement wait_readers on timeouted down_read()
drivers/tty/tty_io.c | 12 ++++++++---- drivers/tty/tty_ldisc.c | 5 +++++ drivers/tty/tty_ldsem.c | 5 ++++- 3 files changed, 17 insertions(+), 5 deletions(-)
Signature · 1 line
2.13.6
PATCH v3 1/6 tty: Drop tty->count on tty_reopen() failure
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))
--- drivers/tty/tty_io.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
drivers/tty/tty_io.c +8 -3
@@ -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; }/**
Signature · 1 line
2.13.6
PATCH v3 2/6 tty/ldsem: Update waiter->task before waking up reader
In reply to Dmitry SafonovThere is a couple of reports about lockup in ldsem_down_read() without anyone holding write end of ldisc semaphore:
lkml.kernel.org/r/<[email protected]> lkml.kernel.org/r/<20180907045041.GF1110@shao2-debian>
They all looked like a missed wake up.
I wasn't lucky enough to reproduce it, but it seems like reader on another CPU can miss waiter->task update and schedule again, resulting in indefinite (MAX_SCHEDULE_TIMEOUT) sleep.
Make sure waked up reader will see waiter->task == NULL.
--- drivers/tty/tty_ldsem.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/tty/tty_ldsem.c +3 -1
@@ -118,6 +118,8 @@ static void __ldsem_wake_readers(struct ld_semaphore *sem) tsk = waiter->task;smp_mb (); waiter->task =NULL ; +/* Make sure down_read_failed() will see !waiter->task update */ +smp_wmb ();wake_up_process (tsk);put_task_struct (tsk); }
@@ -217,7 +219,7 @@ down_read_failed(struct ld_semaphore *sem, long count, long timeout)for (;;) {set_current_state (TASK_UNINTERRUPTIBLE ); -if (!waiter.task) +if (!READ_ONCE (waiter.task))break ;if (!timeout)break ;
Signature · 1 line
2.13.6
Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader
Quoted 9 lines · Dmitry Safonov
There is a couple of reports about lockup in
ldsem_down_read()without anyone holding write end of ldisc semaphore:
lkml.kernel.org/r/<[email protected]> lkml.kernel.org/r/<20180907045041.GF1110@shao2-debian>They all looked like a missed wake up.
I wasn't lucky enough to reproduce it, but it seems like reader on another CPU can misswaiter->taskupdate and schedule again, resulting in indefinite (MAX_SCHEDULE_TIMEOUT) sleep.
Certainly, something suspicious is going on.
drivers/tty/tty_ldsem.c static void __ldsem_wake_readers(struct ld_semaphore *sem) · line 123
Quoted 7 lines
drivers/tty/tty_ldsem.c +2
-0@@ -118,6 +118,8 @@ static void __ldsem_wake_readers(struct ld_semaphore *sem) tsk = waiter->task;smp_mb (); waiter->task =NULL ; +/* Make sure down_read_failed() will see !waiter->task update */ +smp_wmb ();wake_up_process (tsk);
Hmm. I think wake_up_process() executes a full memory barrier, because it accesses task state.
drivers/tty/tty_ldsem.c down_read_failed(struct ld_semaphore *sem, long count, long timeout) · line 220
Quoted 5 lines
drivers/tty/tty_ldsem.c +0
-0put_task_struct (tsk); }@@ -217,7 +219,7 @@ down_read_failed(struct ld_semaphore *sem, long count, long timeout)for (;;) {set_current_state (TASK_UNINTERRUPTIBLE );
I think that set_current_state() also executes memory barrier. Just because it accesses task state.
drivers/tty/tty_ldsem.c down_read_failed(struct ld_semaphore *sem, long count, long timeout) · line 225
Quoted 5 lines
drivers/tty/tty_ldsem.c +1
-1-if (!waiter.task) +if (!READ_ONCE (waiter.task))break ;if (!timeout)break ;
-ss
Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader
Quoted 11 lines · Sergey Senozhatsky
drivers/tty/tty_ldsem.c down_read_failed(struct ld_semaphore *sem, long count, long timeout) · line 220
Quoted 2 lines
drivers/tty/tty_ldsem.c +0
-0for (;;) {set_current_state (TASK_UNINTERRUPTIBLE );I think that
set_current_state()also executes memory barrier. Just because it accesses task state.drivers/tty/tty_ldsem.c down_read_failed(struct ld_semaphore *sem, long count, long timeout) · line 225
Quoted 5 lines
drivers/tty/tty_ldsem.c +1
-1-if (!waiter.task) +if (!READ_ONCE (waiter.task))break ;if (!timeout)break ;
This READ_ONCE(waiter.task) looks interesting. Maybe could be moved to a loop condition
while (!READ_ONCE(waiter.task)) {
...
}
-ssRe: Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader
Quoted 16 lines · Sergey Senozhatsky
Quoted 11 lines · Sergey Senozhatsky
drivers/tty/tty_ldsem.c down_read_failed(struct ld_semaphore *sem, long count, long timeout) · line 220
Quoted 2 lines
drivers/tty/tty_ldsem.c +0
-0for (;;) {set_current_state (TASK_UNINTERRUPTIBLE );I think that
set_current_state()also executes memory barrier. Just because it accesses task state.drivers/tty/tty_ldsem.c down_read_failed(struct ld_semaphore *sem, long count, long timeout) · line 225
Quoted 5 lines
drivers/tty/tty_ldsem.c +1
-1-if (!waiter.task) +if (!READ_ONCE (waiter.task))break ;if (!timeout)break ;This
READ_ONCE(waiter.task)looks interesting. Maybe could be moved to a loop condition
Quoted 3 lines
while (!READ_ONCE(waiter.task)) {
...
}We can't reorder event check and set_current_state(), because this will lead to missing of wakeup:
Documentation/memory-barriers.txt
Also, it looks like READ_ONCE() is not need. In case of compiler had optimized this, then all wait_event() in kernel w/o READ_ONCE would have not worked like expected, wouldn't they?
Kirill
Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader
In reply to Sergey SenozhatskyQuoted 16 lines · Sergey Senozhatsky
Quoted 11 lines · Sergey Senozhatsky
drivers/tty/tty_ldsem.c down_read_failed(struct ld_semaphore *sem, long count, long timeout) · line 220
Quoted 2 lines
drivers/tty/tty_ldsem.c +0
-0for (;;) {set_current_state (TASK_UNINTERRUPTIBLE );I think that
set_current_state()also executes memory barrier. Just because it accesses task state.drivers/tty/tty_ldsem.c down_read_failed(struct ld_semaphore *sem, long count, long timeout) · line 225
Quoted 5 lines
drivers/tty/tty_ldsem.c +1
-1-if (!waiter.task) +if (!READ_ONCE (waiter.task))break ;if (!timeout)break ;This
READ_ONCE(waiter.task)looks interesting. Maybe could be moved to a loop condition
Quoted 3 lines
while (!READ_ONCE(waiter.task)) {
...
}No, it must be after set_current_state(). See that same comment.
Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader
In reply to Sergey SenozhatskyQuoted 24 lines · Sergey Senozhatsky
Quoted 9 lines · Dmitry Safonov
There is a couple of reports about lockup in
ldsem_down_read()without anyone holding write end of ldisc semaphore:
lkml.kernel.org/r/<[email protected]> lkml.kernel.org/r/<20180907045041.GF1110@shao2-debian>They all looked like a missed wake up.
I wasn't lucky enough to reproduce it, but it seems like reader on another CPU can misswaiter->taskupdate and schedule again, resulting in indefinite (MAX_SCHEDULE_TIMEOUT) sleep.Certainly, something suspicious is going on.
drivers/tty/tty_ldsem.c static void __ldsem_wake_readers(struct ld_semaphore *sem) · line 123
Quoted 7 lines
drivers/tty/tty_ldsem.c +2
-0@@ -118,6 +118,8 @@ static void __ldsem_wake_readers(struct ld_semaphore *sem) tsk = waiter->task;smp_mb (); waiter->task =NULL ; +/* Make sure down_read_failed() will see !waiter->task update */ +smp_wmb ();wake_up_process (tsk);Hmm. I think
wake_up_process()executes a full memory barrier, because it accesses task state.
Quoted 8 lines
drivers/tty/tty_ldsem.c down_read_failed(struct ld_semaphore *sem, long count, long timeout) · line 220
Quoted 5 lines
drivers/tty/tty_ldsem.c +0
-0put_task_struct (tsk); }@@ -217,7 +219,7 @@ down_read_failed(struct ld_semaphore *sem, long count, long timeout)for (;;) {set_current_state (TASK_UNINTERRUPTIBLE );I think that
set_current_state()also executes memory barrier. Just because it accesses task state.
In both cases, the rationale, 'because it accesses task state' is utterly wrong.
The reasoning can be found in the comment near set_current_state().
Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader
In reply to Dmitry SafonovQuoted 15 lines · Dmitry Safonov
There is a couple of reports about lockup in
ldsem_down_read()without anyone holding write end of ldisc semaphore:
lkml.kernel.org/r/<[email protected]> lkml.kernel.org/r/<20180907045041.GF1110@shao2-debian>They all looked like a missed wake up.
I wasn't lucky enough to reproduce it, but it seems like reader on another CPU can misswaiter->taskupdate and schedule again, resulting in indefinite (MAX_SCHEDULE_TIMEOUT) sleep.Make sure waked up reader will see
waiter->task== NULL.
drivers/tty/tty_ldsem.c static void __ldsem_wake_readers(struct ld_semaphore *sem) · line 123
Quoted 6 lines
drivers/tty/tty_ldsem.c +2
-0@@ -118,6 +118,8 @@ static void __ldsem_wake_readers(struct ld_semaphore *sem) tsk = waiter->task;smp_mb (); waiter->task =NULL ; +/* Make sure down_read_failed() will see !waiter->task update */ +smp_wmb ();wake_up_process (tsk);
This is 'wrong', wake_up_process() should imply sufficient for this to already be true.
Quoted 2 lines
drivers/tty/tty_ldsem.c +0
-0put_task_struct (tsk); }
Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader
Quoted 23 lines · Peter Zijlstra
Quoted 22 lines · Dmitry Safonov
There is a couple of reports about lockup in
ldsem_down_read()without
anyone holding write end of ldisc semaphore:
lkml.kernel.org/r/<[email protected] el.com> lkml.kernel.org/r/<20180907045041.GF1110@shao2-debian>They all looked like a missed wake up.
I wasn't lucky enough to reproduce it, but it seems like reader on another CPU can misswaiter->taskupdate and schedule again, resulting
in indefinite (MAX_SCHEDULE_TIMEOUT) sleep.Make sure waked up reader will see
waiter->task== NULL.drivers/tty/tty_ldsem.c +1
-0@@ -118,6 +118,8 @@ static void __ldsem_wake_readers(struct ld_semaphore *sem) tsk = waiter->task;smp_mb (); waiter->task =NULL ; +/* Make sure down_read_failed() will see !waiter-
Quoted 7 lines
drivers/tty/tty_ldsem.c static void __ldsem_wake_readers(struct ld_semaphore *sem) · line 123
Quoted 3 lines
Quoted 1 line
task update */
drivers/tty/tty_ldsem.c +1
-0+smp_wmb ();wake_up_process (tsk);This is 'wrong',
wake_up_process()should imply sufficient for this to
already be true.
Yeah, thanks.
It was stupid of me not to check that..
Saw the smoke that would describe the reports and made too long-going conjectures. Need more covfefe and staring into that code.
Quoted 3 lines
Quoted 3 lines
drivers/tty/tty_ldsem.c +0
-0put_task_struct (tsk); }
Signature · 2 lines
Thanks,
DmitryPATCH v3 3/6 tty: Hold tty_ldisc_lock() during tty_reopen()
In reply to Dmitry Safonovtty_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: [email protected]
--- drivers/tty/tty_io.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/tty/tty_io.c +6 -3
@@ -1255,7 +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; +int retval =0 ;if (driver->type ==TTY_DRIVER_TYPE_PTY && driver->subtype ==PTY_TYPE_MASTER )
@@ -1267,15 +1267,18 @@ static int tty_reopen(struct tty_struct *tty)if (test_bit (TTY_EXCLUSIVE , &tty->flags) && !capable (CAP_SYS_ADMIN ))return -EBUSY ; - tty->count++; +tty_ldisc_lock (tty,MAX_SCHEDULE_TIMEOUT ); + 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.13.6
PATCH v3 4/6 tty/lockdep: Add ldisc_sem asserts
In reply to Dmitry SafonovMake 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.
Exception here is tty_ldisc_open(), as it's called without ldisc_sem locked by tty_init_dev() for the tty->link.
--- drivers/tty/tty_ldisc.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/tty/tty_ldisc.c +5 -0
@@ -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 (&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 (&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 (&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 (&tty->ldisc_sem); ld =tty_ldisc_get (tty, disc);if (IS_ERR (ld)) {BUG_ON (disc ==N_TTY );
@@ -825,6 +829,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.13.6
Re: [PATCHv3 4/6] tty/lockdep: Add ldisc_sem asserts
Subject: tty/ldsem: Convert to regular lockdep annotations
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
@@ -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
@@ -310,17 +287,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 ; }
@@ -329,17 +306,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 ; }
@@ -362,8 +339,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 ; } }
@@ -388,8 +365,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 ; } }
@@ -403,7 +380,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 )
@@ -417,7 +394,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 )
Re: [PATCHv3 4/6] tty/lockdep: Add ldisc_sem asserts
In reply to Dmitry SafonovQuoted 2 lines · Dmitry Safonov
Make sure under
CONFIG_LOCKDEPthat each change to line discipline is done with held write semaphor.
But you don't do that. You just assert it is held, not for writing.
Quoted 5 lines
Otherwise potential reader will have a good time dereferencing incomplete/uninitialized ldisc.
Exception here is
tty_ldisc_open(), as it's called withoutldisc_semlocked bytty_init_dev()for thetty->link.
You fail to explain how that is not broken...
Quoted 12 lines
Cc Greg Kroah-Hartman <[email protected]>Cc Jiri Slaby <[email protected]>Signed-off-by Dmitry Safonov <[email protected]>--- drivers/tty/tty_ldisc.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/tty/tty_ldisc.c +0
-0
drivers/tty/tty_ldisc.c static int tty_ldisc_open(struct tty_struct *tty, struct tty_ldisc *ld) · line 474
Quoted 4 lines
drivers/tty/tty_ldisc.c +1
-0@@ -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 (&tty->ldisc_sem);
Did you want:
lockdep_assert_held_exclusive(&tty->ldisc_sem);
?
Quoted 3 lines
drivers/tty/tty_ldisc.c +0
-0WARN_ON (!test_bit (TTY_LDISC_OPEN , &tty->flags));clear_bit (TTY_LDISC_OPEN , &tty->flags);if (ld->ops->close)
Re: [PATCHv3 4/6] tty/lockdep: Add ldisc_sem asserts
Quoted 6 lines · Peter Zijlstra
Quoted 2 lines · Dmitry Safonov
Make sure under
CONFIG_LOCKDEPthat each change to line discipline is done with held write semaphor.But you don't do that. You just assert it is held, not for writing.
Quoted 8 lines
Quoted 6 lines
Otherwise potential reader will have a good time dereferencing incomplete/uninitialized ldisc.
Exception here is
tty_ldisc_open(), as it's called withoutldisc_sem
locked bytty_init_dev()for thetty->link.You fail to explain how that is not broken...
I'll add the explanation to the next version.
Quoted 22 lines
Quoted 13 lines
Cc Greg Kroah-Hartman <[email protected]>Cc Jiri Slaby <[email protected]>Signed-off-by Dmitry Safonov <[email protected]>--- drivers/tty/tty_ldisc.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/tty/tty_ldisc.c +0
-0drivers/tty/tty_ldisc.c static int tty_ldisc_open(struct tty_struct · line 476
Quoted 6 lines
drivers/tty/tty_ldisc.c +1
-0@@ -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 (&tty->ldisc_sem);Did you want:
Quoted 3 lines
lockdep_assert_held_exclusive(&tty->ldisc_sem);?
Oh, yes, thanks.
Quoted 4 lines
Quoted 4 lines
drivers/tty/tty_ldisc.c +0
-0WARN_ON (!test_bit (TTY_LDISC_OPEN , &tty->flags));drivers/tty/tty_ldisc.c +0
-0clear_bit (TTY_LDISC_OPEN , &tty->flags);if (ld->ops->close)
Signature · 2 lines
Thanks,
DmitryPATCH v3 5/6 tty: Simplify tty->count math in tty_reopen()
In reply to Dmitry SafonovAs 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 | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
drivers/tty/tty_io.c +5 -9
@@ -1268,17 +1268,13 @@ static int tty_reopen(struct tty_struct *tty)return -EBUSY ;tty_ldisc_lock (tty,MAX_SCHEDULE_TIMEOUT ); +if (!tty->ldisc) + retval =tty_ldisc_reinit (tty, tty->termios.c_line); +tty_ldisc_unlock (tty); - tty->count++; -if (tty->ldisc) - goto out_unlock; +if (retval ==0 ) + tty->count++; - 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.13.6
PATCH v3 6/6 tty/ldsem: Decrement wait_readers on timeouted down_read()
In reply to Dmitry SafonovIt 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..
--- drivers/tty/tty_ldsem.c | 1 + 1 file changed, 1 insertion(+)
drivers/tty/tty_ldsem.c +1 -0
@@ -237,6 +237,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.13.6
Re: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
Quoted 6 lines · Dmitry Safonov
It seems like when
ldsem_down_read()fails with timeout, it misses update forsem->wait_readers. By that reason, when writer finally releases write end of the semaphore__ldsem_wake_readers()does adjustsem->countwith wrong value:sem->wait_readers* (LDSEM_ACTIVE_BIAS-LDSEM_WAIT_BIAS)
Quoted 7 lines
I.e, if update comes with 1 missed
wait_readersdecrement,sem->countwill be0x100000001which 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 thanMAX_SCHEDULE_TIMEOUT, so it might be worth to delete timeout parameter and error path fall-back..
You might want to think about ditching that ldsem thing entirely, and use a regular rwsem ?
Re: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
Quoted 14 lines · Peter Zijlstra
Quoted 13 lines · Dmitry Safonov
It seems like when
ldsem_down_read()fails with timeout, it misses update forsem->wait_readers. By that reason, when writer finally releases write end of the semaphore__ldsem_wake_readers()does adjustsem->countwith wrong value:sem->wait_readers* (LDSEM_ACTIVE_BIAS-LDSEM_WAIT_BIAS)I.e, if update comes with 1 missed
wait_readersdecrement, sem-Quoted 1 line
count
will be
0x100000001which means that there is active reader and it'll
make any further writer to fail in acquiring the semaphore.
Quoted 8 lines
Quoted 5 lines
It looks like, this is a dead-code, because
ldsem_down_read()is never
called with timeout different thanMAX_SCHEDULE_TIMEOUT, so it might be
worth to delete timeout parameter and error path fall-back..You might want to think about ditching that ldsem thing entirely, and use a regular rwsem ?
Yeah, but AFAICS, regular rwsem will need to have a timeout then (for write). So, I thought fixing this pile would be simpler than adding timeout and probably writer-priority to generic rwsem?
And I guess, we still will need fixes for stable for the bugs here..
I expect that timeouts are ABI, while the gain of adding priority may be measured. I'll give it a shot (adding timeout/priority for linux- next) to rwsem if you say it's acceptable.
Signature · 2 lines
Thanks,
DmitryRe: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
Quoted 31 lines · Dmitry Safonov
Quoted 22 lines · Peter Zijlstra
Quoted 15 lines · Dmitry Safonov
It seems like when
ldsem_down_read()fails with timeout, it misses
update forsem->wait_readers. By that reason, when writer finally releases write end of the semaphore__ldsem_wake_readers()does adjustsem->countwith wrong value:sem->wait_readers* (LDSEM_ACTIVE_BIAS-LDSEM_WAIT_BIAS)I.e, if update comes with 1 missed
wait_readersdecrement, sem-Quoted 1 line
count
will be
0x100000001which means that there is active reader and it'll
make any further writer to fail in acquiring the semaphore.Quoted 5 lines
It looks like, this is a dead-code, because
ldsem_down_read()is never
called with timeout different thanMAX_SCHEDULE_TIMEOUT, so it might be
worth to delete timeout parameter and error path fall-back..Quoted 3 lines
You might want to think about ditching that ldsem thing entirely, and
use a regular rwsem ?Yeah, but AFAICS, regular rwsem will need to have a timeout then (for write). So, I thought fixing this pile would be simpler than adding timeout and probably writer-priority to generic rwsem?
Quoted 5 lines
And I guess, we still will need fixes for stable for the bugs here..
I expect that timeouts are ABI, while the gain of adding priority may be measured. I'll give it a shot (adding timeout/priority for linux- next) to rwsem if you say it's acceptable.
Actually, priority looks quite simple: we can add writers in the head of wait_list and it probably may work.
Timeout looks also not a rocket science.
So, I can try to do that if you say it's acceptable (with the gain measures).
After this can of worms that I need to fix regardless.
Signature · 2 lines
Thanks,
DmitryRe: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
Quoted 13 lines · Dmitry Safonov
Quoted 7 lines
Quoted 2 lines
You might want to think about ditching that ldsem thing entirely, and use a regular rwsem ?
Yeah, but AFAICS, regular rwsem will need to have a timeout then (for write). So, I thought fixing this pile would be simpler than adding timeout and probably writer-priority to generic rwsem?
Quoted 5 lines
And I guess, we still will need fixes for stable for the bugs here..
I expect that timeouts are ABI, while the gain of adding priority may be measured. I'll give it a shot (adding timeout/priority for linux- next) to rwsem if you say it's acceptable.
Quoted 5 lines
Actually, priority looks quite simple: we can add writers in the head of
wait_listand it probably may work.
Timeout looks also not a rocket science.
So, I can try to do that if you say it's acceptable (with the gain measures).
So why do you need writer priority? The comment that goes with ldsems doesn't explain I think, it just says it has it.
In general I dislike unfair locks, they always cause trouble.
Quoted 1 line
After this can of worms that I need to fix regardless.
Sure.
Re: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
Quoted 27 lines · Peter Zijlstra
Quoted 19 lines · Dmitry Safonov
Quoted 10 lines
Quoted 3 lines
You might want to think about ditching that ldsem thing entirely,
and use a regular rwsem ?Yeah, but AFAICS, regular rwsem will need to have a timeout then (for
write). So, I thought fixing this pile would be simpler than adding
timeout and probably writer-priority to generic rwsem?Quoted 8 lines
And I guess, we still will need fixes for stable for the bugs here..
I expect that timeouts are ABI, while the gain of adding priority may
be measured. I'll give it a shot (adding timeout/priority for linux-
next) to rwsem if you say it's acceptable.Quoted 6 lines
Actually, priority looks quite simple: we can add writers in the head
ofwait_listand it probably may work.
Timeout looks also not a rocket science.
So, I can try to do that if you say it's acceptable (with the gain measures).
Quoted 2 lines
So why do you need writer priority? The comment that goes with ldsems doesn't explain I think, it just says it has it.
Well, as far as I can fetch from the commit 4898e640caf0, it describes that you should halt and scrap pending i/o (reader side) to prevent the loss or change of the current line dicipline (write lock). So, AFAIU, line discipline is expected to change within 5 sec by ABI and write-priority makes it more likely.
Quoted 5 lines
In general I dislike unfair locks, they always cause trouble.
Quoted 1 line
After this can of worms that I need to fix regardless.
Sure.
Signature · 2 lines
Thanks,
DmitryQuoted 1 line · Dmitry Safonov
Hi all,
Hi,
Quoted 11 lines
Three fixes that worth to have in the @stable, as we've hit them 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 optional and probably, timeout can be dropped for
read_lock(). I'll do it if everyone agrees.Rong Chen, could you kindly re-run this version to see if the lockup from v1 still happens? I wasn't able to reproduce it..
These patches seem to fix issues I've been seeing on arm64 for a while but hadn't managed to track down.
For patches 1, 3, and 5, feel free to add:
On vanilla v4.19-rc2, the below reproducer would fire in seconds, whereas with those patches applied, I have not seen issues after 10s of minutes of testing.
Thanks,
Mark.
Syzkaller hit 'KASAN: user-memory-access Write in n_tty_set_termios' bug.
IPv6: ADDRCONF(NETDEV_UP): veth0: link is not ready
ipV6: ADDRCONF(NETDEV_UP): veth1: link is not ready
IPv6: ADDRCONF(NETDEV_CHANGE): veth1: link becomes ready
IPv6: ADDRCONF(NETDEV_CHANGE): veth0: link becomes ready
==================================================================
BUG: KASAN: user-memory-access in memset include/linux/string.h:330 [inline] BUG: KASAN: user-memory-access in bitmap_zero include/linux/bitmap.h:216 [inline] BUG: KASAN: user-memory-access in n_tty_set_termios+0xe4/0xd08 drivers/tty/n_tty.c:1784 Write of size 512 at addr 0000000000001060 by task syz-executor0/3007 CPU: 1 PID: 3007 Comm: syz-executor0 Not tainted 4.19.0-rc2-dirty #4 Hardware name: linux,dummy-virt (DT) Call trace: dump_backtrace+0x0/0x340 arch/arm64/include/asm/ptrace.h:270 show_stack+0x20/0x30 arch/arm64/kernel/traps.c:152 __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0xec/0x150 lib/dump_stack.c:113 kasan_report_error mm/kasan/report.c:352 [inline] kasan_report+0x228/0x360 mm/kasan/report.c:412 check_memory_region_inline mm/kasan/kasan.c:253 [inline] check_memory_region+0x114/0x1c8 mm/kasan/kasan.c:267 memset+0x2c/0x50 mm/kasan/kasan.c:285 memset include/linux/string.h:330 [inline] bitmap_zero include/linux/bitmap.h:216 [inline] n_tty_set_termios+0xe4/0xd08 drivers/tty/n_tty.c:1784 tty_set_termios+0x538/0x760 drivers/tty/tty_ioctl.c:341 set_termios+0x348/0x968 drivers/tty/tty_ioctl.c:414 tty_mode_ioctl+0x8f0/0xc60 drivers/tty/tty_ioctl.c:779 n_tty_ioctl_helper+0x6c/0x390 drivers/tty/tty_ioctl.c:940 n_tty_ioctl+0x6c/0x490 drivers/tty/n_tty.c:2450 tty_ioctl+0x610/0x19a8 drivers/tty/tty_io.c:2655 vfs_ioctl fs/ioctl.c:46 [inline] file_ioctl fs/ioctl.c:501 [inline] do_vfs_ioctl+0x1bc/0x1618 fs/ioctl.c:685 ksys_ioctl+0xbc/0x108 fs/ioctl.c:702 __do_sys_ioctl fs/ioctl.c:709 [inline] __se_sys_ioctl fs/ioctl.c:707 [inline] __arm64_sys_ioctl+0x6c/0xa0 fs/ioctl.c:707 __invoke_syscall arch/arm64/kernel/syscall.c:36 [inline] invoke_syscall arch/arm64/kernel/syscall.c:48 [inline] el0_svc_common+0x150/0x288 arch/arm64/kernel/syscall.c:84 el0_svc_handler+0x54/0xf0 arch/arm64/kernel/syscall.c:130 el0_svc+0x8/0xc arch/arm64/kernel/entry.S:917 ==================================================================
Syzkaller reproducer:
# {Threaded:true Collide:true Repeat:true RepeatTimes:0 Procs:1 Sandbox:none Fault:false FaultCall:-1 FaultNth:0 EnableTun:true UseTmpDir:true EnableCgroups:true EnableNetdev:true ResetNet:true HandleSegv:true Repro:false Trace:false} r0 = openat$ptmx(0xffffffffffffff9c, &(0x7f0000000000)='/dev/ptmx\x00', 0x0, 0x0)
ioctl$TIOCGPTPEER(r0, 0x40045431, 0x6e0000)
r1 = syz_open_pts(r0, 0x0)
ioctl$TCXONC(r1, 0x5437, 0x0)
ioctl$TIOCGSOFTCAR(r0, 0x5419, &(0x7f00000000c0))
r2 = semget(0x0, 0x1, 0x1a)
semctl$IPC_INFO(r2, 0x0, 0x3, &(0x7f0000000100)=""/166)syz_open_pts(r0, 0x2)
ioctl$TCSETAW(r0, 0x5407, &(0x7f0000000080))
Quoted 20 lines · Mark Rutland
Quoted 1 line · Dmitry Safonov
Hi all,
Hi,
Quoted 8 lines
Three fixes that worth to have in the @stable, as we've hit them 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.Quoted 6 lines
The last patch is optional and probably, timeout can be dropped for
read_lock(). I'll do it if everyone agrees.Rong Chen, could you kindly re-run this version to see if the lockup
from v1 still happens? I wasn't able to reproduce it..
Quoted 7 lines
These patches seem to fix issues I've been seeing on arm64 for a while
but hadn't managed to track down.For patches 1, 3, and 5, feel free to add:
Tested-by Mark Rutland <[email protected]>
Thanks, Mark!
Will add on the next version.
Quoted 77 lines
On vanilla v4.19-rc2, the below reproducer would fire in seconds, whereas with those patches applied, I have not seen issues after 10s of
minutes of testing.Thanks,
Mark.Syzkaller hit 'KASAN: user-memory-access Write in
n_tty_set_termios' bug.IPv6:
ADDRCONF(NETDEV_UP): veth0: link is not ready
ipV6:ADDRCONF(NETDEV_UP): veth1: link is not ready
IPv6:ADDRCONF(NETDEV_CHANGE): veth1: link becomes ready
IPv6:ADDRCONF(NETDEV_CHANGE): veth0: link becomes ready
==================================================================BUG: KASAN: user-memory-access in memset include/linux/string.h:330 [inline] BUG: KASAN: user-memory-access in bitmap_zero include/linux/bitmap.h:216 [inline] BUG: KASAN: user-memory-access in n_tty_set_termios+0xe4/0xd08 drivers/tty/n_tty.c:1784 Write of size 512 at addr 0000000000001060 by task syz-executor0/3007 CPU: 1 PID: 3007 Comm: syz-executor0 Not tainted 4.19.0-rc2-dirty #4 Hardware name: linux,dummy-virt (DT) Call trace: dump_backtrace+0x0/0x340 arch/arm64/include/asm/ptrace.h:270 show_stack+0x20/0x30 arch/arm64/kernel/traps.c:152 __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0xec/0x150 lib/dump_stack.c:113 kasan_report_error mm/kasan/report.c:352 [inline] kasan_report+0x228/0x360 mm/kasan/report.c:412 check_memory_region_inline mm/kasan/kasan.c:253 [inline] check_memory_region+0x114/0x1c8 mm/kasan/kasan.c:267 memset+0x2c/0x50 mm/kasan/kasan.c:285 memset include/linux/string.h:330 [inline] bitmap_zero include/linux/bitmap.h:216 [inline] n_tty_set_termios+0xe4/0xd08 drivers/tty/n_tty.c:1784 tty_set_termios+0x538/0x760 drivers/tty/tty_ioctl.c:341 set_termios+0x348/0x968 drivers/tty/tty_ioctl.c:414 tty_mode_ioctl+0x8f0/0xc60 drivers/tty/tty_ioctl.c:779 n_tty_ioctl_helper+0x6c/0x390 drivers/tty/tty_ioctl.c:940 n_tty_ioctl+0x6c/0x490 drivers/tty/n_tty.c:2450 tty_ioctl+0x610/0x19a8 drivers/tty/tty_io.c:2655 vfs_ioctl fs/ioctl.c:46 [inline] file_ioctl fs/ioctl.c:501 [inline] do_vfs_ioctl+0x1bc/0x1618 fs/ioctl.c:685 ksys_ioctl+0xbc/0x108 fs/ioctl.c:702 __do_sys_ioctl fs/ioctl.c:709 [inline] __se_sys_ioctl fs/ioctl.c:707 [inline] __arm64_sys_ioctl+0x6c/0xa0 fs/ioctl.c:707 __invoke_syscall arch/arm64/kernel/syscall.c:36 [inline] invoke_syscall arch/arm64/kernel/syscall.c:48 [inline] el0_svc_common+0x150/0x288 arch/arm64/kernel/syscall.c:84 el0_svc_handler+0x54/0xf0 arch/arm64/kernel/syscall.c:130 el0_svc+0x8/0xc arch/arm64/kernel/entry.S:917 ==================================================================Syzkaller reproducer:
# {Threaded:true Collide:true Repeat:true RepeatTimes:0 Procs:1 Sandbox:none Fault:false FaultCall:-1 FaultNth:0 EnableTun:true UseTmpDir:true EnableCgroups:true EnableNetdev:true ResetNet:true HandleSegv:true Repro:false Trace:false}
r0 = openat$ptmx(0xffffffffffffff9c, &(0x7f0000000000)='/dev/ptmx\x00',0x0,0x0) ioctl$TIOCGPTPEER(r0,0x40045431,0x6e0000)
r1 =syz_open_pts(r0, 0x0)
ioctl$TCXONC(r1,0x5437,0x0)
ioctl$TIOCGSOFTCAR(r0,0x5419, &(0x7f00000000c0))
r2 =semget(0x0, 0x1, 0x1a)
semctl$IPC_INFO(r2,0x0,0x3, &(0x7f0000000100)=""/166)syz_open_pts(r0, 0x2)
ioctl$TCSETAW(r0,0x5407, &(0x7f0000000080))
Signature · 2 lines
Thanks,
Dmitry