[PATCHv3 0/6] tty: Hold write ldisc sem in tty_reopen()

[PATCHv3 0/6] tty: Hold write ldisc sem in tty_reopen()

[Dmitry Safonov]

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/<20180829022353.23568-1-dima@arista.com>

Huuge cc list:
Cc: Daniel Axtens <dja@axtens.net>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Michael Neuling <mikey@neuling.org>
Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Nathan March <nathan@gt.net>
Cc: Pasi Kärkkäinen <pasik@iki.fi>
Cc: Peter Hurley <peter@hurleysoftware.com>
Cc: "Rong, Chen" <rong.a.chen@intel.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Tan Xiaojun <tanxiaojun@huawei.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
(please, ignore if I Cc'ed you mistakenly)

Dmitry Safonov (6):
  tty: Drop tty->count on tty_reopen() failure
  tty/ldsem: 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(-)

-- 
2.13.6

[PATCHv3 1/6] tty: Drop tty->count on tty_reopen() failure

[Dmitry Safonov]

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

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

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

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

[PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader

[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/<20171121132855.ajdv4k6swzhvktl6@wfg-t540p.sh.intel.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 miss waiter->task update and schedule again, resulting
in indefinite (MAX_SCHEDULE_TIMEOUT) sleep.

Make sure waked up reader will see waiter->task == NULL.

Cc: stable@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Reported-by: kernel test robot <rong.a.chen@intel.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/tty_ldsem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_ldsem.c b/drivers/tty/tty_ldsem.c
index 0c98d88f795a..832accbbcb6d 100644
--- a/drivers/tty/tty_ldsem.c
+++ b/drivers/tty/tty_ldsem.c
@@ -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;
-- 
2.13.6

[PATCHv3 3/6] tty: Hold tty_ldisc_lock() during tty_reopen()

[Dmitry Safonov]

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

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

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

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

Backport-first: b027e2298bd5 ("tty: fix data race between tty_init_dev
and flush of buf")
Cc: stable@vger.kernel.org

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

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 5e5da9acaf0a..a947719b4626 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -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;
 }
 
-- 
2.13.6

Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader

[Sergey Senozhatsky]

On (09/11/18 02:48), Dmitry Safonov wrote:
> There is a couple of reports about lockup in ldsem_down_read() without
> anyone holding write end of ldisc semaphore:
> lkml.kernel.org/r/<20171121132855.ajdv4k6swzhvktl6@wfg-t540p.sh.intel.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 miss waiter->task update and schedule again, resulting
> in indefinite (MAX_SCHEDULE_TIMEOUT) sleep.

Certainly, something suspicious is going on.

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

>  		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);

I think that set_current_state() also executes memory barrier. Just
because it accesses task state.

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

[Sergey Senozhatsky]

On (09/11/18 14:04), Sergey Senozhatsky wrote:
> >  	for (;;) {
> >  		set_current_state(TASK_UNINTERRUPTIBLE);
> 
> I think that set_current_state() also executes memory barrier. Just
> because it accesses task state.
> 
> > -		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)) {
		...
	}

	-ss

Re: Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader

[Kirill Tkhai]

On 9/11/18 8:41 AM, Sergey Senozhatsky wrote:
> On (09/11/18 14:04), Sergey Senozhatsky wrote:
>>>  	for (;;) {
>>>  		set_current_state(TASK_UNINTERRUPTIBLE);
>>
>> I think that set_current_state() also executes memory barrier. Just
>> because it accesses task state.
>>
>>> -		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)) {
> 		...
> 	}

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

[Peter Zijlstra]

On Tue, Sep 11, 2018 at 02:48:17AM +0100, Dmitry Safonov wrote:
> There is a couple of reports about lockup in ldsem_down_read() without
> anyone holding write end of ldisc semaphore:
> lkml.kernel.org/r/<20171121132855.ajdv4k6swzhvktl6@wfg-t540p.sh.intel.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 miss waiter->task update and schedule again, resulting
> in indefinite (MAX_SCHEDULE_TIMEOUT) sleep.
> 
> Make sure waked up reader will see waiter->task == NULL.

> --- a/drivers/tty/tty_ldsem.c
> +++ b/drivers/tty/tty_ldsem.c
> @@ -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. 

>  		put_task_struct(tsk);
>  	}

Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader

[Peter Zijlstra]

On Tue, Sep 11, 2018 at 02:04:49PM +0900, Sergey Senozhatsky wrote:
> On (09/11/18 02:48), Dmitry Safonov wrote:
> > There is a couple of reports about lockup in ldsem_down_read() without
> > anyone holding write end of ldisc semaphore:
> > lkml.kernel.org/r/<20171121132855.ajdv4k6swzhvktl6@wfg-t540p.sh.intel.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 miss waiter->task update and schedule again, resulting
> > in indefinite (MAX_SCHEDULE_TIMEOUT) sleep.
> 
> Certainly, something suspicious is going on.
> 
> > @@ -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.
> 
> >  		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);
> 
> 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

[Peter Zijlstra]

On Tue, Sep 11, 2018 at 02:41:29PM +0900, Sergey Senozhatsky wrote:
> On (09/11/18 14:04), Sergey Senozhatsky wrote:
> > >  	for (;;) {
> > >  		set_current_state(TASK_UNINTERRUPTIBLE);
> > 
> > I think that set_current_state() also executes memory barrier. Just
> > because it accesses task state.
> > 
> > > -		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)) {
> 		...
> 	}

No, it must be after set_current_state(). See that same comment.

Re: [PATCHv3 0/6] tty: Hold write ldisc sem in tty_reopen()

[Mark Rutland]

On Tue, Sep 11, 2018 at 02:48:15AM +0100, Dmitry Safonov wrote:
> Hi all,

Hi,

> 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:

Tested-by: Mark Rutland <mark.rutland@arm.com>

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

Re: [PATCHv3 0/6] tty: Hold write ldisc sem in tty_reopen()

[Dmitry Safonov]

On Tue, 2018-09-11 at 13:16 +0100, Mark Rutland wrote:
> On Tue, Sep 11, 2018 at 02:48:15AM +0100, Dmitry Safonov wrote:
> > Hi all,
> 
> Hi,
> 
> > 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:
> 
> Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks, Mark!
Will add on the next version.

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

-- 
Thanks,
             Dmitry

Re: [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader

[Dmitry Safonov]

On Tue, 2018-09-11 at 13:40 +0200, Peter Zijlstra wrote:
> On Tue, Sep 11, 2018 at 02:48:17AM +0100, Dmitry Safonov wrote:
> > There is a couple of reports about lockup in ldsem_down_read()
> > without
> > anyone holding write end of ldisc semaphore:
> > lkml.kernel.org/r/<20171121132855.ajdv4k6swzhvktl6@wfg-t540p.sh.int
> > 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 miss waiter->task update and schedule again,
> > resulting
> > in indefinite (MAX_SCHEDULE_TIMEOUT) sleep.
> > 
> > Make sure waked up reader will see waiter->task == NULL.
> > --- a/drivers/tty/tty_ldsem.c
> > +++ b/drivers/tty/tty_ldsem.c
> > @@ -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. 

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.

> 
> >  		put_task_struct(tsk);
> >  	}

-- 
Thanks,
             Dmitry