vgone() calling VOP_CLOSE() -> blocked threads?

[Ed Schouten]

Hello Bruce,

* Bruce Evans <brde at optusnet.com.au> wrote:
> On Sat, 15 Mar 2008, Ed Schouten wrote:
>
>> The last couple of days I'm seeing some strange things in my mpsafetty
>> branch related to terminal revocation.
>>
>> In my current TTY design, I hold a count (t_ldisccnt) of the amount of
>> threads that are sleeping in the line discipline. I need to store such a
>> count, because it's not possible to change line disciplines while some
>> threads are still blocked inside the discipline. This means that when
>> d_close() is called on a TTY, t_ldisccnt should always be 0. There
>> cannot be any threads stuck inside the line discipline when there aren't
>> any descriptors referencing it.
>>
>> Unfortunately, this isn't entirely true with the current VFS/devfs
>> design. When vgone() is called, a VOP_CLOSE() is performed , which means
>> there could be a dozen threads still stuck inside a device driver, but
>> the close routine is already called to clean up stuff. There are a
>> *real* lot of drivers that blindly clean up their stuff in the d_close()
>> routine, expecting that the device is completely unused. This can
>> easily be demonstrated by revoking a bpf device, while running tcpdump.
>
> Yes, most drivers are broken here, but the problem is rarely noticed
> because revoke() isn't normally applied to any devices except ttys.
> Even ordinary close() can cause problems when a thread is sleeping
> in device open, but this too is only common for ttys (for callin and
> callout devices).

I think I could in theory work around these crashes by using the
si_threadcount, but I'd rather not. Even though revoke() is only used on
TTY devices, I don't think that's a valid reason to allow FreeBSD to
crash in such cases.

> [...]
>
>> - Maybe vgonel() shouldn't call VOP_CLOSE(). It should probably move the
>>  vnode into deadfs, with the exception of the close() routine. Maybe
>>  it's better to add a new function to do this, vrevoke().
>>
>> This means that when a revoke() call is performed, all blocked threads
>> are woken up, will leave the driver, to find out their terminal has been
>> revoked. Further system calls will fail, because the vnode is in deadfs,
>> but when the processes close the descriptor, the device driver can still
>> clean up everything.
>
> I think vfs already moves the vnode to deadfs.  It doesn't do anything
> to synchronize with threads running in device drivers.  The forced
> last-close() should complete synchronously as part of revoke().  Then
> other threads leave the device driver asynchronously, hopefully not
> much later.  Then if the generation count stuff is working right, the
> syscall is restarted, but now file descriptors point to deadfs so the
> syscall normally fails.  I think the async completion is OK provided
> it is done right (don't delay it indefinitely, and don't do more
> i/o on completion).  It doesn't seem to be useful to make revoke()
> wait for the completions.
>
> I don't think it would work well to move everything except d_close to
> deadfs.

It wasn't my idea to make revoke() wait for all threads to leave. It
should just inform the device driver that a revoke() has been performed,
to wake up sleeping threads, and change the vnode to prevent further
access.

The problem with the current implementation is that the device driver
cannot sanely determine whether a revoke() or a real close() is called.
Especially in my new TTY design, where a TTY could even be deallocated
when a close() is performed - when the device driver has abandoned the
TTY device - it would even destroy the TTY object that's being used by
the sleeping threads.

This is why I chose an approach that would allow threads to just leave
the device driver as they normally would, which reduces complexity a
lot.

My question is: what approach would you take in such a situation?
Thanks for your input so far.

-- 
 Ed Schouten <ed at fxq.nl>
 WWW: http://g-rave.nl/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-arch/attachments/20080317/67a595b8/attachment.pgp