| Message ID | 20230209094541.248729-1-joshi.k@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [blktests] nvme/046: add test for unprivileged passthrough | expand |
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+
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.
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.
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/
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.
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
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