Add a static assert to the KVM/VM/vCPU ioctl() helpers to verify that the
size of the argument provided matches the expected size of the IOCTL.
Because ioctl() ultimately takes a "void *", it's all too easy to pass in
garbage and not detect the error until runtime. E.g. while working on a
CPUID rework, selftests happily compiled when vcpu_set_cpuid()
unintentionally passed the cpuid() function as the parameter to ioctl()
(a local "cpuid" parameter was removed, but its use was not replaced with
"vcpu->cpuid" as intended).
Tweak a variety of benign issues that aren't compatible with the sanity
check, e.g. passing a non-pointer for ioctls().
Note, static_assert() requires a string on older versions of GCC. Feed
it an empty string to make the compiler happy.
Signed-off-by: Sean Christopherson <[email protected]>
---
.../selftests/kvm/include/kvm_util_base.h | 61 +++++++++++++------
.../selftests/kvm/lib/aarch64/processor.c | 2 +-
tools/testing/selftests/kvm/lib/guest_modes.c | 2 +-
tools/testing/selftests/kvm/lib/kvm_util.c | 29 +--------
tools/testing/selftests/kvm/s390x/resets.c | 6 +-
.../selftests/kvm/x86_64/mmio_warning_test.c | 2 +-
.../kvm/x86_64/pmu_event_filter_test.c | 2 +-
.../selftests/kvm/x86_64/xen_shinfo_test.c | 6 +-
8 files changed, 56 insertions(+), 54 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 04ddab322b6b..0eaf0c9b7612 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -180,29 +180,56 @@ static inline bool kvm_has_cap(long cap)
#define __KVM_IOCTL_ERROR(_name, _ret) __KVM_SYSCALL_ERROR(_name, _ret)
#define KVM_IOCTL_ERROR(_ioctl, _ret) __KVM_IOCTL_ERROR(#_ioctl, _ret)
-#define __kvm_ioctl(kvm_fd, cmd, arg) \
- ioctl(kvm_fd, cmd, arg)
+#define kvm_do_ioctl(fd, cmd, arg) \
+({ \
+ static_assert(!_IOC_SIZE(cmd) || sizeof(*arg) == _IOC_SIZE(cmd), ""); \
+ ioctl(fd, cmd, arg); \
+})
-static inline void _kvm_ioctl(int kvm_fd, unsigned long cmd, const char *name,
- void *arg)
-{
- int ret = __kvm_ioctl(kvm_fd, cmd, arg);
+#define __kvm_ioctl(kvm_fd, cmd, arg) \
+ kvm_do_ioctl(kvm_fd, cmd, arg)
- TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));
-}
+
+#define _kvm_ioctl(kvm_fd, cmd, name, arg) \
+({ \
+ int ret = __kvm_ioctl(kvm_fd, cmd, arg); \
+ \
+ TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret)); \
+})
#define kvm_ioctl(kvm_fd, cmd, arg) \
_kvm_ioctl(kvm_fd, cmd, #cmd, arg)
-int __vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg);
-void _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, const char *name, void *arg);
-#define vm_ioctl(vm, cmd, arg) _vm_ioctl(vm, cmd, #cmd, arg)
-
-int __vcpu_ioctl(struct kvm_vcpu *vcpu, unsigned long cmd,
- void *arg);
-void _vcpu_ioctl(struct kvm_vcpu *vcpu, unsigned long cmd,
- const char *name, void *arg);
-#define vcpu_ioctl(vcpu, cmd, arg) \
+#define __vm_ioctl(vm, cmd, arg) \
+({ \
+ static_assert(sizeof(*(vm)) == sizeof(struct kvm_vm), ""); \
+ kvm_do_ioctl((vm)->fd, cmd, arg); \
+})
+
+#define _vm_ioctl(vcpu, cmd, name, arg) \
+({ \
+ int ret = __vm_ioctl(vcpu, cmd, arg); \
+ \
+ TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret)); \
+})
+
+#define vm_ioctl(vm, cmd, arg) \
+ _vm_ioctl(vm, cmd, #cmd, arg)
+
+#define __vcpu_ioctl(vcpu, cmd, arg) \
+({ \
+ static_assert(sizeof(*(vcpu)) == sizeof(struct kvm_vcpu), ""); \
+ kvm_do_ioctl((vcpu)->fd, cmd, arg); \
+})
+
+#define _vcpu_ioctl(vcpu, cmd, name, arg) \
+({ \
+ int ret = __vcpu_ioctl(vcpu, cmd, arg); \
+ \
+ TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret)); \
+})
+
+#define vcpu_ioctl(vcpu, cmd, arg) \
_vcpu_ioctl(vcpu, cmd, #cmd, arg)
/*
diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
index 6bd27782f00c..6f5551368944 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
@@ -472,7 +472,7 @@ void aarch64_get_supported_page_sizes(uint32_t ipa,
};
kvm_fd = open_kvm_dev_path_or_exit();
- vm_fd = __kvm_ioctl(kvm_fd, KVM_CREATE_VM, ipa);
+ vm_fd = __kvm_ioctl(kvm_fd, KVM_CREATE_VM, (void *)(unsigned long)ipa);
TEST_ASSERT(vm_fd >= 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, vm_fd));
vcpu_fd = ioctl(vm_fd, KVM_CREATE_VCPU, 0);
diff --git a/tools/testing/selftests/kvm/lib/guest_modes.c b/tools/testing/selftests/kvm/lib/guest_modes.c
index 0be56c63aed6..99a575bbbc52 100644
--- a/tools/testing/selftests/kvm/lib/guest_modes.c
+++ b/tools/testing/selftests/kvm/lib/guest_modes.c
@@ -65,7 +65,7 @@ void guest_modes_append_default(void)
struct kvm_s390_vm_cpu_processor info;
kvm_fd = open_kvm_dev_path_or_exit();
- vm_fd = __kvm_ioctl(kvm_fd, KVM_CREATE_VM, 0);
+ vm_fd = __kvm_ioctl(kvm_fd, KVM_CREATE_VM, NULL);
kvm_device_attr_get(vm_fd, KVM_S390_VM_CPU_MODEL,
KVM_S390_VM_CPU_PROCESSOR, &info);
close(vm_fd);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 603a6d529357..f0300767df16 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -72,7 +72,7 @@ unsigned int kvm_check_cap(long cap)
int kvm_fd;
kvm_fd = open_kvm_dev_path_or_exit();
- ret = __kvm_ioctl(kvm_fd, KVM_CHECK_EXTENSION, cap);
+ ret = __kvm_ioctl(kvm_fd, KVM_CHECK_EXTENSION, (void *)cap);
TEST_ASSERT(ret >= 0, KVM_IOCTL_ERROR(KVM_CHECK_EXTENSION, ret));
close(kvm_fd);
@@ -92,7 +92,7 @@ static void vm_open(struct kvm_vm *vm)
TEST_REQUIRE(kvm_has_cap(KVM_CAP_IMMEDIATE_EXIT));
- vm->fd = __kvm_ioctl(vm->kvm_fd, KVM_CREATE_VM, vm->type);
+ vm->fd = __kvm_ioctl(vm->kvm_fd, KVM_CREATE_VM, (void *)vm->type);
TEST_ASSERT(vm->fd >= 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, vm->fd));
}
@@ -1449,19 +1449,6 @@ struct kvm_reg_list *vcpu_get_reg_list(struct kvm_vcpu *vcpu)
return reg_list;
}
-int __vcpu_ioctl(struct kvm_vcpu *vcpu, unsigned long cmd, void *arg)
-{
- return ioctl(vcpu->fd, cmd, arg);
-}
-
-void _vcpu_ioctl(struct kvm_vcpu *vcpu, unsigned long cmd, const char *name,
- void *arg)
-{
- int ret = __vcpu_ioctl(vcpu, cmd, arg);
-
- TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));
-}
-
void *vcpu_map_dirty_ring(struct kvm_vcpu *vcpu)
{
uint32_t page_size = vcpu->vm->page_size;
@@ -1491,18 +1478,6 @@ void *vcpu_map_dirty_ring(struct kvm_vcpu *vcpu)
return vcpu->dirty_gfns;
}
-int __vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
-{
- return ioctl(vm->fd, cmd, arg);
-}
-
-void _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, const char *name, void *arg)
-{
- int ret = __vm_ioctl(vm, cmd, arg);
-
- TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));
-}
-
/*
* Device Ioctl
*/
diff --git a/tools/testing/selftests/kvm/s390x/resets.c b/tools/testing/selftests/kvm/s390x/resets.c
index 4ba866047401..359fd18f473b 100644
--- a/tools/testing/selftests/kvm/s390x/resets.c
+++ b/tools/testing/selftests/kvm/s390x/resets.c
@@ -224,7 +224,7 @@ static void test_normal(void)
inject_irq(vcpu);
- vcpu_ioctl(vcpu, KVM_S390_NORMAL_RESET, 0);
+ vcpu_ioctl(vcpu, KVM_S390_NORMAL_RESET, NULL);
/* must clears */
assert_normal(vcpu);
@@ -247,7 +247,7 @@ static void test_initial(void)
inject_irq(vcpu);
- vcpu_ioctl(vcpu, KVM_S390_INITIAL_RESET, 0);
+ vcpu_ioctl(vcpu, KVM_S390_INITIAL_RESET, NULL);
/* must clears */
assert_normal(vcpu);
@@ -270,7 +270,7 @@ static void test_clear(void)
inject_irq(vcpu);
- vcpu_ioctl(vcpu, KVM_S390_CLEAR_RESET, 0);
+ vcpu_ioctl(vcpu, KVM_S390_CLEAR_RESET, NULL);
/* must clears */
assert_normal(vcpu);
diff --git a/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c b/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
index 0e4590afd0e1..fb02581953a3 100644
--- a/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
+++ b/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
@@ -59,7 +59,7 @@ void test(void)
kvm = open("/dev/kvm", O_RDWR);
TEST_ASSERT(kvm != -1, "failed to open /dev/kvm");
- kvmvm = __kvm_ioctl(kvm, KVM_CREATE_VM, 0);
+ kvmvm = __kvm_ioctl(kvm, KVM_CREATE_VM, NULL);
TEST_ASSERT(kvmvm > 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, kvmvm));
kvmcpu = ioctl(kvmvm, KVM_CREATE_VCPU, 0);
TEST_ASSERT(kvmcpu != -1, KVM_IOCTL_ERROR(KVM_CREATE_VCPU, kvmcpu));
diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
index de9ee00d84cf..66930384ef97 100644
--- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
+++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
@@ -266,7 +266,7 @@ static void test_without_filter(struct kvm_vcpu *vcpu)
static uint64_t test_with_filter(struct kvm_vcpu *vcpu,
struct kvm_pmu_event_filter *f)
{
- vm_ioctl(vcpu->vm, KVM_SET_PMU_EVENT_FILTER, (void *)f);
+ vm_ioctl(vcpu->vm, KVM_SET_PMU_EVENT_FILTER, f);
return run_vcpu_to_sync(vcpu);
}
diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
index bdcb28186ccc..a4a78637c35a 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -472,7 +472,7 @@ int main(int argc, char *argv[])
irq_routes.entries[1].u.xen_evtchn.vcpu = vcpu->id;
irq_routes.entries[1].u.xen_evtchn.priority = KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL;
- vm_ioctl(vm, KVM_SET_GSI_ROUTING, &irq_routes);
+ vm_ioctl(vm, KVM_SET_GSI_ROUTING, &irq_routes.info);
struct kvm_irqfd ifd = { };
@@ -716,7 +716,7 @@ int main(int argc, char *argv[])
if (verbose)
printf("Testing restored oneshot timer\n");
- tmr.u.timer.expires_ns = rs->state_entry_time + 100000000,
+ tmr.u.timer.expires_ns = rs->state_entry_time + 100000000;
vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &tmr);
evtchn_irq_expected = true;
alarm(1);
@@ -743,7 +743,7 @@ int main(int argc, char *argv[])
if (verbose)
printf("Testing SCHEDOP_poll wake on masked event\n");
- tmr.u.timer.expires_ns = rs->state_entry_time + 100000000,
+ tmr.u.timer.expires_ns = rs->state_entry_time + 100000000;
vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &tmr);
alarm(1);
break;
--
2.36.1.255.ge46751e96f-goog
On Fri, Jun 03, 2022 at 12:43:31AM +0000, Sean Christopherson wrote:
> Add a static assert to the KVM/VM/vCPU ioctl() helpers to verify that the
> size of the argument provided matches the expected size of the IOCTL.
> Because ioctl() ultimately takes a "void *", it's all too easy to pass in
> garbage and not detect the error until runtime. E.g. while working on a
> CPUID rework, selftests happily compiled when vcpu_set_cpuid()
> unintentionally passed the cpuid() function as the parameter to ioctl()
> (a local "cpuid" parameter was removed, but its use was not replaced with
> "vcpu->cpuid" as intended).
>
> Tweak a variety of benign issues that aren't compatible with the sanity
> check, e.g. passing a non-pointer for ioctls().
>
> Note, static_assert() requires a string on older versions of GCC. Feed
> it an empty string to make the compiler happy.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> .../selftests/kvm/include/kvm_util_base.h | 61 +++++++++++++------
> .../selftests/kvm/lib/aarch64/processor.c | 2 +-
> tools/testing/selftests/kvm/lib/guest_modes.c | 2 +-
> tools/testing/selftests/kvm/lib/kvm_util.c | 29 +--------
> tools/testing/selftests/kvm/s390x/resets.c | 6 +-
> .../selftests/kvm/x86_64/mmio_warning_test.c | 2 +-
> .../kvm/x86_64/pmu_event_filter_test.c | 2 +-
> .../selftests/kvm/x86_64/xen_shinfo_test.c | 6 +-
> 8 files changed, 56 insertions(+), 54 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> index 04ddab322b6b..0eaf0c9b7612 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> @@ -180,29 +180,56 @@ static inline bool kvm_has_cap(long cap)
> #define __KVM_IOCTL_ERROR(_name, _ret) __KVM_SYSCALL_ERROR(_name, _ret)
> #define KVM_IOCTL_ERROR(_ioctl, _ret) __KVM_IOCTL_ERROR(#_ioctl, _ret)
>
> -#define __kvm_ioctl(kvm_fd, cmd, arg) \
> - ioctl(kvm_fd, cmd, arg)
> +#define kvm_do_ioctl(fd, cmd, arg) \
> +({ \
> + static_assert(!_IOC_SIZE(cmd) || sizeof(*arg) == _IOC_SIZE(cmd), ""); \
> + ioctl(fd, cmd, arg); \
> +})
>
> -static inline void _kvm_ioctl(int kvm_fd, unsigned long cmd, const char *name,
> - void *arg)
> -{
> - int ret = __kvm_ioctl(kvm_fd, cmd, arg);
> +#define __kvm_ioctl(kvm_fd, cmd, arg) \
> + kvm_do_ioctl(kvm_fd, cmd, arg)
>
> - TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));
> -}
> +
While we've gained the static asserts we've also lost the type checking
that the inline functions provided. Is there anyway we can bring them back
with more macro tricks?
> +#define _kvm_ioctl(kvm_fd, cmd, name, arg) \
> +({ \
> + int ret = __kvm_ioctl(kvm_fd, cmd, arg); \
> + \
> + TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret)); \
> +})
>
> #define kvm_ioctl(kvm_fd, cmd, arg) \
> _kvm_ioctl(kvm_fd, cmd, #cmd, arg)
>
> -int __vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg);
> -void _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, const char *name, void *arg);
> -#define vm_ioctl(vm, cmd, arg) _vm_ioctl(vm, cmd, #cmd, arg)
> -
> -int __vcpu_ioctl(struct kvm_vcpu *vcpu, unsigned long cmd,
> - void *arg);
> -void _vcpu_ioctl(struct kvm_vcpu *vcpu, unsigned long cmd,
> - const char *name, void *arg);
> -#define vcpu_ioctl(vcpu, cmd, arg) \
> +#define __vm_ioctl(vm, cmd, arg) \
> +({ \
> + static_assert(sizeof(*(vm)) == sizeof(struct kvm_vm), ""); \
> + kvm_do_ioctl((vm)->fd, cmd, arg); \
> +})
> +
> +#define _vm_ioctl(vcpu, cmd, name, arg) \
> +({ \
> + int ret = __vm_ioctl(vcpu, cmd, arg); \
> + \
> + TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret)); \
> +})
> +
> +#define vm_ioctl(vm, cmd, arg) \
> + _vm_ioctl(vm, cmd, #cmd, arg)
> +
> +#define __vcpu_ioctl(vcpu, cmd, arg) \
> +({ \
> + static_assert(sizeof(*(vcpu)) == sizeof(struct kvm_vcpu), ""); \
> + kvm_do_ioctl((vcpu)->fd, cmd, arg); \
> +})
> +
> +#define _vcpu_ioctl(vcpu, cmd, name, arg) \
> +({ \
> + int ret = __vcpu_ioctl(vcpu, cmd, arg); \
> + \
> + TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret)); \
> +})
> +
> +#define vcpu_ioctl(vcpu, cmd, arg) \
> _vcpu_ioctl(vcpu, cmd, #cmd, arg)
>
> /*
> diff --git a/tools/testing/selftests/kvm/lib/aarch64/processor.c b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> index 6bd27782f00c..6f5551368944 100644
> --- a/tools/testing/selftests/kvm/lib/aarch64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/aarch64/processor.c
> @@ -472,7 +472,7 @@ void aarch64_get_supported_page_sizes(uint32_t ipa,
> };
>
> kvm_fd = open_kvm_dev_path_or_exit();
> - vm_fd = __kvm_ioctl(kvm_fd, KVM_CREATE_VM, ipa);
> + vm_fd = __kvm_ioctl(kvm_fd, KVM_CREATE_VM, (void *)(unsigned long)ipa);
> TEST_ASSERT(vm_fd >= 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, vm_fd));
>
> vcpu_fd = ioctl(vm_fd, KVM_CREATE_VCPU, 0);
> diff --git a/tools/testing/selftests/kvm/lib/guest_modes.c b/tools/testing/selftests/kvm/lib/guest_modes.c
> index 0be56c63aed6..99a575bbbc52 100644
> --- a/tools/testing/selftests/kvm/lib/guest_modes.c
> +++ b/tools/testing/selftests/kvm/lib/guest_modes.c
> @@ -65,7 +65,7 @@ void guest_modes_append_default(void)
> struct kvm_s390_vm_cpu_processor info;
>
> kvm_fd = open_kvm_dev_path_or_exit();
> - vm_fd = __kvm_ioctl(kvm_fd, KVM_CREATE_VM, 0);
> + vm_fd = __kvm_ioctl(kvm_fd, KVM_CREATE_VM, NULL);
> kvm_device_attr_get(vm_fd, KVM_S390_VM_CPU_MODEL,
> KVM_S390_VM_CPU_PROCESSOR, &info);
> close(vm_fd);
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 603a6d529357..f0300767df16 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -72,7 +72,7 @@ unsigned int kvm_check_cap(long cap)
> int kvm_fd;
>
> kvm_fd = open_kvm_dev_path_or_exit();
> - ret = __kvm_ioctl(kvm_fd, KVM_CHECK_EXTENSION, cap);
> + ret = __kvm_ioctl(kvm_fd, KVM_CHECK_EXTENSION, (void *)cap);
> TEST_ASSERT(ret >= 0, KVM_IOCTL_ERROR(KVM_CHECK_EXTENSION, ret));
>
> close(kvm_fd);
> @@ -92,7 +92,7 @@ static void vm_open(struct kvm_vm *vm)
>
> TEST_REQUIRE(kvm_has_cap(KVM_CAP_IMMEDIATE_EXIT));
>
> - vm->fd = __kvm_ioctl(vm->kvm_fd, KVM_CREATE_VM, vm->type);
> + vm->fd = __kvm_ioctl(vm->kvm_fd, KVM_CREATE_VM, (void *)vm->type);
> TEST_ASSERT(vm->fd >= 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, vm->fd));
> }
>
> @@ -1449,19 +1449,6 @@ struct kvm_reg_list *vcpu_get_reg_list(struct kvm_vcpu *vcpu)
> return reg_list;
> }
>
> -int __vcpu_ioctl(struct kvm_vcpu *vcpu, unsigned long cmd, void *arg)
> -{
> - return ioctl(vcpu->fd, cmd, arg);
> -}
> -
> -void _vcpu_ioctl(struct kvm_vcpu *vcpu, unsigned long cmd, const char *name,
> - void *arg)
> -{
> - int ret = __vcpu_ioctl(vcpu, cmd, arg);
> -
> - TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));
> -}
> -
> void *vcpu_map_dirty_ring(struct kvm_vcpu *vcpu)
> {
> uint32_t page_size = vcpu->vm->page_size;
> @@ -1491,18 +1478,6 @@ void *vcpu_map_dirty_ring(struct kvm_vcpu *vcpu)
> return vcpu->dirty_gfns;
> }
>
> -int __vm_ioctl(struct kvm_vm *vm, unsigned long cmd, void *arg)
> -{
> - return ioctl(vm->fd, cmd, arg);
> -}
> -
> -void _vm_ioctl(struct kvm_vm *vm, unsigned long cmd, const char *name, void *arg)
> -{
> - int ret = __vm_ioctl(vm, cmd, arg);
> -
> - TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));
> -}
> -
> /*
> * Device Ioctl
> */
> diff --git a/tools/testing/selftests/kvm/s390x/resets.c b/tools/testing/selftests/kvm/s390x/resets.c
> index 4ba866047401..359fd18f473b 100644
> --- a/tools/testing/selftests/kvm/s390x/resets.c
> +++ b/tools/testing/selftests/kvm/s390x/resets.c
> @@ -224,7 +224,7 @@ static void test_normal(void)
>
> inject_irq(vcpu);
>
> - vcpu_ioctl(vcpu, KVM_S390_NORMAL_RESET, 0);
> + vcpu_ioctl(vcpu, KVM_S390_NORMAL_RESET, NULL);
>
> /* must clears */
> assert_normal(vcpu);
> @@ -247,7 +247,7 @@ static void test_initial(void)
>
> inject_irq(vcpu);
>
> - vcpu_ioctl(vcpu, KVM_S390_INITIAL_RESET, 0);
> + vcpu_ioctl(vcpu, KVM_S390_INITIAL_RESET, NULL);
>
> /* must clears */
> assert_normal(vcpu);
> @@ -270,7 +270,7 @@ static void test_clear(void)
>
> inject_irq(vcpu);
>
> - vcpu_ioctl(vcpu, KVM_S390_CLEAR_RESET, 0);
> + vcpu_ioctl(vcpu, KVM_S390_CLEAR_RESET, NULL);
>
> /* must clears */
> assert_normal(vcpu);
> diff --git a/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c b/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
> index 0e4590afd0e1..fb02581953a3 100644
> --- a/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
> @@ -59,7 +59,7 @@ void test(void)
>
> kvm = open("/dev/kvm", O_RDWR);
> TEST_ASSERT(kvm != -1, "failed to open /dev/kvm");
> - kvmvm = __kvm_ioctl(kvm, KVM_CREATE_VM, 0);
> + kvmvm = __kvm_ioctl(kvm, KVM_CREATE_VM, NULL);
> TEST_ASSERT(kvmvm > 0, KVM_IOCTL_ERROR(KVM_CREATE_VM, kvmvm));
> kvmcpu = ioctl(kvmvm, KVM_CREATE_VCPU, 0);
> TEST_ASSERT(kvmcpu != -1, KVM_IOCTL_ERROR(KVM_CREATE_VCPU, kvmcpu));
> diff --git a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> index de9ee00d84cf..66930384ef97 100644
> --- a/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/pmu_event_filter_test.c
> @@ -266,7 +266,7 @@ static void test_without_filter(struct kvm_vcpu *vcpu)
> static uint64_t test_with_filter(struct kvm_vcpu *vcpu,
> struct kvm_pmu_event_filter *f)
> {
> - vm_ioctl(vcpu->vm, KVM_SET_PMU_EVENT_FILTER, (void *)f);
> + vm_ioctl(vcpu->vm, KVM_SET_PMU_EVENT_FILTER, f);
> return run_vcpu_to_sync(vcpu);
> }
>
> diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> index bdcb28186ccc..a4a78637c35a 100644
> --- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> @@ -472,7 +472,7 @@ int main(int argc, char *argv[])
> irq_routes.entries[1].u.xen_evtchn.vcpu = vcpu->id;
> irq_routes.entries[1].u.xen_evtchn.priority = KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL;
>
> - vm_ioctl(vm, KVM_SET_GSI_ROUTING, &irq_routes);
> + vm_ioctl(vm, KVM_SET_GSI_ROUTING, &irq_routes.info);
>
> struct kvm_irqfd ifd = { };
>
> @@ -716,7 +716,7 @@ int main(int argc, char *argv[])
> if (verbose)
> printf("Testing restored oneshot timer\n");
>
> - tmr.u.timer.expires_ns = rs->state_entry_time + 100000000,
> + tmr.u.timer.expires_ns = rs->state_entry_time + 100000000;
> vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &tmr);
> evtchn_irq_expected = true;
> alarm(1);
> @@ -743,7 +743,7 @@ int main(int argc, char *argv[])
> if (verbose)
> printf("Testing SCHEDOP_poll wake on masked event\n");
>
> - tmr.u.timer.expires_ns = rs->state_entry_time + 100000000,
> + tmr.u.timer.expires_ns = rs->state_entry_time + 100000000;
> vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &tmr);
> alarm(1);
> break;
> --
> 2.36.1.255.ge46751e96f-goog
>
The last two changes don't really belong in this commit, but I won't tell
anyway, if you don't.
Thanks,
drew
On Fri, Jun 10, 2022, Andrew Jones wrote:
> On Fri, Jun 03, 2022 at 12:43:31AM +0000, Sean Christopherson wrote:
> > Add a static assert to the KVM/VM/vCPU ioctl() helpers to verify that the
> > size of the argument provided matches the expected size of the IOCTL.
> > Because ioctl() ultimately takes a "void *", it's all too easy to pass in
> > garbage and not detect the error until runtime. E.g. while working on a
> > CPUID rework, selftests happily compiled when vcpu_set_cpuid()
> > unintentionally passed the cpuid() function as the parameter to ioctl()
> > (a local "cpuid" parameter was removed, but its use was not replaced with
> > "vcpu->cpuid" as intended).
> >
> > Tweak a variety of benign issues that aren't compatible with the sanity
> > check, e.g. passing a non-pointer for ioctls().
> >
> > Note, static_assert() requires a string on older versions of GCC. Feed
> > it an empty string to make the compiler happy.
> >
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > .../selftests/kvm/include/kvm_util_base.h | 61 +++++++++++++------
> > .../selftests/kvm/lib/aarch64/processor.c | 2 +-
> > tools/testing/selftests/kvm/lib/guest_modes.c | 2 +-
> > tools/testing/selftests/kvm/lib/kvm_util.c | 29 +--------
> > tools/testing/selftests/kvm/s390x/resets.c | 6 +-
> > .../selftests/kvm/x86_64/mmio_warning_test.c | 2 +-
> > .../kvm/x86_64/pmu_event_filter_test.c | 2 +-
> > .../selftests/kvm/x86_64/xen_shinfo_test.c | 6 +-
> > 8 files changed, 56 insertions(+), 54 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > index 04ddab322b6b..0eaf0c9b7612 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util_base.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
> > @@ -180,29 +180,56 @@ static inline bool kvm_has_cap(long cap)
> > #define __KVM_IOCTL_ERROR(_name, _ret) __KVM_SYSCALL_ERROR(_name, _ret)
> > #define KVM_IOCTL_ERROR(_ioctl, _ret) __KVM_IOCTL_ERROR(#_ioctl, _ret)
> >
> > -#define __kvm_ioctl(kvm_fd, cmd, arg) \
> > - ioctl(kvm_fd, cmd, arg)
> > +#define kvm_do_ioctl(fd, cmd, arg) \
> > +({ \
> > + static_assert(!_IOC_SIZE(cmd) || sizeof(*arg) == _IOC_SIZE(cmd), ""); \
> > + ioctl(fd, cmd, arg); \
> > +})
> >
> > -static inline void _kvm_ioctl(int kvm_fd, unsigned long cmd, const char *name,
> > - void *arg)
> > -{
> > - int ret = __kvm_ioctl(kvm_fd, cmd, arg);
> > +#define __kvm_ioctl(kvm_fd, cmd, arg) \
> > + kvm_do_ioctl(kvm_fd, cmd, arg)
> >
> > - TEST_ASSERT(!ret, __KVM_IOCTL_ERROR(name, ret));
> > -}
> > +
>
> While we've gained the static asserts we've also lost the type checking
> that the inline functions provided. Is there anyway we can bring them back
> with more macro tricks?
Gah, I overthought this. It doesn't even require macros, just a dummy helper.
I wasn't trying to use static_assert() to enforce the type check, which is how I
ended up with the sizeof() ugliness (not the one above). But it's far easier to
let the compiler do the checking.
I'll send a small fixup series to address this and your other feedback.
static __always_inline void static_assert_is_vm(struct kvm_vm *vm) { }
#define __vm_ioctl(vm, cmd, arg) \
({ \
static_assert_is_vm(vm); \
kvm_do_ioctl((vm)->fd, cmd, arg); \
})
static __always_inline void static_assert_is_vcpu(struct kvm_vcpu *vcpu) { }
#define __vcpu_ioctl(vcpu, cmd, arg) \
({ \
static_assert_is_vcpu(vcpu); \
kvm_do_ioctl((vcpu)->fd, cmd, arg); \
})
In file included from include/kvm_util.h:10,
from lib/x86_64/processor.c:9:
lib/x86_64/processor.c: In function ‘_vcpu_set_msr’:
lib/x86_64/processor.c:831:33: error: passing argument 1 of ‘static_assert_is_vcpu’ from incompatible pointer type [-Werror=incompatible-pointer-types]
831 | return __vcpu_ioctl(vcpu->vm, KVM_SET_MSRS, &buffer.header);
| ~~~~^~~~
| |
| struct kvm_vm *
include/kvm_util_base.h:232:31: note: in definition of macro ‘__vcpu_ioctl’
232 | static_assert_is_vcpu(vcpu); \
| ^~~~
include/kvm_util_base.h:225:68: note: expected ‘struct kvm_vcpu *’ but argument is of type ‘struct kvm_vm *’
225 | static __always_inline void static_assert_is_vcpu(struct kvm_vcpu *vcpu)
| ~~~~~~~~~~~~~~~~~^~~~
cc1: all warnings being treated as errors