Re: Resolving open/close, attach/detach races

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Tue, 18 Jan 2022 20:05:24 +0100
> From: "J. Hannken-Illjes" <hannken%mailbox.org@localhost>
> 
> This doesn't work.  Running the attached program on a current kernel:
> 
> # ./revoke
> pty /dev/tty00, pid 2107
> revoke /dev/tty00
> read /dev/tty00 -> 0
> done
> 
> With your patch:
> 
> # ./revoke
> pty /dev/tty00, pid 975
> revoke /dev/tty00
> 
> and hangs hard.  The revoke() operation enters spec_node_revoke()
> and here it waits forever for sn_iocnt to become zero.
> 
> As the read from the other thread set sn_iocnt to one by entering
> through spec_io_enter() the system will hang until this read returns.
> 
> Am I missing something?

You're not missing anything; I missed something, and yesterday I
started drafting a change to fix it but haven't finished yet.

What I missed is that the only mechanism we have for interrupting read
(or write or ioctl) and causing it to fail is: close.  This means,
with devsw built as it is, we can't wait for all I/O operations to
complete before calling close.

So, two possibilities are:

1. Wait for sn_iocnt to drain _after_ the last .d_close.  I think
   we'll have to do this, but it's unfortunate because it would be
   nice if drivers didn't have to have driver-specific logic to wait
   for all reads and writes and stuff to drain.

2. Create a new .d_cancel operation.  A driver can implement this with
   a function that prevents anything in the driver from issuing new
   I/O operations and that interrupts pending ones, such as all
   cv_waits and usbd_sync_transfers and whatnot.  Then, for drivers
   which support it, the sequence in .d_close on last close will be:

	.d_cancel
	wait for sn_ioccnt to drain
	.d_close

   Of course, this will only work for drivers that opt into it with a
   .d_cancel function, but it will save a good deal of effort to have
   the `wait for I/O operations to drain before .d_close' logic
   centralized in spec_vnops.c (and perhaps be done with psref to keep
   it lightweight).

There's another issue which is that sn_opencnt management in spec_open
is racy -- I had missed the part of chuq's earlier patch dealing with
that.  So I'll have an updated patch soon to address these issues, but
I need to do some more work on it first and run through some more
tests.