2003-03-03 17:59:15

by Nicolas Pitre

Subject: [patch] small tty irq race fix

--- linux-2.5.63/drivers/char/tty_io.c.orig Mon Feb 24 14:05:34 2003
+++ linux-2.5.63/drivers/char/tty_io.c Mon Mar 3 13:02:31 2003
@@ -1947,17 +1947,17 @@
if (tty->flip.buf_num) {
cp = tty->flip.char_buf + TTY_FLIPBUF_SIZE;
fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
- tty->flip.buf_num = 0;

local_irq_save(flags); // FIXME: is this safe?
+ tty->flip.buf_num = 0;
tty->flip.char_buf_ptr = tty->flip.char_buf;
tty->flip.flag_buf_ptr = tty->flip.flag_buf;
} else {
cp = tty->flip.char_buf;
fp = tty->flip.flag_buf;
- tty->flip.buf_num = 1;

local_irq_save(flags); // FIXME: is this safe?
+ tty->flip.buf_num = 1;
tty->flip.char_buf_ptr = tty->flip.char_buf + TTY_FLIPBUF_SIZE;
tty->flip.flag_buf_ptr = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
}


Nicolas

2003-03-03 18:51:36

by Alan

Subject: Re: [patch] small tty irq race fix

> fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
> - tty->flip.buf_num = 0;
>
> local_irq_save(flags); // FIXME: is this safe?
> + tty->flip.buf_num = 0;

The other CPU can be touching these fields too surely. Its a
useful note that the spinlocks need putting in the right spot
but its still broken 8(

2003-03-03 19:05:21

by Nicolas Pitre

Subject: Re: [patch] small tty irq race fix

On 3 Mar 2003, Alan Cox wrote:

>
> > fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
> > - tty->flip.buf_num = 0;
> >
> > local_irq_save(flags); // FIXME: is this safe?
> > + tty->flip.buf_num = 0;
>
> The other CPU can be touching these fields too surely. Its a
> useful note that the spinlocks need putting in the right spot
> but its still broken 8(

Well I only care about UP at the moment and the patch makes it right for UP
at least. Someone with his brain around the tty locking requirements can
look at replacing the local_irq_save() (there and elsewhere as well), which
is sort of a different issue.


Nicolas

2003-03-03 21:10:37

by Nicolas Pitre

Subject: Re: [patch] small tty irq race fix

On Mon, 3 Mar 2003, Alan Cox wrote:

>
> > fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
> > - tty->flip.buf_num = 0;
> >
> > local_irq_save(flags); // FIXME: is this safe?
> > + tty->flip.buf_num = 0;
>
> The other CPU can be touching these fields too surely. Its a
> useful note that the spinlocks need putting in the right spot
> but its still broken 8(

What about this one? It just happens that tty->read_lock is actually used
deeper in the same call instance (in n_tty.c) so this looks to be the best
lock to use.

--- linux-2.5.63/drivers/char/tty_io.c.orig Mon Feb 24 14:05:34 2003
+++ linux-2.5.63/drivers/char/tty_io.c Mon Mar 3 16:13:30 2003
@@ -1947,23 +1947,23 @@
if (tty->flip.buf_num) {
cp = tty->flip.char_buf + TTY_FLIPBUF_SIZE;
fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
- tty->flip.buf_num = 0;

- local_irq_save(flags); // FIXME: is this safe?
+ spin_lock_irqsave(&tty->read_lock, flags);
+ tty->flip.buf_num = 0;
tty->flip.char_buf_ptr = tty->flip.char_buf;
tty->flip.flag_buf_ptr = tty->flip.flag_buf;
} else {
cp = tty->flip.char_buf;
fp = tty->flip.flag_buf;
- tty->flip.buf_num = 1;

- local_irq_save(flags); // FIXME: is this safe?
+ spin_lock_irqsave(&tty->read_lock, flags);
+ tty->flip.buf_num = 1;
tty->flip.char_buf_ptr = tty->flip.char_buf + TTY_FLIPBUF_SIZE;
tty->flip.flag_buf_ptr = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
}
count = tty->flip.count;
tty->flip.count = 0;
- local_irq_restore(flags); // FIXME: is this safe?
+ spin_unlock_irqrestore(&tty->read_lock, flags);

tty->ldisc.receive_buf(tty, cp, fp, count);
}

Nicolas

2003-03-03 21:31:47

by Linus Torvalds

Subject: Re: [patch] small tty irq race fix

On Mon, 3 Mar 2003, Nicolas Pitre wrote:
>
> What about this one? It just happens that tty->read_lock is actually used
> deeper in the same call instance (in n_tty.c) so this looks to be the best
> lock to use.

Looks ok. I would suggest moving the "spin_lock_irqsave()" to outside the
'if'-statement, though, since that should make the code a lot more
readable, and if the lock is supposed to protect tty->flip.buf_num, then
let's do it right and protect the read as well, no?

Linus

2003-03-03 21:41:29

by Nicolas Pitre

Subject: Re: [patch] small tty irq race fix

On Mon, 3 Mar 2003, Linus Torvalds wrote:

>
> On Mon, 3 Mar 2003, Nicolas Pitre wrote:
> >
> > What about this one? It just happens that tty->read_lock is actually used
> > deeper in the same call instance (in n_tty.c) so this looks to be the best
> > lock to use.
>
> Looks ok. I would suggest moving the "spin_lock_irqsave()" to outside the
> 'if'-statement, though, since that should make the code a lot more
> readable, and if the lock is supposed to protect tty->flip.buf_num, then
> let's do it right and protect the read as well, no?

Oh sure.

--- linux-2.5.63/drivers/char/tty_io.c.orig Mon Feb 24 14:05:34 2003
+++ linux-2.5.63/drivers/char/tty_io.c Mon Mar 3 16:49:44 2003
@@ -1944,27 +1944,25 @@
schedule_delayed_work(&tty->flip.work, 1);
return;
}
+
+ spin_lock_irqsave(&tty->read_lock, flags);
if (tty->flip.buf_num) {
cp = tty->flip.char_buf + TTY_FLIPBUF_SIZE;
fp = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
tty->flip.buf_num = 0;
-
- local_irq_save(flags); // FIXME: is this safe?
tty->flip.char_buf_ptr = tty->flip.char_buf;
tty->flip.flag_buf_ptr = tty->flip.flag_buf;
} else {
cp = tty->flip.char_buf;
fp = tty->flip.flag_buf;
tty->flip.buf_num = 1;
-
- local_irq_save(flags); // FIXME: is this safe?
tty->flip.char_buf_ptr = tty->flip.char_buf + TTY_FLIPBUF_SIZE;
tty->flip.flag_buf_ptr = tty->flip.flag_buf + TTY_FLIPBUF_SIZE;
}
count = tty->flip.count;
tty->flip.count = 0;
- local_irq_restore(flags); // FIXME: is this safe?
-
+ spin_unlock_irqrestore(&tty->read_lock, flags);
+
tty->ldisc.receive_buf(tty, cp, fp, count);
}



Nicolas