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: Sun, 12 Aug 2012 14:15:25 +0100

 On 12 Aug, 2012, at 08:25 , Christos Zoulas wrote:
 
 > | 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.
 >=20
 > 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.
 
 I only know this because I've been working on that driver, but the
 protocol most drivers seem to follow works like this:
 
 - call ndqb() to get the number of bytes available in the clist.  If
   there are some, start the tx interrupt/DMA to send those bytes.
 
 - when the above completes, call ndflush() to remove the bytes sent
   from the clist.
 
 When the driver calls ndqb() or ndflush(), or when the clist byte
 count is zero, it should be quite safe to assume there is no tx output
 in progress.
 
 Given this I'm pretty sure this might work:
 
 (1) Change all drivers which call ndqb() to copy the tp->t_outq.c_cf
     pointer only after the ndqb() call (or, better, make ndqb() return
     the pointer as well as the count, or something).  This avoids a
     potential race when changing what tp->t_outq.c_cf points to.
 
 (2) When replacing the output queue memory, if the byte count in the =
 clist
     is non-zero save a reference to the old buffer memory (no need to =
 keep the
     bookkeeping, just the buffer itself) rather than freeing it.
 
 (3) If the driver calls ndqb() check to see if an old buffer is present =
 and,
     if it is, free it (before returning information from the current =
 clist,
     as normal).  If the driver is calling ndqb() it is safe to dump the =
 old
     memory.
 
 (4) If the driver calls ndflush() when an old buffer is present, free =
 the old
     buffer and do not flush anything from the current buffer (i.e. =
 ignore the
     byte count).  If the driver is calling ndflush() whatever output it =
 was
     doing has completed, so it is safe to free the old buffer, but if =
 the old
     buffer is present the byte count it is giving you must be from the
     previous buffer and shouldn't be subtracted from the current clist.
 
 (5) While most drivers use the ndqb()/ndflush(), there may be a single
     character _get() routine as well (I'm doing this from memory since
     I'm traveling and away from my sources).  Any code which reads bytes
     from the outq should similarly check for and free the old buffer.
 
 If that's too much work, however, returning EBUSY when the output queue
 has bytes in it should also be safe.
 
 Dennis Ferguson=