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

["Frédéric L. W. Meunier" <fredlwm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Mon, 12 Oct 2009, Justin P. Mattock wrote:

> Linus Torvalds wrote:
>> [ 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);
>>
>> 
> I can throw your patch in over here for the heck of it.
> If there's somebody who's really hitting this bug
> then the results would be better  if this is the area that causing
> this bug.(from here the only issue I'm seeing is spinning
> history commands in the terminal  from time to time,
> nothing of any unusable keys like others are reporting).

I tested it on top of 2.6.31.4 (after putting back 
e043e42bdb66885b3ac10d27a01ccb9972e2b0a3), and the keyboard is 
fine after almost 3h. Before that, the problems would appear in 
less than 1h. Maybe I spoke too soon, but...

Boyan, does it work for you ?