[PATCH blktests] nvme/046: add test for unprivileged passthrough

[PATCH blktests] nvme/046: add test for unprivileged passthrough

[Kanchan Joshi]

Ths creates a non-root user "blktest46", alters permissions for
char-device node (/dev/ngX) and runs few passthrough commands.
At the end of the test, user is deleted and permissions are reverted.

Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
---
 tests/nvme/046     | 55 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/046.out |  2 ++
 2 files changed, 57 insertions(+)
 create mode 100644 tests/nvme/046
 create mode 100644 tests/nvme/046.out

diff --git a/tests/nvme/046 b/tests/nvme/046
new file mode 100644
index 0000000..40bda62
--- /dev/null
+++ b/tests/nvme/046
@@ -0,0 +1,55 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2023 Kanchan Joshi, Samsung Electronics
+# Test for unprivileged passthrough
+
+. tests/nvme/rc
+
+DESCRIPTION="basic test for unprivileged passthrough on /dev/ngX"
+QUICK=1
+
+requires() {
+	_nvme_requires
+	_have_fio
+}
+
+device_requires() {
+	_require_test_dev_is_nvme
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+	local ngdev=${TEST_DEV/nvme/ng}
+	local usr="blktest46"
+	local perm=$(stat -c "%a" $ngdev)
+	local nsid=$(_test_dev_nvme_nsid)
+
+	useradd -m $usr
+	chmod g+r,o+r "$ngdev"
+
+	if ! su $usr -c "nvme io-passthru ${ngdev} -o 2 -l 4096 \
+		-n $nsid -r" >> "${FULL}" 2>&1; then
+		echo "Error: io-passthru read failed"
+	fi
+
+	if su $usr -c "echo hello | nvme io-passthru ${ngdev} -o 1 -l 4096 \
+		-n $nsid -r" >> "${FULL}" 2>&1; then
+		echo "Error: io-passthru write passed (unexpected)"
+	fi
+
+	if ! su $usr -c "nvme id-ns ${ngdev}" >> "${FULL}" 2>&1; then
+		echo "Error: id-ns failed"
+	fi
+
+	if ! su $usr -c "nvme id-ctrl ${ngdev}" >> "${FULL}" 2>&1; then
+		echo "Error: id-ctrl failed"
+	fi
+
+	if su $usr -c "nvme ns-descs ${ngdev}" >> "${FULL}" 2>&1; then
+		echo "Error: ns-descs passed (unexpected)"
+	fi
+
+	echo "Test complete"
+	chmod  $perm "$ngdev"
+	userdel -r  $usr >> "${FULL}" 2>&1
+}
diff --git a/tests/nvme/046.out b/tests/nvme/046.out
new file mode 100644
index 0000000..2b5fa6a
--- /dev/null
+++ b/tests/nvme/046.out
@@ -0,0 +1,2 @@
+Running nvme/046
+Test complete
-- 
2.25.1

Re: [PATCH blktests] nvme/046: add test for unprivileged passthrough

[kernel test robot]

Hi Kanchan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hch-configfs/for-next]
[also build test WARNING on linus/master v6.2-rc7 next-20230209]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kanchan-Joshi/nvme-046-add-test-for-unprivileged-passthrough/20230209-174831
base:   git://git.infradead.org/users/hch/configfs.git for-next
patch link:    https://lore.kernel.org/r/20230209094541.248729-1-joshi.k%40samsung.com
patch subject: [PATCH blktests] nvme/046: add test for unprivileged passthrough
reproduce:
        scripts/spdxcheck.py

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302100053.tumPI2Xy-lkp@intel.com/

spdxcheck warnings: (new ones prefixed by >>)
   drivers/cpufreq/amd-pstate-ut.c: 1:28 Invalid License ID: GPL-1.0-or-later
>> tests/nvme/046: 2:27 Invalid License ID: GPL-3.0+

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH blktests] nvme/046: add test for unprivileged passthrough

[Shinichiro Kawasaki]

On Feb 09, 2023 / 15:15, Kanchan Joshi wrote:
> Ths creates a non-root user "blktest46", alters permissions for
> char-device node (/dev/ngX) and runs few passthrough commands.
> At the end of the test, user is deleted and permissions are reverted.
> 
> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>

Thanks for the patch. I guess this test case exercises nvme_cmd_allowed() in
drivers/nvme/host/ioctl.c. The test contents look valid and good.

This test case adds and deletes a user. For every test case run, it creates and
removes the user home directory and touches /etc files. It does not sound right
for me. It changes system set up, and sudden test case stop will leave the user.

I suggest to ask blktests users to prepare the normal user and specify it to a
config file variable (it can be named NORMAL_USER or something). I also suggest
to add two new helper functions: _require_user() will check that the specified
user is valid, and _run_user() will wrap the "su $NORMAL_USER -c" command line.

If you don't mind, I can create another patch for further discussion based on
the suggestion above, and modify your patch to use the new helper functions.

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests] nvme/046: add test for unprivileged passthrough

[Kanchan Joshi]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

On Fri, Feb 10, 2023 at 02:01:14AM +0000, Shinichiro Kawasaki wrote:
>On Feb 09, 2023 / 15:15, Kanchan Joshi wrote:
>> Ths creates a non-root user "blktest46", alters permissions for
>> char-device node (/dev/ngX) and runs few passthrough commands.
>> At the end of the test, user is deleted and permissions are reverted.
>>
>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>
>Thanks for the patch. I guess this test case exercises nvme_cmd_allowed() in
>drivers/nvme/host/ioctl.c.

Yes. Thanks for review.
>The test contents look valid and good.
>
>This test case adds and deletes a user. For every test case run, it creates and
>removes the user home directory and touches /etc files. It does not sound right
>for me. It changes system set up, and sudden test case stop will leave the user.
>
>I suggest to ask blktests users to prepare the normal user and specify it to a
>config file variable (it can be named NORMAL_USER or something). I also suggest
>to add two new helper functions: _require_user() will check that the specified
>user is valid, and _run_user() will wrap the "su $NORMAL_USER -c" command line.

I was trying to make this automatic for blktests users.
Script attempts cleanup always (regardless of test-failure).
But yes, if any command gets stuck, cleanup won't happen.
So what you mentioned - sounds fine to me.

>If you don't mind, I can create another patch for further discussion based on
>the suggestion above, and modify your patch to use the new helper functions.
Sure. Please remove "_have_fio" line also in v2.

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH blktests] nvme/046: add test for unprivileged passthrough

[Shinichiro Kawasaki]

On Feb 10, 2023 / 16:42, Kanchan Joshi wrote:
> On Fri, Feb 10, 2023 at 02:01:14AM +0000, Shinichiro Kawasaki wrote:
[...]
> > If you don't mind, I can create another patch for further discussion based on
> > the suggestion above, and modify your patch to use the new helper functions.
> Sure. Please remove "_have_fio" line also in v2.

Okay. I've sent out v2 [1]. Please check and confirm that it works for you.

FYI, I did a couple of more nit edits:

- No need to call _require_test_dev_is_nvme, since it is called from
  group_device_requires in tests/nvme/rc.
- Separated local variable declarations and initialization. ('make check'
  reported shellcheck complaints about it.)

[1] https://lore.kernel.org/linux-block/20230214044739.1903364-1-shinichiro.kawasaki@wdc.com/

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests] nvme/046: add test for unprivileged passthrough

[Kanchan Joshi]

On Tue, Feb 14, 2023 at 10:32 AM Shinichiro Kawasaki
<shinichiro.kawasaki@wdc.com> wrote:
>
> On Feb 10, 2023 / 16:42, Kanchan Joshi wrote:
> > On Fri, Feb 10, 2023 at 02:01:14AM +0000, Shinichiro Kawasaki wrote:
> [...]
> > > If you don't mind, I can create another patch for further discussion based on
> > > the suggestion above, and modify your patch to use the new helper functions.
> > Sure. Please remove "_have_fio" line also in v2.
>
> Okay. I've sent out v2 [1]. Please check and confirm that it works for you.

Thanks. I am PTO for a few days; will come to it next week.