Re: kern/46780: tty TIOCSQSIZE can free clist buffers which drivers are still using for output

[Dennis Ferguson <dennis%mistimed.com@localhost>]

On 11 Aug, 2012, at 04:50 , Christos Zoulas wrote:
> | I don't think this can be fixed in the serial device drivers, they have 
> good reasons to work the way they do.
> 
> How about http://www.netbsd.org/~christos/tty.diff ?

I'd like it if it could work that way, but I'm pretty sure that may be
dangerous.  The usual reason the drivers cache that pointer is that
they are sending those bytes from their interrupt service routine, and
they don't what the ISR touching anything in the struct tty (that would
require locking that the ISR isn't doing).  In your _flush() routines
you don't have the interrupt blocked and the update isn't atomic (i.e.
you write two things, the pointer and the byte count) so a concurrent
interrupt may see an inconsistent pair of values.  And worse is that
the ISR usually updates those values as it sends stuff, so if the ISR
is running concurrently on another core when _flush() writes those
values the ISR might rewrite one or both back to the old values,
slightly adjusted.

For the _flush() routine to work reliably it needs to lock out the ISR,
but how that is done is driver-specific; e.g. in the com driver it
requires taking sc->sc_lock.  And while it might be possible to fix
the drivers which exist to do that this is not guaranteed to be cheap
and easy.  For the driver I am working on, which caused me to notice
the problem in the first place, I was planning on not taking a spin
lock in the ISR (and saving that overhead) since for that device it
looks possible to produce an mpsafe driver while running the ISR
lockless; supporting the _flush() entry would likely add the cost of
the lock back in.  And this wouldn't fix, e.g., the VAX dhu driver
which does DMA output directly from the output clist buffer (i.e.
the pointer is cached inside the hardware rather than in the softc
structure).

While I still think it is possible to do a _flush() entry point I
think it is going to take more driver-specific code then you have.
The alternative of deferring the free of a non-empty output buffer's
memory (the bookkeeping needn't be kept, just the ring buffer memory
allocation) until the driver indicates that the hardware/interrupt
is inactive by calling ndqb() or ndflush() is probably easier even
if it does require some checks to be added to the 'normal' code
path.

Dennis Ferguson