[PATCH blktests v2 0/2] nvme: add test for unprivileged passthrough

[PATCH blktests v2 0/2] nvme: add test for unprivileged passthrough

[Shin'ichiro Kawasaki]

Per suggestion by Kanchan, add a new test case to test unprivileged passthrough
of NVME character devices. The first patch adds a feature to run commands with
normal user privilege. The second patch adds the test case using the feature.

Changes from v2:
* Added the first patch to add normal user privilege support to blktests
* Adjusted the test case to the functions for normal user privilege support

Kanchan Joshi (1):
  nvme/046: add test for unprivileged passthrough

Shin'ichiro Kawasaki (1):
  check, common/rc: support normal user privilege

 Documentation/running-tests.md | 10 +++++++
 check                          |  1 +
 common/rc                      | 13 +++++++++
 tests/nvme/046                 | 50 ++++++++++++++++++++++++++++++++++
 tests/nvme/046.out             |  2 ++
 5 files changed, 76 insertions(+)
 create mode 100644 tests/nvme/046
 create mode 100644 tests/nvme/046.out

-- 
2.38.1

[PATCH blktests v2 1/2] check, common/rc: support normal user privilege

[Shin'ichiro Kawasaki]

To run commands with normal user privilege, add a new config variable
NORMAL_USER and two helper functions _run_user and _require_normal_user.
The user name specified to NORMAL_USER is used to run the commands
specified to _run_user. The test cases which require NORMAL_USER shall
call _require_normal_user to ensure the NORMAL_USER is valid.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 Documentation/running-tests.md | 10 ++++++++++
 check                          |  1 +
 common/rc                      | 13 +++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md
index 586be0b..3550f37 100644
--- a/Documentation/running-tests.md
+++ b/Documentation/running-tests.md
@@ -113,6 +113,16 @@ use_siw=1 ./check nvmeof-mp/
 use_siw=1 ./check srp/
 ```
 
+### Normal user
+
+To run test cases which require normal user privilege, prepare a user and
+specify it to the `NORMAL_USER` variable. The test cases are skipped unless a
+valid user is specified.
+
+```sh
+NORMAL_USER=blktests_user
+```
+
 ### Custom Setup
 
 The `config` file is really just a bash file that is sourced at the beginning
diff --git a/check b/check
index 34e96c4..8eaf5c6 100755
--- a/check
+++ b/check
@@ -799,6 +799,7 @@ fi
 
 : "${LOGGER_PROG:="$(type -P logger || echo true)"}"
 : "${RUN_ZONED_TESTS:=0}"
+: "${NORMAL_USER:=''}"
 
 # Sanity check options.
 if [[ $QUICK_RUN -ne 0 && ! "${TIMEOUT:-}" ]]; then
diff --git a/common/rc b/common/rc
index ef23ebe..af4c0b1 100644
--- a/common/rc
+++ b/common/rc
@@ -381,6 +381,14 @@ _require_test_dev_is_partition() {
 	return 0
 }
 
+_require_normal_user() {
+	if ! id "$NORMAL_USER" >/dev/null 2>&1; then
+		SKIP_REASONS+=("valid NORMAL_USER is not specfied")
+		return 1
+	fi
+	return 0
+}
+
 # Prints a space-separated list with the names of all I/O schedulers supported
 # by block device $1.
 _io_schedulers() {
@@ -409,3 +417,8 @@ _have_writeable_kmsg() {
 	fi
 	return 0
 }
+
+# Run the given command as NORMAL_USER
+_run_user() {
+	su "$NORMAL_USER" -c "$1"
+}
-- 
2.38.1

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

[Shin'ichiro Kawasaki]

From: Kanchan Joshi <joshi.k@samsung.com>

Alters permissions for char-device node (/dev/ngX) and runs few
passthrough commands as a normal user to exercise nvme_cmd_allowed().

Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
[Shin'ichiro: adjusted to normal user helper functions]
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/nvme/046     | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/046.out |  2 ++
 2 files changed, 52 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..5689a2b
--- /dev/null
+++ b/tests/nvme/046
@@ -0,0 +1,50 @@
+#!/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
+	_require_normal_user
+}
+
+test_device() {
+	echo "Running ${TEST_NAME}"
+	local ngdev=${TEST_DEV/nvme/ng}
+	local perm nsid
+
+	perm="$(stat -c "%a" "$ngdev")"
+	nsid="$(_test_dev_nvme_nsid)"
+
+	chmod g+r,o+r "$ngdev"
+
+	if ! _run_user "nvme io-passthru ${ngdev} -o 2 -l 4096 \
+		-n $nsid -r" >> "${FULL}" 2>&1; then
+		echo "Error: io-passthru read failed"
+	fi
+
+	if _run_user "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 ! _run_user "nvme id-ns ${ngdev}" >> "${FULL}" 2>&1; then
+		echo "Error: id-ns failed"
+	fi
+
+	if ! _run_user "nvme id-ctrl ${ngdev}" >> "${FULL}" 2>&1; then
+		echo "Error: id-ctrl failed"
+	fi
+
+	if _run_user "nvme ns-descs ${ngdev}" >> "${FULL}" 2>&1; then
+		echo "Error: ns-descs passed (unexpected)"
+	fi
+
+	echo "Test complete"
+	chmod "$perm" "$ngdev"
+}
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.38.1

Re: [PATCH blktests v2 0/2] nvme: add test for unprivileged passthrough

[Kanchan Joshi]

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

On Tue, Feb 14, 2023 at 01:47:37PM +0900, Shin'ichiro Kawasaki wrote:
>Per suggestion by Kanchan, add a new test case to test unprivileged passthrough
>of NVME character devices. The first patch adds a feature to run commands with
>normal user privilege. The second patch adds the test case using the feature.
>
>Changes from v2:
>* Added the first patch to add normal user privilege support to blktests
>* Adjusted the test case to the functions for normal user privilege support

Thanks, this looks way better. And works fine in my setup.
If required,
Tested-by: Kanchan Joshi <joshi.k@samsung.com>

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

Re: [PATCH blktests v2 0/2] nvme: add test for unprivileged passthrough

[Shinichiro Kawasaki]

On Feb 27, 2023 / 11:35, Kanchan Joshi wrote:
> On Tue, Feb 14, 2023 at 01:47:37PM +0900, Shin'ichiro Kawasaki wrote:
> > Per suggestion by Kanchan, add a new test case to test unprivileged passthrough
> > of NVME character devices. The first patch adds a feature to run commands with
> > normal user privilege. The second patch adds the test case using the feature.
> > 
> > Changes from v2:
> > * Added the first patch to add normal user privilege support to blktests
> > * Adjusted the test case to the functions for normal user privilege support
> 
> Thanks, this looks way better. And works fine in my setup.
> If required,
> Tested-by: Kanchan Joshi <joshi.k@samsung.com>

Thanks for the confirmation. Sounds good.

I found two more minor points to improve:

1) tests/nvme/046 does not have executable mode bit. I will add it when I apply
   the patch.

2) I ran the test case with kernel version v6.1 and it failed. Does the test
   case require kernel version 6.2 or higher? If that is the case, one more line
   change will be required as follows. If you are ok with the change, I can fold
   this change in when I apply the patches.

diff --git a/tests/nvme/046 b/tests/nvme/046
index 5689a2b..b37b9e9 100644
--- a/tests/nvme/046
+++ b/tests/nvme/046
@@ -11,6 +11,7 @@ QUICK=1
 requires() {
 	_nvme_requires
 	_require_normal_user
+	_have_kver 6 2
 }
 
 test_device() {


-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests v2 0/2] nvme: add test for unprivileged passthrough

[Kanchan Joshi]

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

On Mon, Feb 27, 2023 at 11:24:04AM +0000, Shinichiro Kawasaki wrote:
>On Feb 27, 2023 / 11:35, Kanchan Joshi wrote:
>> On Tue, Feb 14, 2023 at 01:47:37PM +0900, Shin'ichiro Kawasaki wrote:
>> > Per suggestion by Kanchan, add a new test case to test unprivileged passthrough
>> > of NVME character devices. The first patch adds a feature to run commands with
>> > normal user privilege. The second patch adds the test case using the feature.
>> >
>> > Changes from v2:
>> > * Added the first patch to add normal user privilege support to blktests
>> > * Adjusted the test case to the functions for normal user privilege support
>>
>> Thanks, this looks way better. And works fine in my setup.
>> If required,
>> Tested-by: Kanchan Joshi <joshi.k@samsung.com>
>
>Thanks for the confirmation. Sounds good.
>
>I found two more minor points to improve:
>
>1) tests/nvme/046 does not have executable mode bit. I will add it when I apply
>   the patch.
>
>2) I ran the test case with kernel version v6.1 and it failed. Does the test
>   case require kernel version 6.2 or higher? If that is the case, one more line
>   change will be required as follows. If you are ok with the change, I can fold
>   this change in when I apply the patches.

Yes, unprivileged passthrough exists from 6.2. Changes looks good.
Thanks.

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

Re: [PATCH blktests v2 0/2] nvme: add test for unprivileged passthrough

[Shinichiro Kawasaki]

On Feb 27, 2023 / 19:48, Kanchan Joshi wrote:
> On Mon, Feb 27, 2023 at 11:24:04AM +0000, Shinichiro Kawasaki wrote:
> > On Feb 27, 2023 / 11:35, Kanchan Joshi wrote:
> > > On Tue, Feb 14, 2023 at 01:47:37PM +0900, Shin'ichiro Kawasaki wrote:
> > > > Per suggestion by Kanchan, add a new test case to test unprivileged passthrough
> > > > of NVME character devices. The first patch adds a feature to run commands with
> > > > normal user privilege. The second patch adds the test case using the feature.
> > > >
> > > > Changes from v2:
> > > > * Added the first patch to add normal user privilege support to blktests
> > > > * Adjusted the test case to the functions for normal user privilege support
> > > 
> > > Thanks, this looks way better. And works fine in my setup.
> > > If required,
> > > Tested-by: Kanchan Joshi <joshi.k@samsung.com>
> > 
> > Thanks for the confirmation. Sounds good.
> > 
> > I found two more minor points to improve:
> > 
> > 1) tests/nvme/046 does not have executable mode bit. I will add it when I apply
> >   the patch.
> > 
> > 2) I ran the test case with kernel version v6.1 and it failed. Does the test
> >   case require kernel version 6.2 or higher? If that is the case, one more line
> >   change will be required as follows. If you are ok with the change, I can fold
> >   this change in when I apply the patches.
> 
> Yes, unprivileged passthrough exists from 6.2. Changes looks good.
> Thanks.

All right, I've applied the patches. Thanks!

-- 
Shin'ichiro Kawasaki