[PATCH storage v2 3/7] zfsnvme: harden node preflight and storage teardown

[Joaquin Varela <joaquinvarela@neatech.ar>]

Fail activation before target mutation when a configured host
interface is absent. Reconcile controllers one path at a time
when their bound interface differs from storage configuration.

Before disconnecting a subsystem, reject teardown while a
userspace file descriptor or kernel holder consumes one of its
namespace heads. Also reject storage deletion while its remote
ZFS dataset owns volumes. These guards prevent an idle cluster
node from disrupting users on another.

Signed-off-by: Joaquin Varela <joaquinvarela@neatech.ar>
---
 src/PVE/Storage/ZFSNVMePlugin.pm | 87 ++++++++++++++++++++++++++++++--
 src/test/zfsnvme_test.pm         | 44 +++++++++++++++-
 2 files changed, 127 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Storage/ZFSNVMePlugin.pm b/src/PVE/Storage/ZFSNVMePlugin.pm
index 83d086c..d02dff4 100644
--- a/src/PVE/Storage/ZFSNVMePlugin.pm
+++ b/src/PVE/Storage/ZFSNVMePlugin.pm
@@ -46,9 +46,11 @@ my $RE_DHCHAP_KEY = qr{
 }nxx;
 my $RE_NVME_CONTROLLER = qr{\A nvme [0-9]+ \z}nxx;
 my $RE_NVME_SUBSYSTEM = qr{\A nvme-subsys [0-9]+ \z}nxx;
+my $RE_NVME_NAMESPACE = qr{\A nvme [0-9]+ n [0-9]+ \z}nxx;
 my $RE_TRADDR = qr{(?: \A | ,) traddr=(?<value>[^,]+)}nxx;
 my $RE_TRSVCID = qr{(?: \A | ,) trsvcid=(?<value>[^,]+)}nxx;
 my $RE_HOST_IFACE_ADDRESS = qr{(?: \A | ,) host_iface=(?<value>[^,]+)}nxx;
+my $RE_PROC_FD = qr{\A /proc/ (?<pid>[0-9]+) /}nxx;
 
 sub verify_nvme_nqn($value, $noerr = undef) {
 
@@ -155,6 +157,18 @@ sub _configured_portals($scfg) {
     return $portals;
 }
 
+sub _local_iface_exists($iface) {
+    return -d "/sys/class/net/$iface";
+}
+
+sub _validate_local_ifaces($portals) {
+    for my $portal ($portals->@*) {
+        my $iface = $portal->{host_iface};
+        die "NVMe/TCP host interface '$iface' does not exist on this node\n"
+            if !_local_iface_exists($iface);
+    }
+}
+
 PVE::JSONSchema::register_format('pve-storage-nvme-nqn', \&verify_nvme_nqn);
 PVE::JSONSchema::register_format('pve-storage-nvme-portals', \&verify_nvme_portals);
 PVE::JSONSchema::register_format('pve-storage-nvme-host-ifaces', \&verify_nvme_host_ifaces);
@@ -385,8 +399,19 @@ sub on_update_hook_full($class, $storeid, $scfg, $update, $delete, $sensitive) {
 }
 
 sub on_delete_hook($class, $storeid, $scfg) {
-    eval { $class->deactivate_storage($storeid, $scfg) };
-    log_warn("failed to disconnect NVMe storage '$storeid': $@") if $@;
+    # This hook runs only on the API node, while another cluster node can still
+    # be using the shared storage. Requiring an empty owned dataset makes the
+    # check cluster-wide without relying on remote process inspection.
+    my $volumes = $class->zfs_list_zvol($scfg);
+    my @volumes = sort keys $volumes->%*;
+    die "refusing to remove NVMe storage '$storeid': it still contains "
+        . join(', ', @volumes) . "\n"
+        if @volumes;
+
+    # Refuse the configuration removal while a VM or another process still has
+    # one of this subsystem's namespaces open. Otherwise deleting the storage
+    # could turn an administrative mistake into immediate guest I/O errors.
+    $class->deactivate_storage($storeid, $scfg);
     eval { PVE::Storage::LunCmd::NVMET::delete_target($scfg) };
     log_warn("failed to remove NVMe target for '$storeid': $@") if $@;
     delete_secret($storeid);
@@ -461,6 +486,57 @@ my sub controller_states($nqn) {
     return $states;
 }
 
+my sub namespace_devices($nqn) {
+    my $devices = {};
+
+    opendir(my $dh, '/sys/class/nvme-subsystem') or return $devices;
+    while (defined(my $entry = readdir($dh))) {
+        next if $entry !~ $RE_NVME_SUBSYSTEM;
+        my $base = "/sys/class/nvme-subsystem/$entry";
+        my $subsys = file_read_firstline("$base/subsysnqn");
+        next if !defined($subsys) || $subsys ne $nqn;
+
+        opendir(my $subsys_dh, $base) or next;
+        while (defined(my $device = readdir($subsys_dh))) {
+            next if $device !~ $RE_NVME_NAMESPACE;
+            $devices->{"/dev/$device"} = $device if -b "/dev/$device";
+        }
+        closedir($subsys_dh);
+    }
+    closedir($dh);
+    return $devices;
+}
+
+sub _namespace_openers($nqn) {
+    my $devices = namespace_devices($nqn);
+    return [] if !$devices->%*;
+
+    my %openers;
+    for my $fd (glob('/proc/[0-9]*/fd/[0-9]*')) {
+        my $target = readlink($fd);
+        next if !defined($target) || !exists($devices->{$target});
+        my $pid = $fd =~ $RE_PROC_FD ? $+{pid} : undef;
+        next if !defined($pid);
+        my $comm = eval { file_read_firstline("/proc/$pid/comm") } // 'unknown';
+        $openers{"$pid:$target"} = "$comm (PID $pid, $target)";
+    }
+
+    # Kernel consumers such as device-mapper do not necessarily keep a userspace
+    # file descriptor open, but expose their dependency in the holders directory.
+    for my $path (keys $devices->%*) {
+        my $device = $devices->{$path};
+        my $holders = "/sys/class/block/$device/holders";
+        opendir(my $holders_dh, $holders) or next;
+        while (defined(my $holder = readdir($holders_dh))) {
+            next if $holder eq '.' || $holder eq '..';
+            $openers{"holder:$device:$holder"} = "$path held by $holder";
+        }
+        closedir($holders_dh);
+    }
+
+    return [sort values %openers];
+}
+
 my sub portal_reachable($portal) {
     my $socket = IO::Socket::IP->new(
         PeerHost => $portal->{address},
@@ -538,8 +614,9 @@ sub activate_storage($class, $storeid, $scfg, $cache = undef) {
     die "native NVMe multipath is disabled in the running kernel\n"
         if (file_read_firstline('/sys/module/nvme_core/parameters/multipath') // 'N') ne 'Y';
 
-    _assert_unique_target($storeid, $scfg);
     my $portals = _configured_portals($scfg);
+    _validate_local_ifaces($portals);
+    _assert_unique_target($storeid, $scfg);
     my $states = controller_states($scfg->{subsysnqn});
     my $force_reconcile =
         delete($cache->{'zfsnvme-force-reconcile'}->{$storeid}) // 0;
@@ -637,6 +714,10 @@ sub activate_storage($class, $storeid, $scfg, $cache = undef) {
 }
 
 sub deactivate_storage($class, $storeid, $scfg, $cache = undef) {
+    my $openers = _namespace_openers($scfg->{subsysnqn});
+    die "refusing to disconnect NVMe storage '$storeid': namespace in use by "
+        . join(', ', $openers->@*) . "\n"
+        if $openers->@*;
 
     run_command(
         [$nvme, 'disconnect', '--nqn', $scfg->{subsysnqn}],
diff --git a/src/test/zfsnvme_test.pm b/src/test/zfsnvme_test.pm
index 3e9e04b..edb8b38 100644
--- a/src/test/zfsnvme_test.pm
+++ b/src/test/zfsnvme_test.pm
@@ -11,6 +11,8 @@ use PVE::Storage::ZFSPlugin;
 use Test::MockModule;
 use Test::More;
 
+my $nvme_mock = Test::MockModule->new('PVE::Storage::ZFSNVMePlugin');
+
 is(
     PVE::Storage::ZFSNVMePlugin::verify_nvme_nqn(
         'nqn.2014-08.org.nvmexpress:uuid:12345678-1234-1234-1234-123456789abc',
@@ -86,6 +88,16 @@ eval {
 };
 like($@, qr/one interface for each/, 'portal and host-interface counts must match');
 
+$nvme_mock->redefine(_local_iface_exists => sub { return $_[0] eq 'ens20' });
+eval {
+    PVE::Storage::ZFSNVMePlugin::_validate_local_ifaces([
+        { host_iface => 'ens20' },
+        { host_iface => 'ens21' },
+    ]);
+};
+like($@, qr/host interface 'ens21' does not exist/, 'missing local interface fails preflight');
+$nvme_mock->redefine(_local_iface_exists => sub { return 1 });
+
 eval {
     PVE::Storage::ZFSNVMePlugin::_assert_unique_target(
         'new-storage',
@@ -161,7 +173,6 @@ is(
 eval { PVE::Storage::ZFSNVMePlugin::_validate_secret('plaintext') };
 like($@, qr/invalid NVMe DH-HMAC-CHAP/, 'rejects a plaintext secret');
 
-my $nvme_mock = Test::MockModule->new('PVE::Storage::ZFSNVMePlugin');
 $nvme_mock->redefine(zfs_get_lu_name => sub { return '12345678-1234-1234-1234-123456789abc' });
 
 my $scfg = {};
@@ -262,6 +273,21 @@ is_deeply(
     'volume listing requires a local or received ownership property',
 );
 
+$nvme_mock->redefine(
+    zfs_list_zvol => sub { return { 'vm-100-disk-0' => 1, 'base-200-disk-0' => 1 } },
+);
+eval {
+    PVE::Storage::ZFSNVMePlugin->on_delete_hook(
+        'nvmetest',
+        { subsysnqn => 'nqn.2026-07.example:test' },
+    );
+};
+like(
+    $@,
+    qr/refusing to remove.*base-200-disk-0, vm-100-disk-0/s,
+    'storage removal requires an empty owned dataset on every cluster node',
+);
+
 eval {
     PVE::Storage::ZFSNVMePlugin->volume_resize(
         {}, 'nvmetest', 'vm-100-disk-0', 2 * 1024 * 1024 * 1024, 1, undef,
@@ -273,6 +299,22 @@ like(
     'online resize is rejected before mutating the backend',
 );
 
+$nvme_mock->redefine(
+    _namespace_openers => sub { return ['qemu-system-x86_64 (PID 123, /dev/nvme0n1)'] },
+);
+eval {
+    PVE::Storage::ZFSNVMePlugin->deactivate_storage(
+        'nvmetest',
+        { subsysnqn => 'nqn.2026-07.example:test' },
+    );
+};
+like(
+    $@,
+    qr/refusing to disconnect.*namespace in use by qemu-system-x86_64/s,
+    'storage deactivation refuses to remove a namespace opened by a VM',
+);
+$nvme_mock->redefine(_namespace_openers => sub { return [] });
+
 my $update_key = 'DHHC-1:01:dXBkYXRlLXRlc3Qta2V5:';
 $nvme_mock->redefine(file_read_firstline => sub { return $update_key });
 my $zfs_parent_mock = Test::MockModule->new('PVE::Storage::ZFSPlugin');
-- 
2.54.0.windows.1