[PATCH] hvc_console fix to protect hvc_write against ldisc write after hvc_close

[PATCH] hvc_console fix to protect hvc_write against ldisc write after hvc_close

[Ryan Arnold]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

Andrew,

Due to the tty ldisc code not stopping write operations against a driver
even after a tty has been closed I added a mechanism to hvc_console in
my previous patch to prevent this by nulling out the tty->driver_data in
hvc_close() but I forgot to check tty->driver_data in hvc_write(). 
Anton Blanchard got several oops'es from hvc_write() accessing NULL as
if it were a pointer to an hvc_struct usually stored in
tty->driver_data.

So this patch checks tty->driver_data in hvc_write() before it is used. 
Hopefully once Alan Cox's patch is checked in ldisc writes won't
continue to happen after tty closes.

Anton Blanchard has tested this patch and is unable to reproduce the
oops with it applied.

Changelog:
drivers/char/hvc_console.c
-Added comment to hvc_close() to explain the reason for NULLing
tty->driver_data.
-Added check to hvc_write() to verify that tty->driver_data is valid
(NOT NULL) which would be the case if the write operation was invoked
after a tty close was initiated on the tty.

Thanks,

Ryan S. Arnold
IBM Linux Technology Center

[-- Attachment #2: hvc_console_write.patch --]
[-- Type: text/x-patch, Size: 783 bytes --]

Signed-off-by: Ryan S. Arnold <rsa@us.ibm.com>
--- linux-2.6.9-rc1/drivers/char/hvc_console.c	Fri Sep 10 15:14:57 2004
+++ hvc_console_working_linux-2.6.9-rc1/drivers/char/hvc_console.c	Mon Sep 13 10:29:53 2004
@@ -265,6 +265,11 @@
 		 */
 		tty_wait_until_sent(tty, HVC_CLOSE_WAIT);
 
+		/*
+		 * Since the line disc doesn't block writes during tty close
+		 * operations we'll set driver_data to NULL and then make sure
+		 * to check tty->driver_data for NULL in hvc_write().
+		 */
 		tty->driver_data = NULL;
 
 		if (irq != NO_IRQ)
@@ -418,6 +423,10 @@
 	struct hvc_struct *hp = tty->driver_data;
 	int written;
 
+	/* This write was probably executed during a tty close. */
+	if (!hp)
+		return -EPIPE;
+
 	if (from_user)
 		written = __hvc_write_user(hp, buf, count);
 	else

Re: [PATCH] hvc_console fix to protect hvc_write against ldisc write after hvc_close

[Alan Cox]

On Mer, 2004-09-15 at 19:43, Ryan Arnold wrote:
> So this patch checks tty->driver_data in hvc_write() before it is used. 
> Hopefully once Alan Cox's patch is checked in ldisc writes won't
> continue to happen after tty closes.

Actually for the short term I may have made the ldisc calling tty race
worse. I'm still looking into some of that. I've fixed the one the other
way but for now driver defensively 8)

Re: [PATCH] hvc_console fix to protect hvc_write against ldisc write after hvc_close

[Theodore Ts'o]

On Wed, Sep 15, 2004 at 01:43:55PM -0500, Ryan Arnold wrote:
> Andrew,
> 
> Due to the tty ldisc code not stopping write operations against a driver
> even after a tty has been closed I added a mechanism to hvc_console in
> my previous patch to prevent this by nulling out the tty->driver_data in
> hvc_close() but I forgot to check tty->driver_data in hvc_write(). 
> Anton Blanchard got several oops'es from hvc_write() accessing NULL as
> if it were a pointer to an hvc_struct usually stored in
> tty->driver_data.
> 
> So this patch checks tty->driver_data in hvc_write() before it is used. 
> Hopefully once Alan Cox's patch is checked in ldisc writes won't
> continue to happen after tty closes.

The current (I can't speak to what Alan Cox is going to change) rules
with tty drivers is that tty drivers are supposed to close the line
discpline in their close routines.  That's the much safer and cleaner
way of fixing this problem, and it is in line with what most of the
other tty drivers are doing.

					- Ted

Re: [PATCH] hvc_console fix to protect hvc_write against ldisc write after hvc_close

[Ryan Arnold]

On Wed, 2004-09-15 at 14:20, Alan Cox wrote:
> Actually for the short term I may have made the ldisc calling tty race
> worse. I'm still looking into some of that. I've fixed the one the other
> way but for now driver defensively 8)

Alan, thanks for the heads up, and thanks for the work on the ldisc code
it is MUCH appreciated.

While we're looking at the ldisc code; I am a bit disturbed by how easy
it is to tty_flip_buffer_push() and silently lose data.  The ldisc
function n_tty_receive_buf() which is normally scheduled as delayed work
via the tty_flip_buffer_push() operation simply overwrites the contents
of the circular tty->read_buf leading to dataloss and out of order
output if too much data is pushed too quickly.

The manual accounting of pushed data is very tricky to do when the
pushing thread isn't sure of when the line discipline will actually
schedule the n_tty_receive_buf() operation.  I think that the tty
throttle() & unthrottle api is supposed to help with this but it is
possible (and in my experience was common) that the throttle callback
would happen AFTER data has already been lost.

Many of us tty driver writers resort to setting tty->low_latency = 1
which synchronizes the n_tty_receive_buf() call to be executed in the
same thread as that which invoked tty_flip_buffer_push() such that we
know when a throttle happens we haven't yet lost data.

I don't know when tty->low_latency is really appropriate to use but it
solves my problems.  I've heard people say that tty data delivery is
inherently unreliable.  This doesn't make sense to me because imho tty
data is very important.  I hope I'm not approaching this topic of
flip_buffer_pushing and throttling from a mistaken assumption.

Thanks,

Ryan S. Arnold
Linux Technology Center

Re: [PATCH] hvc_console fix to protect hvc_write against ldisc write after hvc_close

[Alan Cox]

On Mer, 2004-09-15 at 21:41, Theodore Ts'o wrote:
> The current (I can't speak to what Alan Cox is going to change) rules
> with tty drivers is that tty drivers are supposed to close the line
> discpline in their close routines. 

That has no actual effect in the real world because the ldisc can in
part be running on another processor at the same time in another
function.  This is a crash case seen on 2.4 in the real world.

Alan

Re: [PATCH] hvc_console fix to protect hvc_write against ldisc write after hvc_close

[Ryan Arnold]

On Wed, 2004-09-15 at 15:41, Theodore Ts'o wrote:
> The current (I can't speak to what Alan Cox is going to change) rules
> with tty drivers is that tty drivers are supposed to close the line
> discpline in their close routines.  That's the much safer and cleaner
> way of fixing this problem, and it is in line with what most of the
> other tty drivers are doing.
				- Ted

Thanks for the pointer Ted.  I've looked through the drivers/char
directory but I must be blind because all I see other drivers doing is
setting tty->closing = 1 (and then = 0 later) and
ldsic->flush_buffer().  Is flush_buffer() what you're refering to?  The
tty->closing variable just seems to prevent the ldisc from reading.

Ryan S. Arnold
IBM Linux Technology Center

ldisc writes during tty release_dev() causing problems.

[Ryan Arnold]

On Wed, 2004-09-15 at 15:41, Theodore Ts'o wrote:

> The current (I can't speak to what Alan Cox is going to change) rules
> with tty drivers is that tty drivers are supposed to close the line
> discpline in their close routines.  That's the much safer and cleaner
> way of fixing this problem, and it is in line with what most of the
> other tty drivers are doing.
> 					- Ted

Ted,

I greped through the drivers directory and it appears that the only tty
drivers that actually invoke ldisc.close() are those drivers that don't
use the N_TTY line discipline (serial drivers).

Many of the drivers actually call ldisc.flush_buffers() which
effectively does the same as a ldisc.close(), cleaning up the
tty->read_buf and tasks waiting on tty->read_wait.

The story is different with writes.  The tty layer dipatches a write via
the line discpline's write_chan() function.  The line discipline is then
responsible for delivering this data to the driver's write() function.

If driver->write() returns 0 [device is blocking] (which it can do often
in the case of hvc_console) the ldisc write_chan() function will
schedule() and get put on the tty->write_wait queue with the expectation
that it will be awoken by the driver when the device is available for
further writes.

If tty release_dev() is called when write_chan() is blocked awaiting I/O
with the driver and tty->count == 1 the final call to driver->close()
will clean up the driver without the ldisc knowing that the driver has
closed the device.

Following this release_dev() actually awakens the tty->write_wait, which
basically tells the ldisc to continue trying to send data to the driver
for the device it has just told the driver to close!

Ideally I would want hvc_close() to be able to tell the ldisc to wake up
the thread blocking on tty->write_wait and discard any unsent data. 
Unless I'm mistaken there is currrently no way to do this.

I suppose I could make hvc_write() return -EIO if hvc_struct->count ==
0.  I can't think of any races with this but it doesn't seem correct.

Additionally, there doesn't seem to be anything to prevent hangup being
called on a tty during the final close operation (this actually happened
in hvc_console).

Do you have any wisdom or advice?  Am I missing something obvious?

Ryan S. Arnold
IBM Linux Technology Center

Re: ldisc writes during tty release_dev() causing problems.

[Alan Cox]

On Gwe, 2004-09-24 at 02:15, Ryan Arnold wrote:
> I greped through the drivers directory and it appears that the only tty
> drivers that actually invoke ldisc.close() are those drivers that don't
> use the N_TTY line discipline (serial drivers).

Doing ldisc closes in the driver close path is also rather unsafe. The
ones I've found doing it I've removed.

> Many of the drivers actually call ldisc.flush_buffers() which
> effectively does the same as a ldisc.close(), cleaning up the
> tty->read_buf and tasks waiting on tty->read_wait.

ldisc dependant. It flushes the buffers but may not stop pending 
queued events.

> If tty release_dev() is called when write_chan() is blocked awaiting I/O
> with the driver and tty->count == 1 the final call to driver->close()
> will clean up the driver without the ldisc knowing that the driver has
> closed the device.

That should not be possible. The terminal cannot be both blocking in
write_chan and executing a close path for the final opener.

> Following this release_dev() actually awakens the tty->write_wait, which
> basically tells the ldisc to continue trying to send data to the driver
> for the device it has just told the driver to close!

In the old 2.0/lock_kernel world the wakeup would have woken the device
which would always have tested for hangup before queuing I/O. Nowdays a
lot of those are broken.

> Ideally I would want hvc_close() to be able to tell the ldisc to wake up
> the thread blocking on tty->write_wait and discard any unsent data. 
> Unless I'm mistaken there is currrently no way to do this.

There is no way to do this. The close path isn't too nasty but hangup
makes it all quite evil. 

> I suppose I could make hvc_write() return -EIO if hvc_struct->count ==
> 0.  I can't think of any races with this but it doesn't seem correct.

On SMP the risk is that you've got pending events on another processor.
We ought to be ok for the main part because of the ref count on the file
and the close code making sure it cleans up for the ldisc. We do
have a problem because the ldisc and driver both needed to close first
in the old model. Thats mostly cured since drivers now take ldisc refs
the ldisc can reliably close first.

> Additionally, there doesn't seem to be anything to prevent hangup being
> called on a tty during the final close operation (this actually happened
> in hvc_console).

I had assumed would have to come from the serial side because if the
last owner is executing close() they can't be executing vhangup on the
same but given a rapid close/re-open/vhangup on a new fd it might be
a dangerous assumption. Added to the TODO pile

Alan

Re: ldisc writes during tty release_dev() causing problems.

[Alan Cox]

On Gwe, 2004-09-24 at 17:18, Ryan Arnold wrote:
> I think I see what you mean.  The ldisc write_chan code is actually
> invoked from the terminal process and if this is I/O blocked then the
> terminal process cannot invoke release_dev().  So why did I experience
> calls to hvc_write() after the final open instance was closed with
> release_dev()?

At the moment if the user types a character and the line discipline
echoes it then you can get a write call to the driver. N_TTY tends to do
this one. When we close the ldisc first this should go away, but we
aren't quite there yet.

> > I had assumed would have to come from the serial side because if the
> > last owner is executing close() they can't be executing vhangup on the
> > same but given a rapid close/re-open/vhangup on a new fd it might be
> > a dangerous assumption. Added to the TODO pile
> 
> In hvc_console we don't have a serial side, but I do have the device
> side which will invoke a tty_hangup() if an hvc adapter was hotplug
> removed.  This doesn't happen for the actual console adapter (hvc0)
> because it isn't allowed in firmware.

Ok so this probably is a close v open->hangup race. In which case it
shouldn't be too hard to fix now the first locking work is done. Until
then the change you suggest makes sense. I'll investigate that race in
more detail however because close() v open() and close cancelling the
workqueue for hangup ought to be stopping it.

Re: ldisc writes during tty release_dev() causing problems.

[Ryan Arnold]

On Fri, 2004-09-24 at 07:40, Alan Cox wrote:

> > If tty release_dev() is called when write_chan() is blocked awaiting I/O
> > with the driver and tty->count == 1 the final call to driver->close()
> > will clean up the driver without the ldisc knowing that the driver has
> > closed the device.
> 
> That should not be possible. The terminal cannot be both blocking in
> write_chan and executing a close path for the final opener.

Thanks for the reply Alan,

I think I see what you mean.  The ldisc write_chan code is actually
invoked from the terminal process and if this is I/O blocked then the
terminal process cannot invoke release_dev().  So why did I experience
calls to hvc_write() after the final open instance was closed with
release_dev()?

> > Additionally, there doesn't seem to be anything to prevent hangup being
> > called on a tty during the final close operation (this actually happened
> > in hvc_console).
> 
> I had assumed would have to come from the serial side because if the
> last owner is executing close() they can't be executing vhangup on the
> same but given a rapid close/re-open/vhangup on a new fd it might be
> a dangerous assumption. Added to the TODO pile

In hvc_console we don't have a serial side, but I do have the device
side which will invoke a tty_hangup() if an hvc adapter was hotplug
removed.  This doesn't happen for the actual console adapter (hvc0)
because it isn't allowed in firmware.

So my strategy for now is going to be to get rid of the tty->driver_data
= NULL in hvc_close() and simply check the open count in hvc_hangup()
and hvc_write() to verify that these operations are actually relevant.

thanks again Alan,

Ryan S. Arnold
IBM Linux Technology Center