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

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

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

From: Dennis Ferguson <dennis%mistimed.com@localhost>
To: 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: Sat, 11 Aug 2012 17:39:05 -0400

 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.
 >=20
 > 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=