linux-kernel - Re: [PATCH blktests v2 2/2] nvme-rc: Cleanup fc resource before module unloading

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <73gciykkvntmmmbsbz6yjec66ujhasmzj33zp2sv4qzezmfzyr@3xsdsqatdrv2>
Date:   Tue, 2 May 2023 06:13:22 +0000
From:   Shinichiro Kawasaki <shinichiro.kawasaki@....com>
To:     Sagi Grimberg <sagi@...mberg.me>
CC:     Daniel Wagner <dwagner@...e.de>,
        "linux-nvme@...ts.infradead.org" <linux-nvme@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        Chaitanya Kulkarni <kch@...dia.com>,
        Shin'ichiro Kawasaki <shinichiro@...tmail.com>
Subject: Re: [PATCH blktests v2 2/2] nvme-rc: Cleanup fc resource before
 module unloading

On May 01, 2023 / 17:10, Sagi Grimberg wrote:
> On 4/30/23 13:34, Shinichiro Kawasaki wrote:
[...]
> > Sagi, this comment above was not clear for me. Is Daniel's patch ok for you?
> > 
> > IMO, it is reasonable to "do clean-up in reverse order as setup" as a general
> > guide. It will reduce the chance to see module related failures when the test
> > cases do not expect such failures. Instead, we can have dedicated test cases for
> > the module load/unload order related failures. start_soft_rdma and
> > stop_soft_rdma do module load and unload. So I think the guide is good for those
> > helper functions also.
> 
> As I mentioned here, this change exercises a code path in the driver
> that is a surprise unplug of the rdma device. It is equivalent to
> triggering a surprise removal of the pci device normally during
> nvme-pci test teardown. While this is worth testing, I'm not sure we
> want the default behavior to do that, but rather add dedicated tests for
> it.
> 
> Hence, my suggestion was to leave nvme-rdma as is.

Thanks for the clarification. I assume that stop_soft_rdma is the "surprise
unplug of the rdma device". If I understand it correctly, the change for nvme-fc
will be like this:


diff --git a/tests/nvme/rc b/tests/nvme/rc
index ec0cc2d..24803af 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -260,6 +260,11 @@ _cleanup_nvmet() {
 	shopt -u nullglob
 	trap SIGINT
 
+	if [[ "${nvme_trtype}" == "fc" ]]; then
+		_cleanup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
+				"${def_remote_wwnn}" "${def_remote_wwpn}"
+		modprobe -rq nvme-fcloop
+	fi
 	modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null
 	if [[ "${nvme_trtype}" != "loop" ]]; then
 		modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null
@@ -268,10 +273,6 @@ _cleanup_nvmet() {
 	if [[ "${nvme_trtype}" == "rdma" ]]; then
 		stop_soft_rdma
 	fi
-	if [[ "${nvme_trtype}" == "fc" ]]; then
-		_cleanup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
-			        "${def_remote_wwnn}" "${def_remote_wwpn}"
-	fi
 }
 
 _setup_nvmet() {