[PATCH] tty: hold lock across tty buffer finding and buffer filling

[PATCH] tty: hold lock across tty buffer finding and buffer filling

[Tu, Xiaobing]

From: Xiaobing Tu <xiaobing.tu@intel.com>

tty_buffer_request_room is well protected, but while after it returns,
 it releases the port->lock. tty->buf.tail might be modified
by either irq handler or other threads. The patch adds more protection
by holding the lock across tty buffer finding and buffer filling.

Signed-off-by: Alek Du <alek.du@intel.com>
Signed-off-by: Xiaobing Tu <xiaobing.tu@intel.com>
---
 drivers/tty/tty_buffer.c |   85 +++++++++++++++++++++++++++++++++++-----------
 1 files changed, 65 insertions(+), 20 deletions(-)
 mode change 100644 => 100755 drivers/tty/tty_buffer.c

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
old mode 100644
new mode 100755
index 6c9b7cd..6810f2d
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -185,25 +185,19 @@ static struct tty_buffer *tty_buffer_find(struct tty_struct *tty, size_t size)
 	/* Should possibly check if this fails for the largest buffer we
 	   have queued and recycle that ? */
 }
-
 /**
- *	tty_buffer_request_room		-	grow tty buffer if needed
+ *	__tty_buffer_request_room		-	grow tty buffer if needed
  *	@tty: tty structure
  *	@size: size desired
  *
  *	Make at least size bytes of linear space available for the tty
  *	buffer. If we fail return the size we managed to find.
- *
- *	Locking: Takes tty->buf.lock
+ *      Locking: Caller must hold tty->buf.lock
  */
-int tty_buffer_request_room(struct tty_struct *tty, size_t size)
+static int __tty_buffer_request_room(struct tty_struct *tty, size_t size)
 {
 	struct tty_buffer *b, *n;
 	int left;
-	unsigned long flags;
-
-	spin_lock_irqsave(&tty->buf.lock, flags);
-
 	/* OPTIMISATION: We could keep a per tty "zero" sized buffer to
 	   remove this conditional if its worth it. This would be invisible
 	   to the callers */
@@ -225,9 +219,30 @@ int tty_buffer_request_room(struct tty_struct *tty, size_t size)
 			size = left;
 	}
 
-	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	return size;
 }
+
+
+/**
+ *	tty_buffer_request_room		-	grow tty buffer if needed
+ *	@tty: tty structure
+ *	@size: size desired
+ *
+ *	Make at least size bytes of linear space available for the tty
+ *	buffer. If we fail return the size we managed to find.
+ *
+ *	Locking: Takes tty->buf.lock
+ */
+int tty_buffer_request_room(struct tty_struct *tty, size_t size)
+{
+	unsigned long flags;
+	int length;
+	
+	spin_lock_irqsave(&tty->buf.lock, flags);
+	length = __tty_buffer_request_room(tty, size);
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
+	return length;
+}
 EXPORT_SYMBOL_GPL(tty_buffer_request_room);
 
 /**
@@ -249,14 +264,22 @@ int tty_insert_flip_string_fixed_flag(struct tty_struct *tty,
 	int copied = 0;
 	do {
 		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
-		int space = tty_buffer_request_room(tty, goal);
-		struct tty_buffer *tb = tty->buf.tail;
+		int space;
+		unsigned long flags;
+		struct tty_buffer *tb;
+		
+		spin_lock_irqsave(&tty->buf.lock, flags);
+		space = __tty_buffer_request_room(tty, goal);
+		tb = tty->buf.tail;
 		/* If there is no space then tb may be NULL */
-		if (unlikely(space == 0))
+		if (unlikely(space == 0)) {
+			spin_unlock_irqrestore(&tty->buf.lock, flags);
 			break;
+		}
 		memcpy(tb->char_buf_ptr + tb->used, chars, space);
 		memset(tb->flag_buf_ptr + tb->used, flag, space);
 		tb->used += space;
+		spin_unlock_irqrestore(&tty->buf.lock, flags);
 		copied += space;
 		chars += space;
 		/* There is a small chance that we need to split the data over
@@ -286,14 +309,22 @@ int tty_insert_flip_string_flags(struct tty_struct *tty,
 	int copied = 0;
 	do {
 		int goal = min_t(size_t, size - copied, TTY_BUFFER_PAGE);
-		int space = tty_buffer_request_room(tty, goal);
-		struct tty_buffer *tb = tty->buf.tail;
+		int space;
+		unsigned long __flags;
+		struct tty_buffer *tb;
+		
+		spin_lock_irqsave(&tty->buf.lock, __flags);
+		space = __tty_buffer_request_room(tty, goal);
+		tb = tty->buf.tail;
 		/* If there is no space then tb may be NULL */
-		if (unlikely(space == 0))
+		if (unlikely(space == 0)) {
+			spin_unlock_irqrestore(&tty->buf.lock, __flags);
 			break;
+		}
 		memcpy(tb->char_buf_ptr + tb->used, chars, space);
 		memcpy(tb->flag_buf_ptr + tb->used, flags, space);
 		tb->used += space;
+		spin_unlock_irqrestore(&tty->buf.lock, __flags);
 		copied += space;
 		chars += space;
 		flags += space;
@@ -344,13 +375,20 @@ EXPORT_SYMBOL(tty_schedule_flip);
 int tty_prepare_flip_string(struct tty_struct *tty, unsigned char **chars,
 								size_t size)
 {
-	int space = tty_buffer_request_room(tty, size);
+	int space;
+	unsigned long flags;
+	struct tty_buffer *tb;
+	
+	spin_lock_irqsave(&tty->buf.lock, flags);
+	space = __tty_buffer_request_room(tty, size);
+
+	tb = tty->buf.tail;
 	if (likely(space)) {
-		struct tty_buffer *tb = tty->buf.tail;
 		*chars = tb->char_buf_ptr + tb->used;
 		memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
 		tb->used += space;
 	}
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	return space;
 }
 EXPORT_SYMBOL_GPL(tty_prepare_flip_string);
@@ -374,13 +412,20 @@ EXPORT_SYMBOL_GPL(tty_prepare_flip_string);
 int tty_prepare_flip_string_flags(struct tty_struct *tty,
 			unsigned char **chars, char **flags, size_t size)
 {
-	int space = tty_buffer_request_room(tty, size);
+	int space;
+	unsigned long __flags;
+	struct tty_buffer *tb;
+	
+	spin_lock_irqsave(&tty->buf.lock, __flags);
+	space = __tty_buffer_request_room(tty, size);
+
+	tb = tty->buf.tail;
 	if (likely(space)) {
-		struct tty_buffer *tb = tty->buf.tail;
 		*chars = tb->char_buf_ptr + tb->used;
 		*flags = tb->flag_buf_ptr + tb->used;
 		tb->used += space;
 	}
+	spin_unlock_irqrestore(&tty->buf.lock, __flags);
 	return space;
 }
 EXPORT_SYMBOL_GPL(tty_prepare_flip_string_flags);
-- 
1.7.4.1

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Alan Cox]

On Thu, 15 Mar 2012 02:06:43 +0000
"Tu, Xiaobing" <xiaobing.tu@intel.com> wrote:

> From: Xiaobing Tu <xiaobing.tu@intel.com>
> 
> tty_buffer_request_room is well protected, but while after it returns,
>  it releases the port->lock. tty->buf.tail might be modified

This can only occur in a way that matters if you have multiple writers to
the tty buffer. 

It is a design requirement of the tty layer that your input is single
threaded paths and this is true for existing drivers. There is a good
reason it is true as well - ordering of bytes is defined for serial data,
any parallel writer breaks that ordering even if you have locks in
tty_insert_flip_*.

In practice that means that the tail pointer has a single incrementer and
the reader is the ldisc processing queue. 

So NAK pending a lot more explanation and an actual case where you can
show it is required.

Under what situations with what in tree driver do you see a problem ?


Alan

RE: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Du, Alek]

Alan,

I agree with you for the case of multiple writers to the tty buffer.

But there is another case that cause kernel hang: driver is writing data to tty, while app shutdown the port or flush through IOCTL. The buf.tail is set to NULL, here is the log:


<1>[ 4713.451954] BUG: unable to handle kernel NULL pointer dereference at
00000004
<1>[ 4713.452424] IP: [<c14ff59c>] tty_insert_flip_string_fixed_flag+0x4c/0x90
<4>[ 4713.452832] *pde = 00000000 
<0>[ 4713.453017] Oops: 0000 [#1] PREEMPT SMP 
<0>[ 4713.453280] last sysfs file:
/sys/devices/pci0000:00/0000:00:03.5/gpio/gpio172/value
<4>[ 4713.453709] Modules linked in: ipv6 atomisp lm3554 mt9m114 mt9e013
videobuf_vmalloc videobuf_dma_contig videobuf_core wl12xx_sdio wl12xx mac80211
cfg80211 compat btwilink st_drv
<4>[ 4713.454768] 
<4>[ 4713.454875] Pid: 0, comm: swapper Not tainted 2.6.35.3-84779-g68baa8f #1
/
<4>[ 4713.455258] EIP: 0060:[<c14ff59c>] EFLAGS: 00010002 CPU: 0
<4>[ 4713.455573] EIP is at tty_insert_flip_string_fixed_flag+0x4c/0x90
<4>[ 4713.455914] EAX: e01a6800 EBX: 00000000 ECX: 00000001 EDX: 00010002
<4>[ 4713.456264] ESI: 00000001 EDI: 00000000 EBP: c1a8be50 ESP: c1a8be38
<4>[ 4713.456614]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
<0>[ 4713.456921] Process swapper (pid: 0, ti=c1a8a000 task=c1aa02a0
task.ti=c1a8a000)
<0>[ 4713.457324] Stack:
<4>[ 4713.457447]  00000000 f5abf800 e01a6800 f6dec358 f6df877c e01a6800
c1a8be78 c1544b13
<4>[ 4713.457958] <0> 00000001 000e0100 f79a785c 00000000 00000001 f6dec358
000000dc 00000001
<4>[ 4713.458511] <0> c1a8be98 c1544d0c 00000002 00000082 f6de0000 f7880f40
c1a908fc 00000000
<0>[ 4713.459087] Call Trace:
<4>[ 4713.459255]  [<c1544b13>] ? hsu_dma_rx+0xa3/0x190
<4>[ 4713.459534]  [<c1544d0c>] ? dma_irq+0x10c/0x160
<4>[ 4713.459806]  [<c12981f4>] ? handle_IRQ_event+0x44/0x1a0
<4>[ 4713.460109]  [<c129a798>] ? handle_fasteoi_irq+0x68/0xd0
<4>[ 4713.460418]  [<c120569d>] ? handle_irq+0x1d/0x30
<4>[ 4713.460695]  [<c18744cf>] ? do_IRQ+0x4f/0xc0
<4>[ 4713.460953]  [<c120356e>] ? common_interrupt+0x2e/0x34
<4>[ 4713.461257]  [<c124007b>] ? pre_schedule_rt+0x1b/0x30
<4>[ 4713.461557]  [<c14ede14>] ? soc_s0ix_idle+0x144/0x290
<4>[ 4713.461859]  [<c16544a0>] ? cpuidle_idle_call+0x90/0x180
<4>[ 4713.462168]  [<c1201d61>] ? cpu_idle+0x51/0xb0
<4>[ 4713.462431]  [<c1853e6b>] ? rest_init+0xab/0xc0
<4>[ 4713.462702]  [<c1b07991>] ? start_kernel+0x342/0x348
<4>[ 4713.462992]  [<c1b07460>] ? unknown_bootoption+0x0/0x1a0
<4>[ 4713.463301]  [<c1b070de>] ? i386_start_kernel+0xde/0xe6
<0>[ 4713.463593] Code: e8 8b 55 08 b8 00 07 00 00 29 fa 81 fa 00 07 00 00 0f
47
d0 8b 45 f0 e8 33 fd ff ff 89 c6 8b 45 f0 85 f6 8b 98 d4 00 00 00 74 2a <8b> 43
04 89 f1 01 f7 8b 55 ec 03 43 0c e8 72 99 fb ff 8b 43 08 
<0>[ 4713.465460] EIP: [<c14ff59c>] tty_insert_flip_string_fixed_flag+0x4c/0x90
SS:ESP 0068:c1a8be3





-----Original Message-----
From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk] 
Sent: Thursday, March 15, 2012 6:08 PM
To: Tu, Xiaobing
Cc: linux-kernel@vger.kernel.org; gregkh@linuxfoundation.org; Zhang, Yanmin; Du, Alek; Zuo, Jiao
Subject: Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

On Thu, 15 Mar 2012 02:06:43 +0000
"Tu, Xiaobing" <xiaobing.tu@intel.com> wrote:

> From: Xiaobing Tu <xiaobing.tu@intel.com>
> 
> tty_buffer_request_room is well protected, but while after it returns,  
> it releases the port->lock. tty->buf.tail might be modified

This can only occur in a way that matters if you have multiple writers to the tty buffer. 

It is a design requirement of the tty layer that your input is single threaded paths and this is true for existing drivers. There is a good reason it is true as well - ordering of bytes is defined for serial data, any parallel writer breaks that ordering even if you have locks in tty_insert_flip_*.

In practice that means that the tail pointer has a single incrementer and the reader is the ldisc processing queue. 

So NAK pending a lot more explanation and an actual case where you can show it is required.

Under what situations with what in tree driver do you see a problem ?


Alan

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Alan Cox]

On Thu, 15 Mar 2012 10:14:07 +0000
"Du, Alek" <alek.du@intel.com> wrote:

> Alan,
> 
> I agree with you for the case of multiple writers to the tty buffer.
> 
> But there is another case that cause kernel hang: driver is writing data to tty, while app shutdown the port or flush through IOCTL. The buf.tail is set to NULL, here is the log:
> 
> 
> <1>[ 4713.451954] BUG: unable to handle kernel NULL pointer dereference at
> 00000004
> <1>[ 4713.452424] IP: [<c14ff59c>] tty_insert_flip_string_fixed_flag+0x4c/0x90


Ouch.. yes that makes sense.

Alan

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[gregkh]

On Thu, Mar 15, 2012 at 02:06:43AM +0000, Tu, Xiaobing wrote:
> From: Xiaobing Tu <xiaobing.tu@intel.com>
> 
> tty_buffer_request_room is well protected, but while after it returns,
>  it releases the port->lock. tty->buf.tail might be modified
> by either irq handler or other threads. The patch adds more protection
> by holding the lock across tty buffer finding and buffer filling.
> 
> Signed-off-by: Alek Du <alek.du@intel.com>
> Signed-off-by: Xiaobing Tu <xiaobing.tu@intel.com>
> ---
>  drivers/tty/tty_buffer.c |   85 +++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 65 insertions(+), 20 deletions(-)
>  mode change 100644 => 100755 drivers/tty/tty_buffer.c
> 
> diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
> old mode 100644
> new mode 100755

What?  Why would you change the mode of the file to be executable?

Also, your patch introduces a bunch of trailing whitespace, please run
it through scripts/checkpatch.pl and fix those issues before resending
it.

thanks,

greg k-h

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Jiri Slaby]

On 03/15/2012 11:14 AM, Du, Alek wrote:
> Alan,
> 
> I agree with you for the case of multiple writers to the tty buffer.
> 
> But there is another case that cause kernel hang: driver is writing data to tty, while app shutdown the port or flush through IOCTL. The buf.tail is set to NULL, here is the log:
> 
> 
> <1>[ 4713.451954] BUG: unable to handle kernel NULL pointer dereference at
> 00000004

The trace only suggests that you don't increment tty reference count in
irq properly. If you did that, you would not have tty buffer gone by
concurrent shutdown.

For the latter case (flush via ioctl), your patch won't help for
tty_prepare_string*, as we return pointer to the buffer which is freed
by the flush, right?

thanks,
-- 
js
suse labs

RE: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Du, Alek]

The tty structure is protect and not null, the tty->buf.tail is null. Many people think the tty reference count isn't protected and cause this bug, it is not true. :-)

For the flush case, it need acquire the spinlock to free the buffer and put buf.tail to NULL. So this patch will help:

Here is the example place, you can see the __tty_buffer_flush is inside the spinlock.

void tty_buffer_flush(struct tty_struct *tty)
{
        unsigned long flags;
        spin_lock_irqsave(&tty->buf.lock, flags);

        /* If the data is being pushed to the tty layer then we can't
           process it here. Instead set a flag and the flush_to_ldisc
           path will process the flush request before it exits */
        if (test_bit(TTY_FLUSHING, &tty->flags)) {
                set_bit(TTY_FLUSHPENDING, &tty->flags);
                spin_unlock_irqrestore(&tty->buf.lock, flags);
                wait_event(tty->read_wait,
                                test_bit(TTY_FLUSHPENDING, &tty->flags) == 0);
                return;
        } else
                __tty_buffer_flush(tty);
        spin_unlock_irqrestore(&tty->buf.lock, flags);
}



Thanks,
Alek

-----Original Message-----
From: Jiri Slaby [mailto:jirislaby@gmail.com] On Behalf Of Jiri Slaby
Sent: Friday, March 16, 2012 5:44 PM
To: Du, Alek
Cc: Alan Cox; Tu, Xiaobing; linux-kernel@vger.kernel.org; gregkh@linuxfoundation.org; Zhang, Yanmin; Zuo, Jiao; Jiri Slaby
Subject: Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

On 03/15/2012 11:14 AM, Du, Alek wrote:
> Alan,
> 
> I agree with you for the case of multiple writers to the tty buffer.
> 
> But there is another case that cause kernel hang: driver is writing data to tty, while app shutdown the port or flush through IOCTL. The buf.tail is set to NULL, here is the log:
> 
> 
> <1>[ 4713.451954] BUG: unable to handle kernel NULL pointer 
> dereference at
> 00000004

The trace only suggests that you don't increment tty reference count in irq properly. If you did that, you would not have tty buffer gone by concurrent shutdown.

For the latter case (flush via ioctl), your patch won't help for tty_prepare_string*, as we return pointer to the buffer which is freed by the flush, right?

thanks,
--
js
suse labs

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Jiri Slaby]

On 03/16/2012 10:49 AM, Du, Alek wrote:
> The tty structure is protect and not null, the tty->buf.tail is null. Many people think the tty reference count isn't protected and cause this bug, it is not true. :-)

You protect the pointer, not what is inside that structure. If you
increment a tty reference count, tty->buf.tail won't be NULL iff tty is
not NULL. IOW, you have to use tty_port_tty_set/get all around.

> For the flush case, it need acquire the spinlock to free the buffer and put buf.tail to NULL. So this patch will help:
> 
> Here is the example place, you can see the __tty_buffer_flush is inside the spinlock.

I'm not talking about flush. I'm talking about prepare. It returns a
pointer to a tty buffer which may be concurrently freed by flush.

     A                                   B
================================================
prepare(&chars);
                                        flush();
memcpy(chars, data_from_HW); <- chars is freed now
flip();

thanks,
-- 
js
suse labs

RE: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Du, Alek]

We prepared the buffer, and use it. And during this, we don't release the spinlock of buf.lock, how the flush could happen?

               A
spin_lock
prepare the buffer
user the buffer
spin unlock


              B
Spin_lock
Flush
Free the buffer
Put buf.tail = NULL
Spin unlock



Thanks,
Alek
-----Original Message-----
From: Jiri Slaby [mailto:jirislaby@gmail.com] On Behalf Of Jiri Slaby
Sent: Friday, March 16, 2012 5:57 PM
To: Du, Alek
Cc: Jiri Slaby; Alan Cox; Tu, Xiaobing; linux-kernel@vger.kernel.org; gregkh@linuxfoundation.org; Zhang, Yanmin; Zuo, Jiao
Subject: Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

On 03/16/2012 10:49 AM, Du, Alek wrote:
> The tty structure is protect and not null, the tty->buf.tail is null. 
> Many people think the tty reference count isn't protected and cause 
> this bug, it is not true. :-)

You protect the pointer, not what is inside that structure. If you increment a tty reference count, tty->buf.tail won't be NULL iff tty is not NULL. IOW, you have to use tty_port_tty_set/get all around.

> For the flush case, it need acquire the spinlock to free the buffer and put buf.tail to NULL. So this patch will help:
> 
> Here is the example place, you can see the __tty_buffer_flush is inside the spinlock.

I'm not talking about flush. I'm talking about prepare. It returns a pointer to a tty buffer which may be concurrently freed by flush.

     A                                   B
================================================
prepare(&chars);
                                        flush(); memcpy(chars, data_from_HW); <- chars is freed now flip();

thanks,
--
js
suse labs

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Jiri Slaby]

On 03/16/2012 11:01 AM, Du, Alek wrote:
> We prepared the buffer, and use it. And during this, we don't release the spinlock of buf.lock, how the flush could happen?
> 
>                A
> spin_lock
> prepare the buffer
> user the buffer
> spin unlock
> 
> 
>               B
> Spin_lock
> Flush
> Free the buffer
> Put buf.tail = NULL
> Spin unlock

Well, you do. Not all drivers use tty_insert_flip_string. Take a look at
tty_prepare_flip_string and its users.

thanks,
-- 
js
suse labs

RE: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Du, Alek]

If you really look at the original patch from Xiaobing, the tty_prepare_flip_string is also patched :-)
Actually it fills up all the possible spin_lock gaps in tty_buffer.c


@@ -344,13 +375,20 @@ EXPORT_SYMBOL(tty_schedule_flip);  int tty_prepare_flip_string(struct tty_struct *tty, unsigned char **chars,
 								size_t size)
 {
-	int space = tty_buffer_request_room(tty, size);
+	int space;
+	unsigned long flags;
+	struct tty_buffer *tb;
+	
+	spin_lock_irqsave(&tty->buf.lock, flags);
+	space = __tty_buffer_request_room(tty, size);
+
+	tb = tty->buf.tail;
 	if (likely(space)) {
-		struct tty_buffer *tb = tty->buf.tail;
 		*chars = tb->char_buf_ptr + tb->used;
 		memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
 		tb->used += space;
 	}
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	return space;
 }

Thanks,
Alek
-----Original Message-----
From: Jiri Slaby [mailto:jirislaby@gmail.com] On Behalf Of Jiri Slaby
Sent: Friday, March 16, 2012 6:04 PM
To: Du, Alek
Cc: jiris >> Jiri Slaby; Alan Cox; Tu, Xiaobing; linux-kernel@vger.kernel.org; gregkh@linuxfoundation.org; Zhang, Yanmin; Zuo, Jiao
Subject: Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

On 03/16/2012 11:01 AM, Du, Alek wrote:
> We prepared the buffer, and use it. And during this, we don't release the spinlock of buf.lock, how the flush could happen?
> 
>                A
> spin_lock
> prepare the buffer
> user the buffer
> spin unlock
> 
> 
>               B
> Spin_lock
> Flush
> Free the buffer
> Put buf.tail = NULL
> Spin unlock

Well, you do. Not all drivers use tty_insert_flip_string. Take a look at tty_prepare_flip_string and its users.

thanks,
--
js
suse labs

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Jiri Slaby]

On 03/16/2012 11:08 AM, Du, Alek wrote:
> If you really look at the original patch from Xiaobing, the tty_prepare_flip_string is also patched :-)
> Actually it fills up all the possible spin_lock gaps in tty_buffer.c
> 
> 
> @@ -344,13 +375,20 @@ EXPORT_SYMBOL(tty_schedule_flip);  int tty_prepare_flip_string(struct tty_struct *tty, unsigned char **chars,
>  								size_t size)
>  {
> -	int space = tty_buffer_request_room(tty, size);
> +	int space;
> +	unsigned long flags;
> +	struct tty_buffer *tb;
> +	
> +	spin_lock_irqsave(&tty->buf.lock, flags);
> +	space = __tty_buffer_request_room(tty, size);
> +
> +	tb = tty->buf.tail;
>  	if (likely(space)) {
> -		struct tty_buffer *tb = tty->buf.tail;
>  		*chars = tb->char_buf_ptr + tb->used;
                ^^^^^^
This is returned to the caller. And it writes to that. And it may be
gone as soon as the lock is unlocked below.

>  		memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
>  		tb->used += space;
>  	}
> +	spin_unlock_irqrestore(&tty->buf.lock, flags);
>  	return space;
>  }

thanks,
-- 
js
suse labs

RE: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Du, Alek]

Isn't this the nature of "flush" ? If the driver is trying to insert chars to tty buffer, while the app is going to flush the buffer, or shutdown the port, 
that's quite nature that those data gone. Why should I care ?

Sorry, you missed the point of this patch.

Thanks,
Alek
-----Original Message-----
From: Jiri Slaby [mailto:jirislaby@gmail.com] On Behalf Of Jiri Slaby
Sent: Friday, March 16, 2012 6:22 PM
To: Du, Alek
Cc: Jiri Slaby; Alan Cox; Tu, Xiaobing; linux-kernel@vger.kernel.org; gregkh@linuxfoundation.org; Zhang, Yanmin; Zuo, Jiao
Subject: Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

On 03/16/2012 11:08 AM, Du, Alek wrote:
> If you really look at the original patch from Xiaobing, the 
> tty_prepare_flip_string is also patched :-) Actually it fills up all 
> the possible spin_lock gaps in tty_buffer.c
> 
> 
> @@ -344,13 +375,20 @@ EXPORT_SYMBOL(tty_schedule_flip);  int tty_prepare_flip_string(struct tty_struct *tty, unsigned char **chars,
>  								size_t size)
>  {
> -	int space = tty_buffer_request_room(tty, size);
> +	int space;
> +	unsigned long flags;
> +	struct tty_buffer *tb;
> +	
> +	spin_lock_irqsave(&tty->buf.lock, flags);
> +	space = __tty_buffer_request_room(tty, size);
> +
> +	tb = tty->buf.tail;
>  	if (likely(space)) {
> -		struct tty_buffer *tb = tty->buf.tail;
>  		*chars = tb->char_buf_ptr + tb->used;
                ^^^^^^
This is returned to the caller. And it writes to that. And it may be gone as soon as the lock is unlocked below.

>  		memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
>  		tb->used += space;
>  	}
> +	spin_unlock_irqrestore(&tty->buf.lock, flags);
>  	return space;
>  }

thanks,
--
js
suse labs

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Jiri Slaby]

On 03/16/2012 12:33 PM, Du, Alek wrote:
> Isn't this the nature of "flush" ? If the driver is trying to insert chars to tty buffer, while the app is going to flush the buffer, or shutdown the port, 
> that's quite nature that those data gone. Why should I care ?
> 
> Sorry, you missed the point of this patch.

No, you do not follow what I am writing.

1) Your driver is broken.
2) Your patch does not solve the problem universally.

ad 1) Use tty refcounting and you won't need to care about buf.tail
being NULL due to shutdown. As an added value, you will fix the other
races too.

ad 2) With your patch, drivers which use tty_prepare_flip_string are
given a tty buffer (chars parameter) which may be *freed* (not flushed,
_freed_) at any time by ioctl flush after the function returns. I.e. the
relocking is hopeless in that case. It only helps drivers which use
tty_insert_flip_string*.

I don't know right now what could be a real cure for point 2). Maybe
RCU. Maybe leaving the lock locked in tty_prepare_flip_string and
introducing tty_finish_flip_string containing an unlock. To be called
right after memcpy_fromio or alike in the drivers.

Or maybe even removing that interface completely...

thanks,
-- 
js
suse labs

RE: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Du, Alek]

For #1, as I said, we already held the tty refcount. The tty refcount cannot help this crash.

For #2, this patch will avoid memcpy to a NULL pointer and avoid a kernel crash.

Thanks,
Alek
-----Original Message-----
From: Jiri Slaby [mailto:jirislaby@gmail.com] On Behalf Of Jiri Slaby
Sent: Friday, March 16, 2012 8:04 PM
To: Du, Alek
Cc: Jiri Slaby; Alan Cox; Tu, Xiaobing; linux-kernel@vger.kernel.org; gregkh@linuxfoundation.org; Zhang, Yanmin; Zuo, Jiao
Subject: Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

On 03/16/2012 12:33 PM, Du, Alek wrote:
> Isn't this the nature of "flush" ? If the driver is trying to insert 
> chars to tty buffer, while the app is going to flush the buffer, or shutdown the port, that's quite nature that those data gone. Why should I care ?
> 
> Sorry, you missed the point of this patch.

No, you do not follow what I am writing.

1) Your driver is broken.
2) Your patch does not solve the problem universally.

ad 1) Use tty refcounting and you won't need to care about buf.tail being NULL due to shutdown. As an added value, you will fix the other races too.

ad 2) With your patch, drivers which use tty_prepare_flip_string are given a tty buffer (chars parameter) which may be *freed* (not flushed,
_freed_) at any time by ioctl flush after the function returns. I.e. the relocking is hopeless in that case. It only helps drivers which use tty_insert_flip_string*.

I don't know right now what could be a real cure for point 2). Maybe RCU. Maybe leaving the lock locked in tty_prepare_flip_string and introducing tty_finish_flip_string containing an unlock. To be called right after memcpy_fromio or alike in the drivers.

Or maybe even removing that interface completely...

thanks,
--
js
suse labs

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Jiri Slaby]

On 03/16/2012 02:50 PM, Du, Alek wrote:
> For #1, as I said, we already held the tty refcount. The tty refcount cannot help this crash.

Where exactly? Do you have some local changes?
$ grep tty_port_tty_ drivers/tty/serial/mfd.c
$

> For #2, this patch will avoid memcpy to a NULL pointer and avoid a kernel crash.

Ok, could you explain how? Linearized code from moxa.c with your patch
applied follows:
ofs = baseAddr + DynPage_addr + bufhead + head;
size = (tail >= head) ? (tail - head) : (rx_mask + 1 - head);
size = min(size, count);
// call to tty_prepare_flip_string
	spin_lock_irqsave(&tty->buf.lock, flags);
	space = __tty_buffer_request_room(tty, size);

	tb = tty->buf.tail;
 	if (likely(space)) {
 		*chars = tb->char_buf_ptr + tb->used;
 		memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
 		tb->used += space;
 	}
	spin_unlock_irqrestore(&tty->buf.lock, flags);
// return to MoxaPortReadData
memcpy_fromio(*chars, ofs, space); <- memcpy without buf.lock
head = (head + len) & rx_mask;
...

thanks,
-- 
js
suse labs

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Alan Cox]

> Or maybe even removing that interface completely...

We should be able to fix that interface fairly simply. If you have a
completed call then you can mark the buffer not to be freed and only
reap it post flush on the completed call.

Once the BKL remnants are gone we can hold tty->ldisc_mutex over ldisc
receive paths at which point this can be turned into a sane API.

Alan

Re: [PATCH] tty: hold lock across tty buffer finding and buffer filling

[Alek Du]

> On 03/16/2012 02:50 PM, Du, Alek wrote:
>> For #1, as I said, we already held the tty refcount. The tty refcount cannot help this crash.
> 
> Where exactly? Do you have some local changes?
> $ grep tty_port_tty_ drivers/tty/serial/mfd.c
> $

Yes, we do have local changes not upstream yet. But you should know now, TTY recount is not the key of the spin lock hole.


> 
>> For #2, this patch will avoid memcpy to a NULL pointer and avoid a kernel crash.
> 
> Ok, could you explain how? Linearized code from moxa.c with your patch
> applied follows:
> ofs = baseAddr + DynPage_addr + bufhead + head;
> size = (tail >= head) ? (tail - head) : (rx_mask + 1 - head);
> size = min(size, count);
> // call to tty_prepare_flip_string
>    spin_lock_irqsave(&tty->buf.lock, flags);
>    space = __tty_buffer_request_room(tty, size);
> 
>    tb = tty->buf.tail;
>    if (likely(space)) {
>        *chars = tb->char_buf_ptr + tb->used;
>        memset(tb->flag_buf_ptr + tb->used, TTY_NORMAL, space);
>        tb->used += space;
>    }
>    spin_unlock_irqrestore(&tty->buf.lock, flags);
> // return to MoxaPortReadData
> memcpy_fromio(*chars, ofs, space); <- memcpy without buf.lock

Ok. This is true. Seems we need another patch to remove this prepare interface. And let callers use the protected interface.

Thanks,
Alek


> head = (head + len) & rx_mask;
> ...
> 
> thanks,
> -- 
> js
> suse labs