Re: [PATCH v7 10/11] KVM: selftests: Test disabling NX hugepages on a VM

Sean Christopherson
2022-05-05T19:14:57Z

On Tue, May 03, 2022, Ben Gardon wrote:
> + if (disable_nx_huge_pages) {
> + /*
> + * Cannot run the test without NX huge pages if the kernel
> + * does not support it.
> + */
> + if (!kvm_check_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES))
> + return;
> +
> + r = __vm_disable_nx_huge_pages(vm);
> + if (reboot_permissions) {
> + TEST_ASSERT(!r, "Disabling NX huge pages should succeed if process has reboot permissions");
> + } else {
> + TEST_ASSERT(r == -EPERM, "This process should not have permission to disable NX huge pages");

This is wrong, the return value on ioctl() failure is -1, the error code is
in errno and it's a positive value.

LOL, but it passes because EPERM == 1, hilarious. To avoid confusion:

TEST_ASSERT(r == -1 && errno == EPERM,
"This process should not have permission to disable NX huge pages");

> diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> index 60bfed8181b9..c21c1f639141 100755
> --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.sh
> @@ -16,6 +16,8 @@ HUGE_PAGES=$(sudo cat /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages)
>
> set +e
>
> +NXECUTABLE="$(dirname $0)/nx_huge_pages_test"
> +
> (
> set -e
>
> @@ -24,7 +26,15 @@ set +e
> sudo echo 100 > /sys/module/kvm/parameters/nx_huge_pages_recovery_period_ms
> sudo echo 3 > /sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages
>
> - "$(dirname $0)"/nx_huge_pages_test 887563923
> + # Test with reboot permissions
> + sudo setcap cap_sys_boot+ep $NXECUTABLE

This leaves cap_sys_boot set on the executable if the script is run as root.

Probably this? It's moderately user friendly without going too crazy on error
handling.

# Test with reboot permissions
if [ $(whoami) != "root" ] ; then
sudo setcap cap_sys_boot+ep $NXECUTABLE
fi
$NXECUTABLE 887563923 1

# Test without reboot permissions
if [ $(whoami) != "root" ] ; then
sudo setcap cap_sys_boot-ep $NXECUTABLE
$NXECUTABLE 887563923 0
fi

> + $NXECUTABLE 887563923 1
> +
> + # Test without reboot permissions
> + if [ $(whoami) != "root" ] ; then
> + sudo setcap cap_sys_boot-ep $NXECUTABLE
> + $NXECUTABLE 887563923 0

I would much prefer a proper flag, not a magic 0 vs. 1.

> + fi
> )
> RET=$?
>
> --
> 2.36.0.464.gb9c8b46e94-goog
>