hvc_console fix to protect hvc_write against ldisc write after hvc_close

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

@@ -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

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

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

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

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

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

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

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

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

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.

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

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.

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

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()?

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

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.

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.