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

[christos%zoulas.com@localhost (Christos Zoulas)]

The following reply was made to PR kern/46780; it has been noted by GNATS.

From: christos%zoulas.com@localhost (Christos Zoulas)
To: Dennis Ferguson <dennis%mistimed.com@localhost>, 
gnats-bugs%NetBSD.org@localhost
Cc: kern-bug-people%netbsd.org@localhost, gnats-admin%netbsd.org@localhost, 
        netbsd-bugs%netbsd.org@localhost
Subject: Re: kern/46780: tty TIOCSQSIZE can free clist buffers which drivers 
are still using for output
Date: Sun, 12 Aug 2012 03:24:37 -0400

 On Aug 11,  5:39pm, dennis%mistimed.com@localhost (Dennis Ferguson) wrote:
 -- Subject: Re: kern/46780: tty TIOCSQSIZE can free clist buffers which drive
 
 | 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).
 
 I agree, the point here is that locking is too expensive. And changing
 the order of assignment is also unsafe.
 
 | 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.
 
 Well, we could always wait for ndflush() but I will need to verify that
 this happens. We could also make the drivers voluntarily call a function
 to get the new pointers once they consume all they bytes from the old ones.
 Or we can make the ioctl fail with ebysy if the buffer is not empty.
 
 christos