[src/trunk]: src/sys/dev vnd(4): Work around deadlock in VNDIOCCLR.

[riastradh <riastradh%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/345c9014a1ea
branches:  trunk
changeset: 366587:345c9014a1ea
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Tue May 31 14:13:31 2022 +0000

description:
vnd(4): Work around deadlock in VNDIOCCLR.

Since the changes this year to eliminate a host of races and
deadlocks in open, close, revoke, attach, and detach, closing the
last instance of a device special node has the side effect of waiting
for all concurrent I/O operations (read, write, ioctl, strategy, &c.)
on the device to complete.

Unfortunately, while this works for physical devices which revoke
open device nodes in their autoconf detach functions, as invoked by
some hardware interrupt indicating that the device is no longer
present, pseudo-devices like vnd(4) work differently -- or, work by
luck, or don't work any more.

VNDIOCCLR acts kind of like an autoconf detach function in that it
revokes open device nodes, which closes the last instance.  But
VNDIOCCLR is itself called via ioctl, which is an I/O operation that
close waits for.  So we end up with a deadlock, spec_io_drain waiting
for spec_close lower down in the call stack:

> spec_io_drain() at netbsd:spec_io_drain+0x84
> spec_close() at netbsd:spec_close+0x1c6
> VOP_CLOSE() at netbsd:VOP_CLOSE+0x38
> spec_node_revoke() at netbsd:spec_node_revoke+0x14d
> vcache_reclaim() at netbsd:vcache_reclaim+0x4e7
> vgone() at netbsd:vgone+0xcd
> vrevoke() at netbsd:vrevoke+0xfa
> genfs_revoke() at netbsd:genfs_revoke+0x13
> VOP_REVOKE() at netbsd:VOP_REVOKE+0x35
> vdevgone() at netbsd:vdevgone+0x64
> vnddoclear.part.0() at netbsd:vnddoclear.part.0+0xaa
> vndioctl() at netbsd:vndioctl+0x78c
> bdev_ioctl() at netbsd:bdev_ioctl+0x91
> spec_ioctl() at netbsd:spec_ioctl+0xa5
> VOP_IOCTL() at netbsd:VOP_IOCTL+0x41
> vn_ioctl() at netbsd:vn_ioctl+0xb3
> sys_ioctl() at netbsd:sys_ioctl+0x555

In the past, there was a workaround for what was presumably a crash
instead of a deadlock here: don't issue revoke (vdevgone) on the open
character devices for the minor number in use by the ioctl.  If you
use, e.g., `vnconfig -u vnd0', and vnconfig(8) picks /dev/rvnd0c or
/dev/rvnd0d, that special case kicks in.  But if you use `vnconfig -u
/dev/vnd0d', the ioctl will be issued on the block device instead, so
the special case doesn't kick in, so the operation deadlocks.

It is actually probably safe not to revoke the block device if what
the ioctl caller holds open is that, because specfs(9) forbids more
than one open of a block device, so nothing else can have it open
anyway.

Unclear what the consequences of failing to revoke the character
device are -- but this is what vnd(4) has done all along.  cgd(4) and
ccd(4) also don't bother to revoke.  We don't have a notion of
`revoke every file descriptor _except_ this one'; only a vnode as a
whole can be revoked, including all references to it.

This is a stop-gap measure to avoid a deadlock we are definitely
hitting on some users.  A slightly better measure would be to revoke
the block or character device according to which one is being used,
but that requires a little more work with two different d_ioctl
functions -- and wouldn't address isues with the character device.  A
proper solution requires identifying the appropriate protocol for all
of these pseudo-device disk drivers and using it uniformly for them.

Reported on current-users:
https://mail-index.netbsd.org/current-users/2022/05/27/msg042437.html

diffstat:

 sys/dev/vnd.c |  9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diffs (31 lines):

diff -r dcc6206a3343 -r 345c9014a1ea sys/dev/vnd.c
--- a/sys/dev/vnd.c     Tue May 31 13:42:58 2022 +0000
+++ b/sys/dev/vnd.c     Tue May 31 14:13:31 2022 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: vnd.c,v 1.285 2022/03/31 19:30:15 pgoyette Exp $       */
+/*     $NetBSD: vnd.c,v 1.286 2022/05/31 14:13:31 riastradh Exp $      */
 
 /*-
  * Copyright (c) 1996, 1997, 1998, 2008, 2020 The NetBSD Foundation, Inc.
@@ -91,7 +91,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: vnd.c,v 1.285 2022/03/31 19:30:15 pgoyette Exp $");
+__KERNEL_RCSID(0, "$NetBSD: vnd.c,v 1.286 2022/05/31 14:13:31 riastradh Exp $");
 
 #if defined(_KERNEL_OPT)
 #include "opt_vnd.h"
@@ -1767,9 +1767,10 @@
        /* Nuke the vnodes for any open instances */
        for (i = 0; i < MAXPARTITIONS; i++) {
                mn = DISKMINOR(device_unit(vnd->sc_dev), i);
-               vdevgone(bmaj, mn, mn, VBLK);
-               if (mn != myminor) /* XXX avoid to kill own vnode */
+               if (mn != myminor) { /* XXX avoid to kill own vnode */
+                       vdevgone(bmaj, mn, mn, VBLK);
                        vdevgone(cmaj, mn, mn, VCHR);
+               }
        }
 
        if ((vnd->sc_flags & VNF_READONLY) == 0)