On Apr 07, 2023 / 17:07, Shin'ichiro Kawasaki wrote:
> Thanks for the patch. I think it's good to have this test case to cover the
> uring-passthrough codes in the nvme driver code. Please find my comments in
> line.
>
> Also, I ran the new test case on my Fedora system using QEMU NVME device and
> found the test case fails with errors like,
>
> fio: io_u error on file /dev/ng0n1: Permission denied: read offset=266240, buflen=4096
>
> I took a look in this and learned that SELinux on my system does not allow
> IORING_OP_URING_CMD by default. I needed to do "setenforce 0" or add a local
> policy to allow IORING_OP_URING_CMD so that the test case passes.
>
> I think this test case should check this security requirement. I'm not sure what
> is the best way to do it. One idea is to just run fio with io_uring_cmd engine
> and check its error message. I created a patch below, and it looks working on my
> system. I suggest to add it, unless anyone knows other better way.
>
> diff --git a/tests/nvme/047 b/tests/nvme/047
> index a0cc8b2..30961ff 100755
> --- a/tests/nvme/047
> +++ b/tests/nvme/047
> @@ -14,6 +14,22 @@ requires() {
> _have_fio_ver 3 33
> }
>
> +device_requires() {
> + local ngdev=${TEST_DEV/nvme/ng}
> + local fio_output
> +
> + if fio_output=$(fio --name=check --size=4k --filename="$ngdev" \
> + --rw=read --ioengine=io_uring_cmd 2>&1); then
> + return 0
> + fi
> + if grep -qe "Permission denied" <<< "$fio_output"; then
> + SKIP_REASONS+=("IORING_OP_URING_CMD is not allowed for $ngdev")
> + else
> + SKIP_REASONS+=("IORING_OP_URING_CMD check for $ngdev failed")
I revisited the code I suggested and noticed this part is not good. The failures
other than "Permission denied" are the failures that this test case intended to
catch. It is not good to skip them.
> + fi
> + return 1
> +}
> +
> test_device() {
> echo "Running ${TEST_NAME}"
> local ngdev=${TEST_DEV/nvme/ng}
As far as I read the kernel code related to security_uring_cmd(), calling uring
command is the only way to check its security permission. It means that we can
not separate the check for the security permission and the uring command test
itself. So it's the better to check the security permission not in
device_requires() but in test_device(), as follows:
diff --git a/tests/nvme/047 b/tests/nvme/047
index a0cc8b2..741ccd9 100755
--- a/tests/nvme/047
+++ b/tests/nvme/047
@@ -30,6 +30,16 @@ test_device() {
--time_based
--runtime=2
)
+ local fio_output
+
+ # check security permission
+ if ! fio_output=$(fio --name=check --size=4k --filename="$ngdev" \
+ --rw=read --ioengine=io_uring_cmd 2>&1) &&
+ grep -qe "Permission denied" <<< "$fio_output"; then
+ SKIP_REASONS+=("IORING_OP_URING_CMD is not allowed for $ngdev")
+ return
+ fi
+
#plain read test
_run_fio "${common_args[@]}"
