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

[Linus Torvalds]

[ Alan, Paulkf - the tty buffering and locking is originally your code, 
  although from about three years ago, when it used to be in tty_io.c..
  Any comment? ]

On Mon, 12 Oct 2009, Linus Torvalds wrote:
> 
> Alan, Ogawa-san, do either of you see some problem in tty_buffer.c, 
> perhaps?

Hmm. I see one, at least.

The "tty_insert_flip_string()" locking seems totally bogus. 

It does that "tty_buffer_request_room()" call and subsequent copying with 
no locking at all - sure, the tty_buffer_request_room() function itself 
locks the buffers, but then unlocks it when returning, so when we actually 
do the memcpy() etc, we can race with anybody.

I don't really see who would care, but it does look totally broken.

I dunno, this patch seems to make sense to me. Am I missing something?

[ NOTE! The patch is totally untested. It compiled for me on x86-64, and
  apart from that I'm just going to say that it looks obvious, and the old 
  code looks obviously buggy. Also, any remaining users of

	tty_prepare_flip_string
	tty_prepare_flip_string_flags

  are still fundamentally broken and buggy, while users of

	tty_buffer_request_room

  are pretty damn odd and suspect (but a lot of them seem to be just 
  pointless: they then call tty_insert_flip_string(), which means that the 
  tty_buffer_request_room() call was totally redundant ]

Comments? Does this work? Does it make any difference? It seems fairly 
unlikely, but it's the only obvious problem I've seen in the tty buffering 
code so far.

And that code is literally 3 years old, and it seems unlikely that a 
regular _keyboard_ buffer would be able to hit the (rather small) race 
condition. But other serialization may have hidden it, and timing 
differences could certainly have caused it to trigger much more easily.

			Linus

---
 drivers/char/tty_buffer.c |   33 +++++++++++++++++++++++++--------
 1 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tty_buffer.c b/drivers/char/tty_buffer.c
index 3108991..25ab538 100644
--- a/drivers/char/tty_buffer.c
+++ b/drivers/char/tty_buffer.c
@@ -196,13 +196,10 @@ static struct tty_buffer *tty_buffer_find(struct tty_struct *tty, size_t size)
  *
  *	Locking: Takes tty->buf.lock
  */
-int tty_buffer_request_room(struct tty_struct *tty, size_t size)
+static int locked_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
@@ -225,9 +222,20 @@ int tty_buffer_request_room(struct tty_struct *tty, size_t size)
 			size = left;
 	}
 
-	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	return size;
 }
+
+int tty_buffer_request_room(struct tty_struct *tty, size_t size)
+{
+	int retval;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->buf.lock, flags);
+	retval = locked_tty_buffer_request_room(tty, size);
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
+	return retval;
+}
+
 EXPORT_SYMBOL_GPL(tty_buffer_request_room);
 
 /**
@@ -239,16 +247,20 @@ EXPORT_SYMBOL_GPL(tty_buffer_request_room);
  *	Queue a series of bytes to the tty buffering. All the characters
  *	passed are marked as without error. Returns the number added.
  *
- *	Locking: Called functions may take tty->buf.lock
+ *	Locking: We take tty->buf.lock
  */
 
 int tty_insert_flip_string(struct tty_struct *tty, const unsigned char *chars,
 				size_t size)
 {
 	int copied = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tty->buf.lock, flags);
 	do {
-		int space = tty_buffer_request_room(tty, size - copied);
+		int space = locked_tty_buffer_request_room(tty, size - copied);
 		struct tty_buffer *tb = tty->buf.tail;
+
 		/* If there is no space then tb may be NULL */
 		if (unlikely(space == 0))
 			break;
@@ -260,6 +272,7 @@ int tty_insert_flip_string(struct tty_struct *tty, const unsigned char *chars,
 		/* There is a small chance that we need to split the data over
 		   several buffers. If this is the case we must loop */
 	} while (unlikely(size > copied));
+	spin_unlock_irqrestore(&tty->buf.lock, flags);
 	return copied;
 }
 EXPORT_SYMBOL(tty_insert_flip_string);
@@ -282,8 +295,11 @@ int tty_insert_flip_string_flags(struct tty_struct *tty,
 		const unsigned char *chars, const char *flags, size_t size)
 {
 	int copied = 0;
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&tty->buf.lock, irqflags);
 	do {
-		int space = tty_buffer_request_room(tty, size - copied);
+		int space = locked_tty_buffer_request_room(tty, size - copied);
 		struct tty_buffer *tb = tty->buf.tail;
 		/* If there is no space then tb may be NULL */
 		if (unlikely(space == 0))
@@ -297,6 +313,7 @@ int tty_insert_flip_string_flags(struct tty_struct *tty,
 		/* There is a small chance that we need to split the data over
 		   several buffers. If this is the case we must loop */
 	} while (unlikely(size > copied));
+	spin_unlock_irqrestore(&tty->buf.lock, irqflags);
 	return copied;
 }
 EXPORT_SYMBOL(tty_insert_flip_string_flags);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/