Re: [PATCH v3] nvme/069: add a test for multipath cdev lifetime

[Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

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"
> +}