Linux block layer
 help / color / mirror / Atom feed
* [PATCH v3] nvme/069: add a test for multipath cdev lifetime
@ 2026-07-15 15:55 John Garry
  2026-07-15 16:21 ` Daniel Wagner
  2026-07-16  6:48 ` Shin'ichiro Kawasaki
  0 siblings, 2 replies; 4+ messages in thread
From: John Garry @ 2026-07-15 15:55 UTC (permalink / raw)
  To: shinichiro.kawasaki, linux-block
  Cc: hch, linux-nvme, dwagner, John Garry, John Garry

From: John Garry <john.garry@linux.dev>

In [0], a fix was proposed for the NS and NS head (multipath) cdev
lifetime.

The issue was that fds for the nvme-generic cdev may exist after we tear
down the nvme-subsystem. Issuing an ioctl on that cdev may expose a
use-after-free.

This test recreates the method described in [0] to expose this issue
for NS head cdev.

First a fd is created by opening the nvme-generic cdev. Next we tear down
the nvme-subsystem. Finally we try to issue an ioctl on the cdev - without
the kernel fix in [0], this should trigger a KASAN warn.

[0] https://lore.kernel.org/linux-nvme/20260713104238.3034640-1-john.g.garry@oracle.com/T/#me118851584fbcbb960795d0f04e2262e5a295613

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
Difference to v2:
- remove sleep and use signal to sync

Difference to v1:
- fix script formatting issues

diff --git a/common/nvme b/common/nvme
index 15e9c3f..902a33f 100644
--- a/common/nvme
+++ b/common/nvme
@@ -300,6 +300,26 @@ _nvme_wait_subsys_removed() {
 	done
 }
 
+_nvme_find_subsys_nvme_generic() {
+	local subsysnqn="$def_subsysnqn"
+
+	for subsyspath in /sys/class/nvme-subsystem/*; do
+		_subsysnqn=$(cat "${subsyspath}/subsysnqn" 2> /dev/null)
+		if [ "$subsysnqn" == "$_subsysnqn" ]; then
+			subsyspathbase="$(basename "$subsyspath")"
+			for nvmegeneric in /sys/class/nvme-generic/*; do
+				nvmegenericbase="$(basename "$nvmegeneric")"
+				devicelink="$(readlink "$nvmegeneric/device")"
+				if [[ "$devicelink" =~ $subsyspathbase ]]; then
+					echo "$nvmegenericbase"
+					break
+				fi
+			done
+			break
+		fi
+	done
+}
+
 _nvme_connect_subsys() {
 	local subsysnqn="$def_subsysnqn"
 	local hostnqn="$def_hostnqn"
diff --git a/src/.gitignore b/src/.gitignore
index 988d785..af9ebf6 100644
--- a/src/.gitignore
+++ b/src/.gitignore
@@ -14,3 +14,4 @@
 /nvme-passthrough-meta
 /ioctl-lbmd-query
 /nvme-passthru-admin-uring
+/nvme-delay-ioctl
diff --git a/src/Makefile b/src/Makefile
index d75daf1..4779ffd 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -18,6 +18,7 @@ C_TARGETS := \
 	loop_change_fd \
 	loop_get_status_null \
 	mount_clear_sock \
+	nvme-delay-ioctl \
 	nvme-passthrough-meta \
 	ioctl-lbmd-query \
 	nbdsetsize \
diff --git a/src/nvme-delay-ioctl.c b/src/nvme-delay-ioctl.c
new file mode 100644
index 0000000..854b702
--- /dev/null
+++ b/src/nvme-delay-ioctl.c
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-3.0+
+
+/*
+ * Simple test to issue an ioctl some time after opening a cdev.
+ * This is to test lifetime of cdev, and whether we can handle
+ * removing the cdev while we have active fds.
+ */
+#define _GNU_SOURCE
+#include <dirent.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <signal.h>
+
+#include <inttypes.h>
+#include <sys/ioctl.h>
+#include <linux/types.h>
+
+#ifndef _LINUX_NVME_IOCTL_H
+#define _LINUX_NVME_IOCTL_H
+#define NVME_IOCTL_ID		_IO('N', 0x40)
+#endif /* _UAPI_LINUX_NVME_IOCTL_H */
+
+int main(int argc, char **argv)
+{
+	int fd, fd1;
+	int count;
+
+	if (argc < 2) {
+		fprintf(stderr, "usage: %s /dev/ngXnX", argv[0]);
+		return EINVAL;
+	}
+
+	fd = open(argv[1], O_RDONLY);
+	if (fd < 0)
+		return fd;
+
+	/*
+	 * Steps:
+	 * a. Signal to parent that we have opened the file so that it may
+	 *    start the teardown.
+	 * b. Try to open device until disallowed/gone.
+	 * c. Sleep to allow nvme-subsystem be torn down.
+	 * d. Issue the ioctl on original fd.
+	 */
+	kill(getppid(), SIGUSR2);
+	count = 0;
+	for (;;) {
+		fd1 = open(argv[1], O_RDONLY);
+		usleep(500000);
+		if (fd1 < 0)
+			break;
+		close(fd1);
+		count++;
+		if (count > 10) {
+			fprintf(stderr, "%s still present", argv[0]);
+			return EINVAL;
+		}
+	}
+
+	ioctl(fd, NVME_IOCTL_ID);
+	close(fd);
+
+	return 0;
+}
diff --git a/tests/nvme/069 b/tests/nvme/069
new file mode 100755
index 0000000..89f048c
--- /dev/null
+++ b/tests/nvme/069
@@ -0,0 +1,61 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2026 John Garry
+#
+# Test NVMe multipath nvme-generic cdev lifetime behaves as expected.
+
+. tests/nvme/rc
+
+DESCRIPTION="NVMe multipath cdev lifetime test"
+CHECK_DMESG=1
+
+requires() {
+	_nvme_requires
+	_have_loop
+	_have_module_param_value nvme_core multipath Y
+	_require_nvme_trtype_is_fabrics
+	_have_kernel_options KASAN
+}
+
+set_conditions() {
+	_set_nvme_trtype "$@"
+}
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	_setup_nvmet
+
+	local nvmedev
+	local loops=0
+	local quit=0
+	_nvmet_target_setup
+
+	_nvme_connect_subsys
+
+	nvmedev=$(_find_nvme_dev "${def_subsysnqn}")
+
+	nvmegeneric=$(_nvme_find_subsys_nvme_generic)
+
+	trap 'quit=1' USR2
+	# nvme-delay-ioctl will open the nvme-generic cdev, wait for teardown,
+	# and then issue an ioctl.
+	# We tear down the nvme-subsystem in the foreground so we can catch
+	# if the ioctl triggers a use-after-free KASAN warn.
+	src/nvme-delay-ioctl "/dev/$nvmegeneric" &
+
+	while [ "$quit" -ne 1 ]; do
+		sleep 0.1
+		((loops++))
+		if [[ "$loops" == "100" ]]; then
+			echo "no signal"
+			break
+		fi
+	done
+
+	_nvme_disconnect_ctrl "${nvmedev}"
+	_nvmet_target_cleanup
+	wait < <(jobs -p)
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/069.out b/tests/nvme/069.out
new file mode 100644
index 0000000..cd2e1fd
--- /dev/null
+++ b/tests/nvme/069.out
@@ -0,0 +1,2 @@
+Running nvme/069
+Test complete
-- 
2.43.7


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] nvme/069: add a test for multipath cdev lifetime
  2026-07-15 15:55 [PATCH v3] nvme/069: add a test for multipath cdev lifetime John Garry
@ 2026-07-15 16:21 ` Daniel Wagner
  2026-07-16  6:48 ` Shin'ichiro Kawasaki
  1 sibling, 0 replies; 4+ messages in thread
From: Daniel Wagner @ 2026-07-15 16:21 UTC (permalink / raw)
  To: John Garry; +Cc: shinichiro.kawasaki, linux-block, hch, linux-nvme, John Garry

On Wed, Jul 15, 2026 at 03:55:31PM +0000, John Garry wrote:
> From: John Garry <john.garry@linux.dev>
> 
> In [0], a fix was proposed for the NS and NS head (multipath) cdev
> lifetime.
> 
> The issue was that fds for the nvme-generic cdev may exist after we tear
> down the nvme-subsystem. Issuing an ioctl on that cdev may expose a
> use-after-free.
> 
> This test recreates the method described in [0] to expose this issue
> for NS head cdev.
> 
> First a fd is created by opening the nvme-generic cdev. Next we tear down
> the nvme-subsystem. Finally we try to issue an ioctl on the cdev - without
> the kernel fix in [0], this should trigger a KASAN warn.
> 
> [0] https://lore.kernel.org/linux-nvme/20260713104238.3034640-1-john.g.garry@oracle.com/T/#me118851584fbcbb960795d0f04e2262e5a295613
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>

Looks good to me! Thanks!

Reviewed-by: Daniel Wagner <dwagner@suse.de>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] nvme/069: add a test for multipath cdev lifetime
  2026-07-15 15:55 [PATCH v3] nvme/069: add a test for multipath cdev lifetime John Garry
  2026-07-15 16:21 ` Daniel Wagner
@ 2026-07-16  6:48 ` Shin'ichiro Kawasaki
  2026-07-16  7:25   ` John Garry
  1 sibling, 1 reply; 4+ messages in thread
From: Shin'ichiro Kawasaki @ 2026-07-16  6:48 UTC (permalink / raw)
  To: John Garry; +Cc: linux-block, hch, linux-nvme, dwagner, John Garry

On Jul 15, 2026 / 15:55, John Garry wrote:
> From: John Garry <john.garry@linux.dev>
> 
> In [0], a fix was proposed for the NS and NS head (multipath) cdev
> lifetime.
> 
> The issue was that fds for the nvme-generic cdev may exist after we tear
> down the nvme-subsystem. Issuing an ioctl on that cdev may expose a
> use-after-free.
> 
> This test recreates the method described in [0] to expose this issue
> for NS head cdev.
> 
> First a fd is created by opening the nvme-generic cdev. Next we tear down
> the nvme-subsystem. Finally we try to issue an ioctl on the cdev - without
> the kernel fix in [0], this should trigger a KASAN warn.
> 
> [0] https://lore.kernel.org/linux-nvme/20260713104238.3034640-1-john.g.garry@oracle.com/T/#me118851584fbcbb960795d0f04e2262e5a295613
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>

Hi John, thank you for the patch. I ran the test case and confirmed that it
triggers the KASAN. I also confirmed the kernel patch [0] avoids the KASAN.
Looks good from test run point of view.

Please find some review comments in-line.

...

> diff --git a/common/nvme b/common/nvme
> index 15e9c3f..902a33f 100644
> --- a/common/nvme
> +++ b/common/nvme
> @@ -300,6 +300,26 @@ _nvme_wait_subsys_removed() {
>  	done
>  }
>  
> +_nvme_find_subsys_nvme_generic() {
> +	local subsysnqn="$def_subsysnqn"

Nit: though it is not strictly required, I suggest to declare local variables.

	local subsyspath _subsysnqn subsyspathbase nvmegenericbase devicelink

> +
> +	for subsyspath in /sys/class/nvme-subsystem/*; do
> +		_subsysnqn=$(cat "${subsyspath}/subsysnqn" 2> /dev/null)
> +		if [ "$subsysnqn" == "$_subsysnqn" ]; then
> +			subsyspathbase="$(basename "$subsyspath")"
> +			for nvmegeneric in /sys/class/nvme-generic/*; do
> +				nvmegenericbase="$(basename "$nvmegeneric")"
> +				devicelink="$(readlink "$nvmegeneric/device")"
> +				if [[ "$devicelink" =~ $subsyspathbase ]]; then
> +					echo "$nvmegenericbase"
> +					break
> +				fi
> +			done
> +			break
> +		fi
> +	done
> +}
> +
>  _nvme_connect_subsys() {
>  	local subsysnqn="$def_subsysnqn"
>  	local hostnqn="$def_hostnqn"

...

> diff --git a/src/nvme-delay-ioctl.c b/src/nvme-delay-ioctl.c
> new file mode 100644
> index 0000000..854b702
> --- /dev/null
> +++ b/src/nvme-delay-ioctl.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-3.0+

Copyright is missing here.

...

> diff --git a/tests/nvme/069 b/tests/nvme/069
> new file mode 100755
> index 0000000..89f048c
> --- /dev/null
> +++ b/tests/nvme/069
> @@ -0,0 +1,61 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2026 John Garry
> +#
> +# Test NVMe multipath nvme-generic cdev lifetime behaves as expected.
> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="NVMe multipath cdev lifetime test"
> +CHECK_DMESG=1

This test case completes within a few seconds, so I suggest,

QUICK=1

> +
> +requires() {
> +	_nvme_requires
> +	_have_loop
> +	_have_module_param_value nvme_core multipath Y
> +	_require_nvme_trtype_is_fabrics
> +	_have_kernel_options KASAN
> +}
> +
> +set_conditions() {
> +	_set_nvme_trtype "$@"
> +}
> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	_setup_nvmet
> +
> +	local nvmedev

Nit: I suggest to declare nvmegeneric also:

	local nvmedev nvmegeneric

> +	local loops=0
> +	local quit=0
> +	_nvmet_target_setup
> +
> +	_nvme_connect_subsys
> +
> +	nvmedev=$(_find_nvme_dev "${def_subsysnqn}")
> +
> +	nvmegeneric=$(_nvme_find_subsys_nvme_generic)
> +
> +	trap 'quit=1' USR2
> +	# nvme-delay-ioctl will open the nvme-generic cdev, wait for teardown,
> +	# and then issue an ioctl.
> +	# We tear down the nvme-subsystem in the foreground so we can catch
> +	# if the ioctl triggers a use-after-free KASAN warn.
> +	src/nvme-delay-ioctl "/dev/$nvmegeneric" &
> +
> +	while [ "$quit" -ne 1 ]; do
> +		sleep 0.1
> +		((loops++))
> +		if [[ "$loops" == "100" ]]; then
> +			echo "no signal"
> +			break
> +		fi
> +	done
> +
> +	_nvme_disconnect_ctrl "${nvmedev}"
> +	_nvmet_target_cleanup
> +	wait < <(jobs -p)

My understanding is that wait command does not receive pids from stdin. Then I
think "jobs -p" output is ignored here. I guess you meant,

	wait $(jobs -p)

> +
> +	echo "Test complete"
> +}

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] nvme/069: add a test for multipath cdev lifetime
  2026-07-16  6:48 ` Shin'ichiro Kawasaki
@ 2026-07-16  7:25   ` John Garry
  0 siblings, 0 replies; 4+ messages in thread
From: John Garry @ 2026-07-16  7:25 UTC (permalink / raw)
  To: Shin'ichiro Kawasaki
  Cc: linux-block, hch, linux-nvme, dwagner, John Garry

On 16/07/2026 07:48, Shin'ichiro Kawasaki wrote:
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
> 
> Hi John, thank you for the patch. I ran the test case and confirmed that it
> triggers the KASAN. I also confirmed the kernel patch [0] avoids the KASAN.
> Looks good from test run point of view.
> 
> Please find some review comments in-line.

Thanks for verifying. I'll address your comments.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-07-16  7:25 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-15 15:55 [PATCH v3] nvme/069: add a test for multipath cdev lifetime John Garry
2026-07-15 16:21 ` Daniel Wagner
2026-07-16  6:48 ` Shin'ichiro Kawasaki
2026-07-16  7:25   ` John Garry

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox