Re: [Bug #14388] keyboard under X with 2.6.31

[Paul Fulghum <paulkf-CZzk0lvu1V9l57MIdRCFDg@public.gmane.org>]

On Tue, 2009-10-13 at 15:42 -0700, Linus Torvalds wrote:

> They are added to the tail only if the tail is non-NULL.

True

tail is null only after:
* initialization - no flush_to_ldisc in progress
* tty_flush_buffer - protected from flush_to_ldisc by TTY_FLUSHING

flush_to_ldisc does not currently set tail to null after
flushing data from the last buffer. Each buffer is marked
individually as consumed so no more data will be added to it.
The buffer is marked empty again as it is recycled.

However, looking at this does reveal a problem.

If an individual buffer of 512 bytes or larger gets into the
free and full lists, that buffer is kfreed in tty_buffer_free()
instead of being recycled. That means that tty->buf.tail can
point to a freed block of memory, at least until another buffer
is allocated.

If that buffer is written to while in this state, the fields
will become incoherent and may result in more data being added to it.

I think the patch below fixes this problem. It sets tail to null
when all buffers are flushed. This is only executed after the buffer
has been passed to the ldisc and the spinlock is held so there is
no place for more data to be added incorrectly. I will test it myself
tomorrow when I get back to the office.

> I do object to the whole crazy subtle TTY locking. I'm convinced it's 
> wrong, and I'm convinced it's wrong exactly _because_ it tries to be so 
> subtle and does non-obvious things.

I understand. The patch below should fix the hole above, and I'm not
aware of any other hole. But I you prefer reworking the locking
to make things more obvious, I have no objection.

--- a/drivers/char/tty_buffer.c	2009-09-09 17:13:59.000000000 -0500
+++ b/drivers/char/tty_buffer.c	2009-10-13 18:34:34.000000000 -0500
@@ -423,6 +423,8 @@ static void flush_to_ldisc(struct work_s
 					break;
 				tbuf = head;
 				head = head->next;
+				if (!head)
+					tty->buf.tail = NULL;
 				tty_buffer_free(tty, tbuf);
 				continue;
 			}