Race condition in tty_flip_buffer_push/flush_to_ldisc?

Race condition in tty_flip_buffer_push/flush_to_ldisc?

[Martin Buck]

I think I've stumbled over a race condition in drivers/char/tty_io.c,
tty_flip_buffer_push()/flush_to_ldisc(). I noticed it in 2.4.19, but the
code in 2.5.49 looks similar.

Suppose I'm running a serial port with low_latency enabled. The low-level
serial interrupt handler will call tty_flip_buffer_push() which will
immediately call flush_to_ldisc() due to low_latency being set. In
flush_to_ldisc(), if the TTY_DONT_FLIP bit is set, it will add itself to
the timer task queue and return. When the task queue gets processed,
processing might get interrupted by another serial interrupt or vice versa,
resulting in 2 concurrent calls to flush_to_ldisc(). This time, the
TTY_DONT_FLIP bit probably isn't set, so they both will try to process the
same flip buffer.

Note that reading the current flip buffer pointers isn't protected by
cli(), only modifying them is. And even if it were, we could end up calling
tty->ldisc.receive_buf() twice for the same tty which probably isn't safe
either.

Any ideas on how to fix this?

And before you ask - yes, I do need low_latency. Not because of the low
latecy, but because I'm trying to use 921600 bps. At this speed, a 512
Bytes flip buffer gets filled in 5.5ms while timer task queue flip buffer
processing only flushes it every 10ms, resulting in slighly more characters
lost than I can accept :-)

Thanks,
Martin

Re: Race condition in tty_flip_buffer_push/flush_to_ldisc?

[Stuart MacDonald]

From: "Martin Buck" <mb-kernel0212@gromit.dyndns.org>
> Suppose I'm running a serial port with low_latency enabled. The low-level
> serial interrupt handler will call tty_flip_buffer_push() which will
> immediately call flush_to_ldisc() due to low_latency being set. In
> flush_to_ldisc(), if the TTY_DONT_FLIP bit is set, it will add itself to
> the timer task queue and return. When the task queue gets processed,
> processing might get interrupted by another serial interrupt or vice
versa,
> resulting in 2 concurrent calls to flush_to_ldisc(). This time, the
> TTY_DONT_FLIP bit probably isn't set, so they both will try to process the
> same flip buffer.

What causes the DONT_FLIP bit to be un/set? I don't think your
situation can occur under normal operation. But ICBW.

> Note that reading the current flip buffer pointers isn't protected by
> cli(), only modifying them is. And even if it were, we could end up
calling
> tty->ldisc.receive_buf() twice for the same tty which probably isn't safe
> either.
>
> Any ideas on how to fix this?

If you search the lkml archive, recently Ted stated that it's
acceptable to call the line discpline's receive_buf() routine
directly, bypassing the flip buffers.

..Stu

Re: Race condition in tty_flip_buffer_push/flush_to_ldisc?

[Martin Buck]

On Tue, Dec 03, 2002 at 11:02:12AM -0500, Stuart MacDonald wrote:
> What causes the DONT_FLIP bit to be un/set? I don't think your
> situation can occur under normal operation. But ICBW.

n_tty.c seems to do it in read_chan(), i.e. it will happen when the
process reading from the TTY is currently in read(2) while at the same time
new characters arrive on the serial port.

> If you search the lkml archive, recently Ted stated that it's
> acceptable to call the line discpline's receive_buf() routine
> directly, bypassing the flip buffers.

Interesting, that should solve my problem. But then the next question is:
What's the use of the DONT_FLIP bit if it's legal to bypass it by calling
receive_buf() directly?

Also, if somebody enables low_latency using setserial with the standard
serial driver, he can still trigger the race condition. It's not very
likely to happen, but it does happen - I added code to flush_to_ldisc() to
detect concurrent calls and it triggered once during an over-night test.

Thanks,
Martin