linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag
@ 2018-04-25 13:20 DaeRyong Jeong
  2018-04-25 13:41 ` Greg KH
  2018-04-25 14:39 ` Alan Cox
  0 siblings, 2 replies; 5+ messages in thread
From: DaeRyong Jeong @ 2018-04-25 13:20 UTC (permalink / raw)
  To: gregkh, jslaby; +Cc: byoungyoung, kt0755, bammanag, linux-kernel

tty_insert_flip_string_fixed_flag() copies chars to the buffer indicated
by th->used and updates tb->used.
But tty_insert_flip_string_fixed_flag() can be executed concurrently and
tb->used can be updated improperly.
It leads slab-out-of-bound write in tty_insert_flip_string_fixed_flag or
slab-out-of-bounds read in flush_to_ldisc

BUG: KASAN: slab-out-of-bounds in tty_insert_flip_string_fixed_flag+0xb5/
0x130 drivers/tty/tty_buffer.c:316 at addr ffff880114fcc121
Write of size 1792 by task syz-executor0/30017
CPU: 1 PID: 30017 Comm: syz-executor0 Not tainted 4.8.0 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
 0000000000000000 ffff88011638f888 ffffffff81694cc3 ffff88007d802140
 ffff880114fcb300 ffff880114fcc300 ffff880114fcb300 ffff88011638f8b0
 ffffffff8130075c ffff88011638f940 ffff88007d802140 ffff880194fcc121
Call Trace:
 __dump_stack lib/dump_stack.c:15 [inline]
 dump_stack+0xb3/0x110 lib/dump_stack.c:51
 kasan_object_err+0x1c/0x70 mm/kasan/report.c:156
 print_address_description mm/kasan/report.c:194 [inline]
 kasan_report_error+0x1f7/0x4e0 mm/kasan/report.c:283
 kasan_report+0x36/0x40 mm/kasan/report.c:303
 check_memory_region_inline mm/kasan/kasan.c:292 [inline]
 check_memory_region+0x13e/0x1a0 mm/kasan/kasan.c:299
 memcpy+0x37/0x50 mm/kasan/kasan.c:335
 tty_insert_flip_string_fixed_flag+0xb5/0x130 drivers/tty/tty_buffer.c:316
 tty_insert_flip_string include/linux/tty_flip.h:35 [inline]
 pty_write+0x7f/0xc0 drivers/tty/pty.c:115
 n_hdlc_send_frames+0x1d4/0x3b0 drivers/tty/n_hdlc.c:419
 n_hdlc_tty_wakeup+0x73/0xa0 drivers/tty/n_hdlc.c:496
 tty_wakeup+0x92/0xb0 drivers/tty/tty_io.c:601
 __start_tty.part.26+0x66/0x70 drivers/tty/tty_io.c:1018
 __start_tty+0x34/0x40 drivers/tty/tty_io.c:1013
 n_tty_ioctl_helper+0x146/0x1e0 drivers/tty/tty_ioctl.c:1138
 n_hdlc_tty_ioctl+0xb3/0x2b0 drivers/tty/n_hdlc.c:794
 tty_ioctl+0xa85/0x16d0 drivers/tty/tty_io.c:2992
 vfs_ioctl fs/ioctl.c:43 [inline]
 do_vfs_ioctl+0x13e/0xba0 fs/ioctl.c:679
 SYSC_ioctl fs/ioctl.c:694 [inline]
 SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685
 entry_SYSCALL_64_fastpath+0x1f/0xbd

Call sequences are as follows.
CPU0                                    CPU1
n_tty_ioctl_helper                      n_tty_ioctl_helper
__start_tty                             tty_send_xchar
tty_wakeup                              pty_write
n_hdlc_tty_wakeup                       tty_insert_flip_string
n_hdlc_send_frames                      tty_insert_flip_string_fixed_flag
pty_write
tty_insert_flip_string
tty_insert_flip_string_fixed_flag

Acquire tty->atomic_write_lock by calling tty_write_lock() before
__start_tty() since __start_tty() can sends frames.

Signed-off-by: DaeRyong Jeong <threeearcat@gmail.com>
---
 drivers/tty/tty_io.c    | 16 +++++++++-------
 drivers/tty/tty_ioctl.c |  5 +++++
 include/linux/tty.h     |  2 ++
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 63114ea35ec1..41f83bd4cc40 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -873,13 +873,15 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
 	return i;
 }

-static void tty_write_unlock(struct tty_struct *tty)
+void tty_write_unlock(struct tty_struct *tty, int wakeup)
 {
 	mutex_unlock(&tty->atomic_write_lock);
-	wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
+	if (wakeup) {
+		wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
+	}
 }

-static int tty_write_lock(struct tty_struct *tty, int ndelay)
+int tty_write_lock(struct tty_struct *tty, int ndelay)
 {
 	if (!mutex_trylock(&tty->atomic_write_lock)) {
 		if (ndelay)
@@ -973,7 +975,7 @@ static inline ssize_t do_tty_write(
 		ret = written;
 	}
 out:
-	tty_write_unlock(tty);
+	tty_write_unlock(tty, 1);
 	return ret;
 }

@@ -997,7 +999,7 @@ void tty_write_message(struct tty_struct *tty, char *msg)
 		if (tty->ops->write && tty->count > 0)
 			tty->ops->write(tty, msg, strlen(msg));
 		tty_unlock(tty);
-		tty_write_unlock(tty);
+		tty_write_unlock(tty, 1);
 	}
 	return;
 }
@@ -1092,7 +1094,7 @@ int tty_send_xchar(struct tty_struct *tty, char ch)
 	if (was_stopped)
 		stop_tty(tty);
 	up_read(&tty->termios_rwsem);
-	tty_write_unlock(tty);
+	tty_write_unlock(tty, 1);
 	return 0;
 }

@@ -2395,7 +2397,7 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 			msleep_interruptible(duration);
 		retval = tty->ops->break_ctl(tty, 0);
 out:
-		tty_write_unlock(tty);
+		tty_write_unlock(tty, 1);
 		if (signal_pending(current))
 			retval = -EINTR;
 	}
diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
index d9b561d89432..a54ab91aec90 100644
--- a/drivers/tty/tty_ioctl.c
+++ b/drivers/tty/tty_ioctl.c
@@ -911,12 +911,17 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
 			spin_unlock_irq(&tty->flow_lock);
 			break;
 		case TCOON:
+			if (tty_write_lock(tty, 0) < 0)
+				return -ERESTARTSYS;
+
 			spin_lock_irq(&tty->flow_lock);
 			if (tty->flow_stopped) {
 				tty->flow_stopped = 0;
 				__start_tty(tty);
 			}
 			spin_unlock_irq(&tty->flow_lock);
+
+			tty_write_unlock(tty, 0);
 			break;
 		case TCIOFF:
 			if (STOP_CHAR(tty) != __DISABLED_CHAR)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 47f8af22f216..9b18393d2f95 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -458,6 +458,8 @@ static inline struct tty_struct *tty_kref_get(struct tty_struct *tty)
 	return tty;
 }

+void tty_write_unlock(struct tty_struct *tty, int wakeup);
+int tty_write_lock(struct tty_struct *tty, int ndelay);
 extern const char *tty_driver_name(const struct tty_struct *tty);
 extern void tty_wait_until_sent(struct tty_struct *tty, long timeout);
 extern int __tty_check_change(struct tty_struct *tty, int sig);
--
2.14.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag
  2018-04-25 13:20 [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag DaeRyong Jeong
@ 2018-04-25 13:41 ` Greg KH
  2018-04-25 17:42   ` DaeRyong Jeong
  2018-04-25 14:39 ` Alan Cox
  1 sibling, 1 reply; 5+ messages in thread
From: Greg KH @ 2018-04-25 13:41 UTC (permalink / raw)
  To: DaeRyong Jeong; +Cc: jslaby, byoungyoung, kt0755, bammanag, linux-kernel

On Wed, Apr 25, 2018 at 10:20:50PM +0900, DaeRyong Jeong wrote:
> tty_insert_flip_string_fixed_flag() copies chars to the buffer indicated
> by th->used and updates tb->used.
> But tty_insert_flip_string_fixed_flag() can be executed concurrently and
> tb->used can be updated improperly.
> It leads slab-out-of-bound write in tty_insert_flip_string_fixed_flag or
> slab-out-of-bounds read in flush_to_ldisc
> 
> BUG: KASAN: slab-out-of-bounds in tty_insert_flip_string_fixed_flag+0xb5/
> 0x130 drivers/tty/tty_buffer.c:316 at addr ffff880114fcc121
> Write of size 1792 by task syz-executor0/30017
> CPU: 1 PID: 30017 Comm: syz-executor0 Not tainted 4.8.0 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
>  0000000000000000 ffff88011638f888 ffffffff81694cc3 ffff88007d802140
>  ffff880114fcb300 ffff880114fcc300 ffff880114fcb300 ffff88011638f8b0
>  ffffffff8130075c ffff88011638f940 ffff88007d802140 ffff880194fcc121
> Call Trace:
>  __dump_stack lib/dump_stack.c:15 [inline]
>  dump_stack+0xb3/0x110 lib/dump_stack.c:51
>  kasan_object_err+0x1c/0x70 mm/kasan/report.c:156
>  print_address_description mm/kasan/report.c:194 [inline]
>  kasan_report_error+0x1f7/0x4e0 mm/kasan/report.c:283
>  kasan_report+0x36/0x40 mm/kasan/report.c:303
>  check_memory_region_inline mm/kasan/kasan.c:292 [inline]
>  check_memory_region+0x13e/0x1a0 mm/kasan/kasan.c:299
>  memcpy+0x37/0x50 mm/kasan/kasan.c:335
>  tty_insert_flip_string_fixed_flag+0xb5/0x130 drivers/tty/tty_buffer.c:316
>  tty_insert_flip_string include/linux/tty_flip.h:35 [inline]
>  pty_write+0x7f/0xc0 drivers/tty/pty.c:115
>  n_hdlc_send_frames+0x1d4/0x3b0 drivers/tty/n_hdlc.c:419
>  n_hdlc_tty_wakeup+0x73/0xa0 drivers/tty/n_hdlc.c:496
>  tty_wakeup+0x92/0xb0 drivers/tty/tty_io.c:601
>  __start_tty.part.26+0x66/0x70 drivers/tty/tty_io.c:1018
>  __start_tty+0x34/0x40 drivers/tty/tty_io.c:1013
>  n_tty_ioctl_helper+0x146/0x1e0 drivers/tty/tty_ioctl.c:1138
>  n_hdlc_tty_ioctl+0xb3/0x2b0 drivers/tty/n_hdlc.c:794
>  tty_ioctl+0xa85/0x16d0 drivers/tty/tty_io.c:2992
>  vfs_ioctl fs/ioctl.c:43 [inline]
>  do_vfs_ioctl+0x13e/0xba0 fs/ioctl.c:679
>  SYSC_ioctl fs/ioctl.c:694 [inline]
>  SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685
>  entry_SYSCALL_64_fastpath+0x1f/0xbd
> 
> Call sequences are as follows.
> CPU0                                    CPU1
> n_tty_ioctl_helper                      n_tty_ioctl_helper
> __start_tty                             tty_send_xchar
> tty_wakeup                              pty_write
> n_hdlc_tty_wakeup                       tty_insert_flip_string
> n_hdlc_send_frames                      tty_insert_flip_string_fixed_flag
> pty_write
> tty_insert_flip_string
> tty_insert_flip_string_fixed_flag
> 
> Acquire tty->atomic_write_lock by calling tty_write_lock() before
> __start_tty() since __start_tty() can sends frames.
> 
> Signed-off-by: DaeRyong Jeong <threeearcat@gmail.com>
> ---
>  drivers/tty/tty_io.c    | 16 +++++++++-------
>  drivers/tty/tty_ioctl.c |  5 +++++
>  include/linux/tty.h     |  2 ++
>  3 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 63114ea35ec1..41f83bd4cc40 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -873,13 +873,15 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
>  	return i;
>  }
> 
> -static void tty_write_unlock(struct tty_struct *tty)
> +void tty_write_unlock(struct tty_struct *tty, int wakeup)
>  {
>  	mutex_unlock(&tty->atomic_write_lock);
> -	wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> +	if (wakeup) {
> +		wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> +	}

Always run scripts/checkpatch.pl before sending patches out so you do
not get grumpy maintainers telling you to run scripts/checkpatch.pl :)

Anyway, these "bool" type options are horrid for trying to remember what
is happening here.  You have to go look up what 1 or 0 is, right?

How about two functions:
	tty_write_unlock()
	tty_write_unlock_wakup()

and the second one calls the first and then does the wakeup?

But are you sure this is the correct fix?  What is protecting the race
from happening with this change?  How do you know to call wakeup or not?
What determines which is better to do?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag
  2018-04-25 13:20 [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag DaeRyong Jeong
  2018-04-25 13:41 ` Greg KH
@ 2018-04-25 14:39 ` Alan Cox
  2018-04-25 17:54   ` DaeRyong Jeong
  1 sibling, 1 reply; 5+ messages in thread
From: Alan Cox @ 2018-04-25 14:39 UTC (permalink / raw)
  To: DaeRyong Jeong
  Cc: gregkh, jslaby, byoungyoung, kt0755, bammanag, linux-kernel

On Wed, 25 Apr 2018 22:20:50 +0900
DaeRyong Jeong <threeearcat@gmail.com> wrote:

> tty_insert_flip_string_fixed_flag() copies chars to the buffer indicated
> by th->used and updates tb->used.
> But tty_insert_flip_string_fixed_flag() can be executed concurrently and
> tb->used can be updated improperly.

The tty input layer does not work if it can be executed concurrently. If
that is happening in the pty code then the pty code is buggy and that
needs serializing on the inbound path.


> -static void tty_write_unlock(struct tty_struct *tty)
> +void tty_write_unlock(struct tty_struct *tty, int wakeup)
>  {
>  	mutex_unlock(&tty->atomic_write_lock);
> -	wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> +	if (wakeup) {
> +		wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> +	}

And this may cause deadlocks.

You don't actually need any of the wakeup changes in your code

> diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> index d9b561d89432..a54ab91aec90 100644
> --- a/drivers/tty/tty_ioctl.c
> +++ b/drivers/tty/tty_ioctl.c
> @@ -911,12 +911,17 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
>  			spin_unlock_irq(&tty->flow_lock);
>  			break;
>  		case TCOON:
> +			if (tty_write_lock(tty, 0) < 0)
> +				return -ERESTARTSYS;
> +
>  			spin_lock_irq(&tty->flow_lock);
>  			if (tty->flow_stopped) {
>  				tty->flow_stopped = 0;
>  				__start_tty(tty);
>  			}
>  			spin_unlock_irq(&tty->flow_lock);
> +
> +			tty_write_unlock(tty, 0);

If you just used these unmodified it would be simpler and as good,
however it won't actually fix anything. The pty layer is broken not this
code.

The tty layer rule for all the input buffer handling is that you may not
call *any* of it from multiple threads at once. This works fine for
normal serial because the IRQ layer or the polling logic has that
property.

The bug looks real, your diagnosis looks right, your fix sort of works
but isn't sufficient.

So NAK.

Alan

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag
  2018-04-25 13:41 ` Greg KH
@ 2018-04-25 17:42   ` DaeRyong Jeong
  0 siblings, 0 replies; 5+ messages in thread
From: DaeRyong Jeong @ 2018-04-25 17:42 UTC (permalink / raw)
  To: Greg KH; +Cc: jslaby, byoungyoung, kt0755, bammanag, linux-kernel

On Wed, Apr 25, 2018 at 03:41:59PM +0200, Greg KH wrote:
> On Wed, Apr 25, 2018 at 10:20:50PM +0900, DaeRyong Jeong wrote:
> > tty_insert_flip_string_fixed_flag() copies chars to the buffer indicated
> > by th->used and updates tb->used.
> > But tty_insert_flip_string_fixed_flag() can be executed concurrently and
> > tb->used can be updated improperly.
> > It leads slab-out-of-bound write in tty_insert_flip_string_fixed_flag or
> > slab-out-of-bounds read in flush_to_ldisc
> > 
> > BUG: KASAN: slab-out-of-bounds in tty_insert_flip_string_fixed_flag+0xb5/
> > 0x130 drivers/tty/tty_buffer.c:316 at addr ffff880114fcc121
> > Write of size 1792 by task syz-executor0/30017
> > CPU: 1 PID: 30017 Comm: syz-executor0 Not tainted 4.8.0 #1
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
> > BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
> >  0000000000000000 ffff88011638f888 ffffffff81694cc3 ffff88007d802140
> >  ffff880114fcb300 ffff880114fcc300 ffff880114fcb300 ffff88011638f8b0
> >  ffffffff8130075c ffff88011638f940 ffff88007d802140 ffff880194fcc121
> > Call Trace:
> >  __dump_stack lib/dump_stack.c:15 [inline]
> >  dump_stack+0xb3/0x110 lib/dump_stack.c:51
> >  kasan_object_err+0x1c/0x70 mm/kasan/report.c:156
> >  print_address_description mm/kasan/report.c:194 [inline]
> >  kasan_report_error+0x1f7/0x4e0 mm/kasan/report.c:283
> >  kasan_report+0x36/0x40 mm/kasan/report.c:303
> >  check_memory_region_inline mm/kasan/kasan.c:292 [inline]
> >  check_memory_region+0x13e/0x1a0 mm/kasan/kasan.c:299
> >  memcpy+0x37/0x50 mm/kasan/kasan.c:335
> >  tty_insert_flip_string_fixed_flag+0xb5/0x130 drivers/tty/tty_buffer.c:316
> >  tty_insert_flip_string include/linux/tty_flip.h:35 [inline]
> >  pty_write+0x7f/0xc0 drivers/tty/pty.c:115
> >  n_hdlc_send_frames+0x1d4/0x3b0 drivers/tty/n_hdlc.c:419
> >  n_hdlc_tty_wakeup+0x73/0xa0 drivers/tty/n_hdlc.c:496
> >  tty_wakeup+0x92/0xb0 drivers/tty/tty_io.c:601
> >  __start_tty.part.26+0x66/0x70 drivers/tty/tty_io.c:1018
> >  __start_tty+0x34/0x40 drivers/tty/tty_io.c:1013
> >  n_tty_ioctl_helper+0x146/0x1e0 drivers/tty/tty_ioctl.c:1138
> >  n_hdlc_tty_ioctl+0xb3/0x2b0 drivers/tty/n_hdlc.c:794
> >  tty_ioctl+0xa85/0x16d0 drivers/tty/tty_io.c:2992
> >  vfs_ioctl fs/ioctl.c:43 [inline]
> >  do_vfs_ioctl+0x13e/0xba0 fs/ioctl.c:679
> >  SYSC_ioctl fs/ioctl.c:694 [inline]
> >  SyS_ioctl+0x8f/0xc0 fs/ioctl.c:685
> >  entry_SYSCALL_64_fastpath+0x1f/0xbd
> > 
> > Call sequences are as follows.
> > CPU0                                    CPU1
> > n_tty_ioctl_helper                      n_tty_ioctl_helper
> > __start_tty                             tty_send_xchar
> > tty_wakeup                              pty_write
> > n_hdlc_tty_wakeup                       tty_insert_flip_string
> > n_hdlc_send_frames                      tty_insert_flip_string_fixed_flag
> > pty_write
> > tty_insert_flip_string
> > tty_insert_flip_string_fixed_flag
> > 
> > Acquire tty->atomic_write_lock by calling tty_write_lock() before
> > __start_tty() since __start_tty() can sends frames.
> > 
> > Signed-off-by: DaeRyong Jeong <threeearcat@gmail.com>
> > ---
> >  drivers/tty/tty_io.c    | 16 +++++++++-------
> >  drivers/tty/tty_ioctl.c |  5 +++++
> >  include/linux/tty.h     |  2 ++
> >  3 files changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index 63114ea35ec1..41f83bd4cc40 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -873,13 +873,15 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
> >  	return i;
> >  }
> > 
> > -static void tty_write_unlock(struct tty_struct *tty)
> > +void tty_write_unlock(struct tty_struct *tty, int wakeup)
> >  {
> >  	mutex_unlock(&tty->atomic_write_lock);
> > -	wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> > +	if (wakeup) {
> > +		wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> > +	}
> 
> Always run scripts/checkpatch.pl before sending patches out so you do
> not get grumpy maintainers telling you to run scripts/checkpatch.pl :)

Yes, I will. I will never corrupt a patch again. I'm sorry..

> 
> Anyway, these "bool" type options are horrid for trying to remember what
> is happening here.  You have to go look up what 1 or 0 is, right?
> 
> How about two functions:
> 	tty_write_unlock()
> 	tty_write_unlock_wakup()
> 
> and the second one calls the first and then does the wakeup?

Yes. I agree that this has more readability and is much better to write a
code.

> 
> But are you sure this is the correct fix?  What is protecting the race
> from happening with this change?  How do you know to call wakeup or not?
> What determines which is better to do?

As Alan cox commented, pty layer is broken, not this code. I thought
wrong way to fix the issue. I will look into tty/pty code and think
deeply. And then I will send a new patch.

Thank you a lot for your comment!

> 
> thanks,
> 
> greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag
  2018-04-25 14:39 ` Alan Cox
@ 2018-04-25 17:54   ` DaeRyong Jeong
  0 siblings, 0 replies; 5+ messages in thread
From: DaeRyong Jeong @ 2018-04-25 17:54 UTC (permalink / raw)
  To: Alan Cox; +Cc: gregkh, jslaby, byoungyoung, kt0755, bammanag, linux-kernel

On Wed, Apr 25, 2018 at 03:39:48PM +0100, Alan Cox wrote:
> On Wed, 25 Apr 2018 22:20:50 +0900
> DaeRyong Jeong <threeearcat@gmail.com> wrote:
> 
> > tty_insert_flip_string_fixed_flag() copies chars to the buffer indicated
> > by th->used and updates tb->used.
> > But tty_insert_flip_string_fixed_flag() can be executed concurrently and
> > tb->used can be updated improperly.
> 
> The tty input layer does not work if it can be executed concurrently. If
> that is happening in the pty code then the pty code is buggy and that
> needs serializing on the inbound path.
> 
> 
> > -static void tty_write_unlock(struct tty_struct *tty)
> > +void tty_write_unlock(struct tty_struct *tty, int wakeup)
> >  {
> >  	mutex_unlock(&tty->atomic_write_lock);
> > -	wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> > +	if (wakeup) {
> > +		wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> > +	}
> 
> And this may cause deadlocks.
> 
> You don't actually need any of the wakeup changes in your code
> 
> > diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> > index d9b561d89432..a54ab91aec90 100644
> > --- a/drivers/tty/tty_ioctl.c
> > +++ b/drivers/tty/tty_ioctl.c
> > @@ -911,12 +911,17 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
> >  			spin_unlock_irq(&tty->flow_lock);
> >  			break;
> >  		case TCOON:
> > +			if (tty_write_lock(tty, 0) < 0)
> > +				return -ERESTARTSYS;
> > +
> >  			spin_lock_irq(&tty->flow_lock);
> >  			if (tty->flow_stopped) {
> >  				tty->flow_stopped = 0;
> >  				__start_tty(tty);
> >  			}
> >  			spin_unlock_irq(&tty->flow_lock);
> > +
> > +			tty_write_unlock(tty, 0);
> 
> If you just used these unmodified it would be simpler and as good,
> however it won't actually fix anything. The pty layer is broken not this
> code.

Yes. I thought the wrong way to resolve the issue.

> 
> The tty layer rule for all the input buffer handling is that you may not
> call *any* of it from multiple threads at once. This works fine for
> normal serial because the IRQ layer or the polling logic has that
> property.
> 
> The bug looks real, your diagnosis looks right, your fix sort of works
> but isn't sufficient.
> 
> So NAK.
> 
> Alan

With your comments, now I know that this patch is incorrect.
Since the bug is real, I will send a new patch with considering all your
comments above.

Thank you a lot for all your comments.

DaeRyong Jeong

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-04-25 17:54 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-25 13:20 [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag DaeRyong Jeong
2018-04-25 13:41 ` Greg KH
2018-04-25 17:42   ` DaeRyong Jeong
2018-04-25 14:39 ` Alan Cox
2018-04-25 17:54   ` DaeRyong Jeong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).