[PATCH] tty_buffer: avoid race due to tty_buffer_free_all() being misused

[PATCH] tty_buffer: avoid race due to tty_buffer_free_all() being misused

[channing]

In tty_buffer.c, function tty_buffer_free_all() is used to remove
all buffers for a tty, although it's declared that it mustn't be called
when the tty is in use, it cannot guarantee that. we can observe some
device driver make use it by mistake, for example, while tty device is
releasing, the tty data forwarding is not stopped, then it might hit
the case that tty buffer is being used while tty_buffer_free_all()
free this tty buffer, and finally lead to random error at any places,
and it's not clear to debug.

Although device driver could do better, it's simpler and safer to
strengthen protection in the view of tty buffer, by adding a tty->buf.lock
in tty_buffer_free_all() to avoid it racing with ongoing tty buffer
operations.

Signed-off-by: channing <chao.bi@intel.com>
---
 drivers/tty/tty_buffer.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 9121c1f..c7c100d 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -32,7 +32,9 @@ void tty_buffer_free_all(struct tty_port *port)
 {
 	struct tty_bufhead *buf = &port->buf;
 	struct tty_buffer *thead;
+	unsigned long flags;
 
+	spin_lock_irqsave(&buf->lock, flags);
 	while ((thead = buf->head) != NULL) {
 		buf->head = thead->next;
 		kfree(thead);
@@ -43,6 +45,7 @@ void tty_buffer_free_all(struct tty_port *port)
 	}
 	buf->tail = NULL;
 	buf->memory_used = 0;
+	spin_unlock_irqrestore(&buf->lock, flags);
 }
 
 /**
-- 
1.7.1

Re: [PATCH] tty_buffer: avoid race due to tty_buffer_free_all() being misused

[Peter Hurley]

On 05/16/2013 04:59 AM, channing wrote:
>
> In tty_buffer.c, function tty_buffer_free_all() is used to remove
> all buffers for a tty, although it's declared that it mustn't be called
> when the tty is in use, it cannot guarantee that. we can observe some
> device driver make use it by mistake, for example, while tty device is
> releasing, the tty data forwarding is not stopped, then it might hit
> the case that tty buffer is being used while tty_buffer_free_all()
> free this tty buffer, and finally lead to random error at any places,
> and it's not clear to debug.

What kernel version?

> Although device driver could do better, it's simpler and safer to
> strengthen protection in the view of tty buffer, by adding a tty->buf.lock
> in tty_buffer_free_all() to avoid it racing with ongoing tty buffer
> operations.

Sorry, but this isn't correct.

The driver cannot continue to perform i/o concurrently with
tty_port_destroy().

If the concurrent use you're observing is with flush_to_ldisc(),
that should be fixed in current mainline.

Regards,
Peter Hurley

Re: [PATCH] tty_buffer: avoid race due to tty_buffer_free_all() being misused

[channing]

On Thu, 2013-05-16 at 08:54 -0400, Peter Hurley wrote:
> On 05/16/2013 04:59 AM, channing wrote:
> >
> > In tty_buffer.c, function tty_buffer_free_all() is used to remove
> > all buffers for a tty, although it's declared that it mustn't be called
> > when the tty is in use, it cannot guarantee that. we can observe some
> > device driver make use it by mistake, for example, while tty device is
> > releasing, the tty data forwarding is not stopped, then it might hit
> > the case that tty buffer is being used while tty_buffer_free_all()
> > free this tty buffer, and finally lead to random error at any places,
> > and it's not clear to debug.
> 
> What kernel version?
3.4
> 
> > Although device driver could do better, it's simpler and safer to
> > strengthen protection in the view of tty buffer, by adding a tty->buf.lock
> > in tty_buffer_free_all() to avoid it racing with ongoing tty buffer
> > operations.
> 
> Sorry, but this isn't correct.
> 
> The driver cannot continue to perform i/o concurrently with
> tty_port_destroy().
> 
Thanks for remind, 3.4 haven't tty_port_destroy(), the mainline has
changed the way to call tty_buffer_free_all().

> If the concurrent use you're observing is with flush_to_ldisc(),
> that should be fixed in current mainline.
> 
Yes, when calling flush_to_ldisc() in Kernel 3.4, we could observe the
tty buffer is corrupted, and dummped stack shows that
tty_buffer_free_all() was called before. Is it a known issue fixed in
old version? would you please tell me related patch to solve this
flush_to_ldisc() issue? Thanks very much.

Chao

Re: [PATCH] tty_buffer: avoid race due to tty_buffer_free_all() being misused

[Peter Hurley]

On 05/17/2013 02:29 AM, channing wrote:
> On Thu, 2013-05-16 at 08:54 -0400, Peter Hurley wrote:
>> On 05/16/2013 04:59 AM, channing wrote:
>>>
>>> In tty_buffer.c, function tty_buffer_free_all() is used to remove
>>> all buffers for a tty, although it's declared that it mustn't be called
>>> when the tty is in use, it cannot guarantee that. we can observe some
>>> device driver make use it by mistake, for example, while tty device is
>>> releasing, the tty data forwarding is not stopped, then it might hit
>>> the case that tty buffer is being used while tty_buffer_free_all()
>>> free this tty buffer, and finally lead to random error at any places,
>>> and it's not clear to debug.
>>
>> What kernel version?
> 3.4
>>
>>> Although device driver could do better, it's simpler and safer to
>>> strengthen protection in the view of tty buffer, by adding a tty->buf.lock
>>> in tty_buffer_free_all() to avoid it racing with ongoing tty buffer
>>> operations.
>>
>> Sorry, but this isn't correct.
>>
>> The driver cannot continue to perform i/o concurrently with
>> tty_port_destroy().
>>
> Thanks for remind, 3.4 haven't tty_port_destroy(), the mainline has
> changed the way to call tty_buffer_free_all().
>
>> If the concurrent use you're observing is with flush_to_ldisc(),
>> that should be fixed in current mainline.
>>
> Yes, when calling flush_to_ldisc() in Kernel 3.4, we could observe the
> tty buffer is corrupted, and dummped stack shows that
> tty_buffer_free_all() was called before. Is it a known issue fixed in
> old version? would you please tell me related patch to solve this
> flush_to_ldisc() issue? Thanks very much.

Hi Chao,

Unfortunately, the fixes for flush_to_ldisc() continuing to
run during, and even after, tty teardown have not been backported
to stable kernels, mostly due to the extensive nature of the fixes.

In fact, even in current mainline and linux-next this problem is
still possible. Hopefully soon the remaining patches will be
reviewed and merged into linux-next.

For your reference, here is a link [1] to the patchset that was
partially accepted for 3.10 (up through 'tty: Fix recursive
deadlock in tty_perform_flush()')

Regards,
Peter Hurley

[1] [PATCH v5 00/44] ldisc patchset
http://www.gossamer-threads.com/lists/linux/kernel/1692114?do=post_view_flat

Re: [PATCH] tty_buffer: avoid race due to tty_buffer_free_all() being misused

[Greg Kroah-Hartman]

On Thu, May 16, 2013 at 04:59:22PM +0800, channing wrote:
> 
> In tty_buffer.c, function tty_buffer_free_all() is used to remove
> all buffers for a tty, although it's declared that it mustn't be called
> when the tty is in use, it cannot guarantee that. we can observe some
> device driver make use it by mistake, for example, while tty device is
> releasing, the tty data forwarding is not stopped, then it might hit
> the case that tty buffer is being used while tty_buffer_free_all()
> free this tty buffer, and finally lead to random error at any places,
> and it's not clear to debug.
> 
> Although device driver could do better, it's simpler and safer to
> strengthen protection in the view of tty buffer, by adding a tty->buf.lock
> in tty_buffer_free_all() to avoid it racing with ongoing tty buffer
> operations.
> 
> Signed-off-by: channing <chao.bi@intel.com>

Note, in the future, I need a "full" name for a signed-off-by: line and
>From line in the patch.

thanks,

greg k-h