This patch series adds more test case issuing ioctls to ucontrol VMs and its floating interrupt controller. The test cases trigger three possible null pointer dereferences within the handling of the KVM_DEV_FLIC_APF_ENABLE, KVM_DEV_FLIC_APF_DISABLE_WAIT and KVM_SET_GSI_ROUTING ioctl.
All of these issues do only exist on ucontrol VMs. Fixes for the issues are included within the patch series.
Christoph Schlameuss (6): kvm: s390: Reject setting flic pfault attributes on ucontrol VMs selftests: kvm: s390: Add ucontrol flic attr selftests kvm: s390: Reject KVM_SET_GSI_ROUTING on ucontrol VMs selftests: kvm: s390: Add ucontrol gis routing test selftests: kvm: s390: Streamline uc_skey test to issue iske after sske selftests: kvm: s390: Add has device attr check to uc_attr_mem_limit selftest
arch/s390/kvm/interrupt.c | 6 + .../selftests/kvm/s390x/ucontrol_test.c | 196 ++++++++++++++++-- 2 files changed, 184 insertions(+), 18 deletions(-)
Prevent null pointer dereference when processing the KVM_DEV_FLIC_APF_ENABLE and KVM_DEV_FLIC_APF_DISABLE_WAIT ioctls in the interrupt controller.
Reported-by: Claudio Imbrenda imbrenda@linux.ibm.com Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com --- arch/s390/kvm/interrupt.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c index ea8dce299954..22d73c13e555 100644 --- a/arch/s390/kvm/interrupt.c +++ b/arch/s390/kvm/interrupt.c @@ -2678,9 +2678,13 @@ static int flic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) kvm_s390_clear_float_irqs(dev->kvm); break; case KVM_DEV_FLIC_APF_ENABLE: + if (kvm_is_ucontrol(dev->kvm)) + return -EINVAL; dev->kvm->arch.gmap->pfault_enabled = 1; break; case KVM_DEV_FLIC_APF_DISABLE_WAIT: + if (kvm_is_ucontrol(dev->kvm)) + return -EINVAL; dev->kvm->arch.gmap->pfault_enabled = 0; /* * Make sure no async faults are in transition when
On 12/9/24 12:07 PM, Christoph Schlameuss wrote:
Prevent null pointer dereference when processing the KVM_DEV_FLIC_APF_ENABLE and KVM_DEV_FLIC_APF_DISABLE_WAIT ioctls in the interrupt controller.
Reported-by: Claudio Imbrenda imbrenda@linux.ibm.com Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com
Please add a fixes tag to this patch and #3. It's fine to just send a reply to the patches until there are enough comments for a proper v2.
On 12/9/24 12:07 PM, Christoph Schlameuss wrote:
Prevent null pointer dereference when processing the KVM_DEV_FLIC_APF_ENABLE and KVM_DEV_FLIC_APF_DISABLE_WAIT ioctls in the interrupt controller.
Reported-by: Claudio Imbrenda imbrenda@linux.ibm.com Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com
Please have a look into the documentation and find a place to write down the !ucontrol requirement.
Documentation/virt/kvm/devices/s390_flic.rst Documentation/virt/kvm/api.rst
Prevent null pointer dereference when processing the KVM_DEV_FLIC_APF_ENABLE and KVM_DEV_FLIC_APF_DISABLE_WAIT ioctls in the interrupt controller.
Fixes: 3c038e6be0e2 ("KVM: async_pf: Async page fault support on s390") Reported-by: Claudio Imbrenda imbrenda@linux.ibm.com Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com ---
Added documentation and fixes comment.
--- Documentation/virt/kvm/devices/s390_flic.rst | 4 ++++ arch/s390/kvm/interrupt.c | 4 ++++ 2 files changed, 8 insertions(+)
diff --git a/Documentation/virt/kvm/devices/s390_flic.rst b/Documentation/virt/kvm/devices/s390_flic.rst index ea96559ba501..b784f8016748 100644 --- a/Documentation/virt/kvm/devices/s390_flic.rst +++ b/Documentation/virt/kvm/devices/s390_flic.rst @@ -58,11 +58,15 @@ Groups: Enables async page faults for the guest. So in case of a major page fault the host is allowed to handle this async and continues the guest.
+ -EINVAL is returned when called on the FLIC of a ucontrol VM. + KVM_DEV_FLIC_APF_DISABLE_WAIT Disables async page faults for the guest and waits until already pending async page faults are done. This is necessary to trigger a completion interrupt for every init interrupt before migrating the interrupt list.
+ -EINVAL is returned when called on the FLIC of a ucontrol VM. + KVM_DEV_FLIC_ADAPTER_REGISTER Register an I/O adapter interrupt source. Takes a kvm_s390_io_adapter describing the adapter to register:: diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c index ea8dce299954..22d73c13e555 100644 --- a/arch/s390/kvm/interrupt.c +++ b/arch/s390/kvm/interrupt.c @@ -2678,9 +2678,13 @@ static int flic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr) kvm_s390_clear_float_irqs(dev->kvm); break; case KVM_DEV_FLIC_APF_ENABLE: + if (kvm_is_ucontrol(dev->kvm)) + return -EINVAL; dev->kvm->arch.gmap->pfault_enabled = 1; break; case KVM_DEV_FLIC_APF_DISABLE_WAIT: + if (kvm_is_ucontrol(dev->kvm)) + return -EINVAL; dev->kvm->arch.gmap->pfault_enabled = 0; /* * Make sure no async faults are in transition when
Add some superficial selftests for the floating interrupt controller when using ucontrol VMs. These tests are intended to cover very basic calls only.
Some of the calls may trigger null pointer dereferences on kernels not containing the fixes in this patch series.
Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com --- .../selftests/kvm/s390x/ucontrol_test.c | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+)
diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c index 0c112319dab1..972fac1023b5 100644 --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c @@ -635,4 +635,154 @@ TEST_F(uc_kvm, uc_skey) uc_assert_diag44(self); }
+static char uc_flic_b[PAGE_SIZE]; +static struct kvm_s390_io_adapter uc_flic_ioa = { .id = 0 }; +static struct kvm_s390_io_adapter_req uc_flic_ioam = { .id = 0 }; +static struct kvm_s390_ais_req uc_flic_asim = { .isc = 0 }; +static struct kvm_s390_ais_all uc_flic_asima = { .simm = 0 }; +static struct uc_flic_attr_test { + char *name; + struct kvm_device_attr a; + int hasrc; + u64 getrc; + int geterrno; + u64 setrc; + int seterrno; +} uc_flic_attr_tests[] = { + { + .name = "KVM_DEV_FLIC_GET_ALL_IRQS", + .setrc = 1, .seterrno = EINVAL, + .a = { + .group = KVM_DEV_FLIC_GET_ALL_IRQS, + .addr = (u64)&uc_flic_b, + .attr = PAGE_SIZE, + }, + }, + { + .name = "KVM_DEV_FLIC_ENQUEUE", + .getrc = 1, .geterrno = EINVAL, + .a = { .group = KVM_DEV_FLIC_ENQUEUE, }, + }, + { + .name = "KVM_DEV_FLIC_CLEAR_IRQS", + .getrc = 1, .geterrno = EINVAL, + .a = { .group = KVM_DEV_FLIC_CLEAR_IRQS, }, + }, + { + .name = "KVM_DEV_FLIC_ADAPTER_REGISTER", + .getrc = 1, .geterrno = EINVAL, + .a = { + .group = KVM_DEV_FLIC_ADAPTER_REGISTER, + .addr = (u64)&uc_flic_ioa, + }, + }, + { + .name = "KVM_DEV_FLIC_ADAPTER_MODIFY", + .getrc = 1, .geterrno = EINVAL, + .setrc = 1, .seterrno = EINVAL, + .a = { + .group = KVM_DEV_FLIC_ADAPTER_MODIFY, + .addr = (u64)&uc_flic_ioam, + .attr = sizeof(uc_flic_ioam), + }, + }, + { + .name = "KVM_DEV_FLIC_CLEAR_IO_IRQ", + .getrc = 1, .geterrno = EINVAL, + .setrc = 1, .seterrno = EINVAL, + .a = { + .group = KVM_DEV_FLIC_CLEAR_IO_IRQ, + .attr = 32, + }, + }, + { + .name = "KVM_DEV_FLIC_AISM", + .getrc = 1, .geterrno = EINVAL, + .setrc = 1, .seterrno = ENOTSUP, + .a = { + .group = KVM_DEV_FLIC_AISM, + .addr = (u64)&uc_flic_asim, + }, + }, + { + .name = "KVM_DEV_FLIC_AIRQ_INJECT", + .getrc = 1, .geterrno = EINVAL, + .a = { .group = KVM_DEV_FLIC_AIRQ_INJECT, }, + }, + { + .name = "KVM_DEV_FLIC_AISM_ALL", + .getrc = 1, .geterrno = ENOTSUP, + .setrc = 1, .seterrno = ENOTSUP, + .a = { + .group = KVM_DEV_FLIC_AISM_ALL, + .addr = (u64)&uc_flic_asima, + .attr = sizeof(uc_flic_asima), + }, + }, + { + .name = "KVM_DEV_FLIC_APF_ENABLE", + .getrc = 1, .geterrno = EINVAL, + .setrc = 1, .seterrno = EINVAL, + .a = { .group = KVM_DEV_FLIC_APF_ENABLE, }, + }, + { + .name = "KVM_DEV_FLIC_APF_DISABLE_WAIT", + .getrc = 1, .geterrno = EINVAL, + .setrc = 1, .seterrno = EINVAL, + .a = { .group = KVM_DEV_FLIC_APF_DISABLE_WAIT, }, + }, +}; + +TEST_F(uc_kvm, uc_flic_attrs) +{ + struct kvm_create_device cd = { .type = KVM_DEV_TYPE_FLIC }; + struct kvm_device_attr attr; + u64 value; + int rc, i; + + rc = ioctl(self->vm_fd, KVM_CREATE_DEVICE, &cd); + ASSERT_EQ(0, rc) TH_LOG("create device failed with err %s (%i)", + strerror(errno), errno); + + for (i = 0; i < ARRAY_SIZE(uc_flic_attr_tests); i++) { + TH_LOG("test %s", uc_flic_attr_tests[i].name); + attr = (struct kvm_device_attr) { + .group = uc_flic_attr_tests[i].a.group, + .attr = uc_flic_attr_tests[i].a.attr, + .addr = uc_flic_attr_tests[i].a.addr, + }; + if (attr.addr == 0) + attr.addr = (u64)&value; + + rc = ioctl(cd.fd, KVM_HAS_DEVICE_ATTR, &attr); + EXPECT_EQ(uc_flic_attr_tests[i].hasrc, !!rc) + TH_LOG("expected dev attr missing %s", + uc_flic_attr_tests[i].name); + + rc = ioctl(cd.fd, KVM_GET_DEVICE_ATTR, &attr); + EXPECT_EQ(uc_flic_attr_tests[i].getrc, !!rc) + TH_LOG("get dev attr rc not expected on %s %s (%i)", + uc_flic_attr_tests[i].name, + strerror(errno), errno); + if (uc_flic_attr_tests[i].geterrno) + EXPECT_EQ(uc_flic_attr_tests[i].geterrno, errno) + TH_LOG("get dev attr errno not expected on %s %s (%i)", + uc_flic_attr_tests[i].name, + strerror(errno), errno); + + rc = ioctl(cd.fd, KVM_SET_DEVICE_ATTR, &attr); + EXPECT_EQ(uc_flic_attr_tests[i].setrc, !!rc) + TH_LOG("set sev attr rc not expected on %s %s (%i)", + uc_flic_attr_tests[i].name, + strerror(errno), errno); + if (uc_flic_attr_tests[i].seterrno) + EXPECT_EQ(uc_flic_attr_tests[i].seterrno, errno) + TH_LOG("set dev attr errno not expected on %s %s (%i)", + uc_flic_attr_tests[i].name, + strerror(errno), errno); + } + + close(cd.fd); +} + TEST_HARNESS_MAIN
On Mon, 9 Dec 2024 12:07:13 +0100 Christoph Schlameuss schlameuss@linux.ibm.com wrote:
Add some superficial selftests for the floating interrupt controller when using ucontrol VMs. These tests are intended to cover very basic calls only.
Some of the calls may trigger null pointer dereferences on kernels not containing the fixes in this patch series.
Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com
.../selftests/kvm/s390x/ucontrol_test.c | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+)
diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c index 0c112319dab1..972fac1023b5 100644 --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c @@ -635,4 +635,154 @@ TEST_F(uc_kvm, uc_skey) uc_assert_diag44(self); } +static char uc_flic_b[PAGE_SIZE]; +static struct kvm_s390_io_adapter uc_flic_ioa = { .id = 0 }; +static struct kvm_s390_io_adapter_req uc_flic_ioam = { .id = 0 }; +static struct kvm_s390_ais_req uc_flic_asim = { .isc = 0 }; +static struct kvm_s390_ais_all uc_flic_asima = { .simm = 0 }; +static struct uc_flic_attr_test {
- char *name;
- struct kvm_device_attr a;
- int hasrc;
- u64 getrc;
- int geterrno;
- u64 setrc;
I wonder if you really need getrc and setrc? (see below)
- int seterrno;
+} uc_flic_attr_tests[] = {
- {
.name = "KVM_DEV_FLIC_GET_ALL_IRQS",.setrc = 1, .seterrno = EINVAL,
please put them on separate lines ^ (if you end up keeping both)
.a = {.group = KVM_DEV_FLIC_GET_ALL_IRQS,.addr = (u64)&uc_flic_b,.attr = PAGE_SIZE,},- },
- {
.name = "KVM_DEV_FLIC_ENQUEUE",.getrc = 1, .geterrno = EINVAL,.a = { .group = KVM_DEV_FLIC_ENQUEUE, },- },
- {
.name = "KVM_DEV_FLIC_CLEAR_IRQS",.getrc = 1, .geterrno = EINVAL,.a = { .group = KVM_DEV_FLIC_CLEAR_IRQS, },- },
- {
.name = "KVM_DEV_FLIC_ADAPTER_REGISTER",.getrc = 1, .geterrno = EINVAL,.a = {.group = KVM_DEV_FLIC_ADAPTER_REGISTER,.addr = (u64)&uc_flic_ioa,},- },
- {
.name = "KVM_DEV_FLIC_ADAPTER_MODIFY",.getrc = 1, .geterrno = EINVAL,.setrc = 1, .seterrno = EINVAL,.a = {.group = KVM_DEV_FLIC_ADAPTER_MODIFY,.addr = (u64)&uc_flic_ioam,.attr = sizeof(uc_flic_ioam),},- },
- {
.name = "KVM_DEV_FLIC_CLEAR_IO_IRQ",.getrc = 1, .geterrno = EINVAL,.setrc = 1, .seterrno = EINVAL,.a = {.group = KVM_DEV_FLIC_CLEAR_IO_IRQ,.attr = 32,},- },
- {
.name = "KVM_DEV_FLIC_AISM",.getrc = 1, .geterrno = EINVAL,.setrc = 1, .seterrno = ENOTSUP,.a = {.group = KVM_DEV_FLIC_AISM,.addr = (u64)&uc_flic_asim,},- },
- {
.name = "KVM_DEV_FLIC_AIRQ_INJECT",.getrc = 1, .geterrno = EINVAL,.a = { .group = KVM_DEV_FLIC_AIRQ_INJECT, },- },
- {
.name = "KVM_DEV_FLIC_AISM_ALL",.getrc = 1, .geterrno = ENOTSUP,.setrc = 1, .seterrno = ENOTSUP,.a = {.group = KVM_DEV_FLIC_AISM_ALL,.addr = (u64)&uc_flic_asima,.attr = sizeof(uc_flic_asima),},- },
- {
.name = "KVM_DEV_FLIC_APF_ENABLE",.getrc = 1, .geterrno = EINVAL,.setrc = 1, .seterrno = EINVAL,.a = { .group = KVM_DEV_FLIC_APF_ENABLE, },- },
- {
.name = "KVM_DEV_FLIC_APF_DISABLE_WAIT",.getrc = 1, .geterrno = EINVAL,.setrc = 1, .seterrno = EINVAL,.a = { .group = KVM_DEV_FLIC_APF_DISABLE_WAIT, },- },
+};
+TEST_F(uc_kvm, uc_flic_attrs) +{
- struct kvm_create_device cd = { .type = KVM_DEV_TYPE_FLIC };
- struct kvm_device_attr attr;
- u64 value;
- int rc, i;
- rc = ioctl(self->vm_fd, KVM_CREATE_DEVICE, &cd);
- ASSERT_EQ(0, rc) TH_LOG("create device failed with err %s (%i)",
strerror(errno), errno);- for (i = 0; i < ARRAY_SIZE(uc_flic_attr_tests); i++) {
TH_LOG("test %s", uc_flic_attr_tests[i].name);attr = (struct kvm_device_attr) {.group = uc_flic_attr_tests[i].a.group,.attr = uc_flic_attr_tests[i].a.attr,.addr = uc_flic_attr_tests[i].a.addr,};if (attr.addr == 0)attr.addr = (u64)&value;rc = ioctl(cd.fd, KVM_HAS_DEVICE_ATTR, &attr);EXPECT_EQ(uc_flic_attr_tests[i].hasrc, !!rc)TH_LOG("expected dev attr missing %s",uc_flic_attr_tests[i].name);rc = ioctl(cd.fd, KVM_GET_DEVICE_ATTR, &attr);EXPECT_EQ(uc_flic_attr_tests[i].getrc, !!rc)
maybe you could just do:
EXPECT_EQ(!!uc_flic_attr_tests[i].geterrno, !!rc)
(unless I am missing something)
this is not super important, though
TH_LOG("get dev attr rc not expected on %s %s (%i)",uc_flic_attr_tests[i].name,strerror(errno), errno);if (uc_flic_attr_tests[i].geterrno)EXPECT_EQ(uc_flic_attr_tests[i].geterrno, errno)TH_LOG("get dev attr errno not expected on %s %s (%i)",uc_flic_attr_tests[i].name,strerror(errno), errno);rc = ioctl(cd.fd, KVM_SET_DEVICE_ATTR, &attr);EXPECT_EQ(uc_flic_attr_tests[i].setrc, !!rc)TH_LOG("set sev attr rc not expected on %s %s (%i)",uc_flic_attr_tests[i].name,strerror(errno), errno);if (uc_flic_attr_tests[i].seterrno)EXPECT_EQ(uc_flic_attr_tests[i].seterrno, errno)TH_LOG("set dev attr errno not expected on %s %s (%i)",uc_flic_attr_tests[i].name,strerror(errno), errno);- }
- close(cd.fd);
+}
TEST_HARNESS_MAIN
On Mon Dec 9, 2024 at 7:14 PM CET, Claudio Imbrenda wrote:
On Mon, 9 Dec 2024 12:07:13 +0100 Christoph Schlameuss schlameuss@linux.ibm.com wrote:
Add some superficial selftests for the floating interrupt controller when using ucontrol VMs. These tests are intended to cover very basic calls only.
Some of the calls may trigger null pointer dereferences on kernels not containing the fixes in this patch series.
Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com
.../selftests/kvm/s390x/ucontrol_test.c | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+)
diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c index 0c112319dab1..972fac1023b5 100644 --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c @@ -635,4 +635,154 @@ TEST_F(uc_kvm, uc_skey) uc_assert_diag44(self); } +static char uc_flic_b[PAGE_SIZE]; +static struct kvm_s390_io_adapter uc_flic_ioa = { .id = 0 }; +static struct kvm_s390_io_adapter_req uc_flic_ioam = { .id = 0 }; +static struct kvm_s390_ais_req uc_flic_asim = { .isc = 0 }; +static struct kvm_s390_ais_all uc_flic_asima = { .simm = 0 }; +static struct uc_flic_attr_test {
- char *name;
- struct kvm_device_attr a;
- int hasrc;
- u64 getrc;
- int geterrno;
- u64 setrc;
I wonder if you really need getrc and setrc? (see below)
- int seterrno;
+} uc_flic_attr_tests[] = {
- {
.name = "KVM_DEV_FLIC_GET_ALL_IRQS",.setrc = 1, .seterrno = EINVAL,please put them on separate lines ^ (if you end up keeping both)
.a = {.group = KVM_DEV_FLIC_GET_ALL_IRQS,.addr = (u64)&uc_flic_b,.attr = PAGE_SIZE,},- },
- {
.name = "KVM_DEV_FLIC_ENQUEUE",.getrc = 1, .geterrno = EINVAL,.a = { .group = KVM_DEV_FLIC_ENQUEUE, },- },
- {
.name = "KVM_DEV_FLIC_CLEAR_IRQS",.getrc = 1, .geterrno = EINVAL,.a = { .group = KVM_DEV_FLIC_CLEAR_IRQS, },- },
- {
.name = "KVM_DEV_FLIC_ADAPTER_REGISTER",.getrc = 1, .geterrno = EINVAL,.a = {.group = KVM_DEV_FLIC_ADAPTER_REGISTER,.addr = (u64)&uc_flic_ioa,},- },
- {
.name = "KVM_DEV_FLIC_ADAPTER_MODIFY",.getrc = 1, .geterrno = EINVAL,.setrc = 1, .seterrno = EINVAL,.a = {.group = KVM_DEV_FLIC_ADAPTER_MODIFY,.addr = (u64)&uc_flic_ioam,.attr = sizeof(uc_flic_ioam),},- },
- {
.name = "KVM_DEV_FLIC_CLEAR_IO_IRQ",.getrc = 1, .geterrno = EINVAL,.setrc = 1, .seterrno = EINVAL,.a = {.group = KVM_DEV_FLIC_CLEAR_IO_IRQ,.attr = 32,},- },
- {
.name = "KVM_DEV_FLIC_AISM",.getrc = 1, .geterrno = EINVAL,.setrc = 1, .seterrno = ENOTSUP,.a = {.group = KVM_DEV_FLIC_AISM,.addr = (u64)&uc_flic_asim,},- },
- {
.name = "KVM_DEV_FLIC_AIRQ_INJECT",.getrc = 1, .geterrno = EINVAL,.a = { .group = KVM_DEV_FLIC_AIRQ_INJECT, },- },
- {
.name = "KVM_DEV_FLIC_AISM_ALL",.getrc = 1, .geterrno = ENOTSUP,.setrc = 1, .seterrno = ENOTSUP,.a = {.group = KVM_DEV_FLIC_AISM_ALL,.addr = (u64)&uc_flic_asima,.attr = sizeof(uc_flic_asima),},- },
- {
.name = "KVM_DEV_FLIC_APF_ENABLE",.getrc = 1, .geterrno = EINVAL,.setrc = 1, .seterrno = EINVAL,.a = { .group = KVM_DEV_FLIC_APF_ENABLE, },- },
- {
.name = "KVM_DEV_FLIC_APF_DISABLE_WAIT",.getrc = 1, .geterrno = EINVAL,.setrc = 1, .seterrno = EINVAL,.a = { .group = KVM_DEV_FLIC_APF_DISABLE_WAIT, },- },
+};
+TEST_F(uc_kvm, uc_flic_attrs) +{
- struct kvm_create_device cd = { .type = KVM_DEV_TYPE_FLIC };
- struct kvm_device_attr attr;
- u64 value;
- int rc, i;
- rc = ioctl(self->vm_fd, KVM_CREATE_DEVICE, &cd);
- ASSERT_EQ(0, rc) TH_LOG("create device failed with err %s (%i)",
strerror(errno), errno);- for (i = 0; i < ARRAY_SIZE(uc_flic_attr_tests); i++) {
TH_LOG("test %s", uc_flic_attr_tests[i].name);attr = (struct kvm_device_attr) {.group = uc_flic_attr_tests[i].a.group,.attr = uc_flic_attr_tests[i].a.attr,.addr = uc_flic_attr_tests[i].a.addr,};if (attr.addr == 0)attr.addr = (u64)&value;rc = ioctl(cd.fd, KVM_HAS_DEVICE_ATTR, &attr);EXPECT_EQ(uc_flic_attr_tests[i].hasrc, !!rc)TH_LOG("expected dev attr missing %s",uc_flic_attr_tests[i].name);rc = ioctl(cd.fd, KVM_GET_DEVICE_ATTR, &attr);EXPECT_EQ(uc_flic_attr_tests[i].getrc, !!rc)maybe you could just do:
EXPECT_EQ(!!uc_flic_attr_tests[i].geterrno, !!rc)
(unless I am missing something)
this is not super important, though
Yes, that should work. I will do that. Thanks!
TH_LOG("get dev attr rc not expected on %s %s (%i)",uc_flic_attr_tests[i].name,strerror(errno), errno);if (uc_flic_attr_tests[i].geterrno)EXPECT_EQ(uc_flic_attr_tests[i].geterrno, errno)TH_LOG("get dev attr errno not expected on %s %s (%i)",uc_flic_attr_tests[i].name,strerror(errno), errno);rc = ioctl(cd.fd, KVM_SET_DEVICE_ATTR, &attr);EXPECT_EQ(uc_flic_attr_tests[i].setrc, !!rc)TH_LOG("set sev attr rc not expected on %s %s (%i)",uc_flic_attr_tests[i].name,strerror(errno), errno);if (uc_flic_attr_tests[i].seterrno)EXPECT_EQ(uc_flic_attr_tests[i].seterrno, errno)TH_LOG("set dev attr errno not expected on %s %s (%i)",uc_flic_attr_tests[i].name,strerror(errno), errno);- }
- close(cd.fd);
+}
TEST_HARNESS_MAIN
Prevent null pointer dereference when processing KVM_IRQ_ROUTING_S390_ADAPTER routing entries. The ioctl cannot be processed for ucontrol VMs.
Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com --- arch/s390/kvm/interrupt.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c index 22d73c13e555..d4f031e086fc 100644 --- a/arch/s390/kvm/interrupt.c +++ b/arch/s390/kvm/interrupt.c @@ -2898,6 +2898,8 @@ int kvm_set_routing_entry(struct kvm *kvm, switch (ue->type) { /* we store the userspace addresses instead of the guest addresses */ case KVM_IRQ_ROUTING_S390_ADAPTER: + if (kvm_is_ucontrol(kvm)) + return -EINVAL; e->set = set_adapter_int; uaddr = gmap_translate(kvm->arch.gmap, ue->u.adapter.summary_addr); if (uaddr == -EFAULT)
Prevent null pointer dereference when processing KVM_IRQ_ROUTING_S390_ADAPTER routing entries. The ioctl cannot be processed for ucontrol VMs.
Fixes: f65470661f36 ("KVM: s390/interrupt: do not pin adapter interrupt pages") Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com ---
Added documentation and fixes comment.
--- Documentation/virt/kvm/api.rst | 3 +++ arch/s390/kvm/interrupt.c | 2 ++ 2 files changed, 5 insertions(+)
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index 454c2aaa155e..f15b61317aad 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -1914,6 +1914,9 @@ No flags are specified so far, the corresponding field must be set to zero. #define KVM_IRQ_ROUTING_HV_SINT 4 #define KVM_IRQ_ROUTING_XEN_EVTCHN 5
+On s390, adding a KVM_IRQ_ROUTING_S390_ADAPTER is rejected on ucontrol VMs with +error -EINVAL. + flags:
- KVM_MSI_VALID_DEVID: used along with KVM_IRQ_ROUTING_MSI routing entry diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c index 22d73c13e555..d4f031e086fc 100644 --- a/arch/s390/kvm/interrupt.c +++ b/arch/s390/kvm/interrupt.c @@ -2898,6 +2898,8 @@ int kvm_set_routing_entry(struct kvm *kvm, switch (ue->type) { /* we store the userspace addresses instead of the guest addresses */ case KVM_IRQ_ROUTING_S390_ADAPTER: + if (kvm_is_ucontrol(kvm)) + return -EINVAL; e->set = set_adapter_int; uaddr = gmap_translate(kvm->arch.gmap, ue->u.adapter.summary_addr); if (uaddr == -EFAULT)
Add a selftests for the interrupt routing configuration when using ucontrol VMs.
Calling the test may trigger a null pointer dereferences on kernels not containing the fixes in this patch series.
Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com --- .../selftests/kvm/s390x/ucontrol_test.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c index 972fac1023b5..d3a5dbfabade 100644 --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c @@ -785,4 +785,23 @@ TEST_F(uc_kvm, uc_flic_attrs) close(cd.fd); }
+TEST_F(uc_kvm, uc_set_gsi_routing) +{ + struct kvm_irq_routing *routing = kvm_gsi_routing_create(); + struct kvm_irq_routing_entry ue = { + .type = KVM_IRQ_ROUTING_S390_ADAPTER, + .gsi = 1, + .u.adapter = (struct kvm_irq_routing_s390_adapter) { + .ind_addr = 0, + }, + }; + int rc; + + routing->entries[0] = ue; + routing->nr = 1; + rc = ioctl(self->vm_fd, KVM_SET_GSI_ROUTING, routing); + ASSERT_EQ(-1, rc) TH_LOG("err %s (%i)", strerror(errno), errno); + ASSERT_EQ(EINVAL, errno) TH_LOG("err %s (%i)", strerror(errno), errno); +} + TEST_HARNESS_MAIN
In some rare situations a non default storage key is already set on the memory used by the test. Within normal VMs the key is reset / zapped when the memory is added to the VM. This is not the case for ucontrol VMs. With the initial iske check removed this test case can work in all situations. The function of the iske instruction is still validated by the remaining code.
Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com --- .../selftests/kvm/s390x/ucontrol_test.c | 22 +++++-------------- 1 file changed, 5 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c index d3a5dbfabade..755cd31e6387 100644 --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c @@ -88,10 +88,6 @@ asm("test_skey_asm:\n" " ahi %r0,1\n" " st %r1,0(%r5,%r6)\n"
- " iske %r1,%r6\n" - " ahi %r0,1\n" - " diag 0,0,0x44\n" - " sske %r1,%r6\n" " xgr %r1,%r1\n" " iske %r1,%r6\n" @@ -593,7 +589,9 @@ TEST_F(uc_kvm, uc_skey) ASSERT_EQ(true, uc_handle_exit(self)); ASSERT_EQ(1, sync_regs->gprs[0]);
- /* ISKE */ + /* SSKE + ISKE */ + sync_regs->gprs[1] = skeyvalue; + run->kvm_dirty_regs |= KVM_SYNC_GPRS; ASSERT_EQ(0, uc_run_once(self));
/* @@ -607,19 +605,9 @@ TEST_F(uc_kvm, uc_skey) TEST_ASSERT_EQ(ICPT_INST, sie_block->icptcode); TEST_REQUIRE(sie_block->ipa != 0xb229);
- /* ISKE contd. */ + /* SSKE + ISKE contd. */ ASSERT_EQ(false, uc_handle_exit(self)); ASSERT_EQ(2, sync_regs->gprs[0]); - /* assert initial skey (ACC = 0, R & C = 1) */ - ASSERT_EQ(0x06, sync_regs->gprs[1]); - uc_assert_diag44(self); - - /* SSKE + ISKE */ - sync_regs->gprs[1] = skeyvalue; - run->kvm_dirty_regs |= KVM_SYNC_GPRS; - ASSERT_EQ(0, uc_run_once(self)); - ASSERT_EQ(false, uc_handle_exit(self)); - ASSERT_EQ(3, sync_regs->gprs[0]); ASSERT_EQ(skeyvalue, sync_regs->gprs[1]); uc_assert_diag44(self);
@@ -628,7 +616,7 @@ TEST_F(uc_kvm, uc_skey) run->kvm_dirty_regs |= KVM_SYNC_GPRS; ASSERT_EQ(0, uc_run_once(self)); ASSERT_EQ(false, uc_handle_exit(self)); - ASSERT_EQ(4, sync_regs->gprs[0]); + ASSERT_EQ(3, sync_regs->gprs[0]); /* assert R reset but rest of skey unchanged */ ASSERT_EQ(skeyvalue & 0xfa, sync_regs->gprs[1]); ASSERT_EQ(0, sync_regs->gprs[1] & 0x04);
In some rare situations a non default storage key is already set on the memory used by the test. Within normal VMs the key is reset / zapped when the memory is added to the VM. This is not the case for ucontrol VMs. With the initial iske check removed this test case can work in all situations. The function of the iske instruction is still validated by the remaining code.
Fixes: 7d900f8ac191 ("selftests: kvm: s390: Add uc_skey VM test case") Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com ---
Added fixes comment.
--- .../selftests/kvm/s390x/ucontrol_test.c | 22 +++++-------------- 1 file changed, 5 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c index d3a5dbfabade..755cd31e6387 100644 --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c @@ -88,10 +88,6 @@ asm("test_skey_asm:\n" " ahi %r0,1\n" " st %r1,0(%r5,%r6)\n"
- " iske %r1,%r6\n" - " ahi %r0,1\n" - " diag 0,0,0x44\n" - " sske %r1,%r6\n" " xgr %r1,%r1\n" " iske %r1,%r6\n" @@ -593,7 +589,9 @@ TEST_F(uc_kvm, uc_skey) ASSERT_EQ(true, uc_handle_exit(self)); ASSERT_EQ(1, sync_regs->gprs[0]);
- /* ISKE */ + /* SSKE + ISKE */ + sync_regs->gprs[1] = skeyvalue; + run->kvm_dirty_regs |= KVM_SYNC_GPRS; ASSERT_EQ(0, uc_run_once(self));
/* @@ -607,19 +605,9 @@ TEST_F(uc_kvm, uc_skey) TEST_ASSERT_EQ(ICPT_INST, sie_block->icptcode); TEST_REQUIRE(sie_block->ipa != 0xb229);
- /* ISKE contd. */ + /* SSKE + ISKE contd. */ ASSERT_EQ(false, uc_handle_exit(self)); ASSERT_EQ(2, sync_regs->gprs[0]); - /* assert initial skey (ACC = 0, R & C = 1) */ - ASSERT_EQ(0x06, sync_regs->gprs[1]); - uc_assert_diag44(self); - - /* SSKE + ISKE */ - sync_regs->gprs[1] = skeyvalue; - run->kvm_dirty_regs |= KVM_SYNC_GPRS; - ASSERT_EQ(0, uc_run_once(self)); - ASSERT_EQ(false, uc_handle_exit(self)); - ASSERT_EQ(3, sync_regs->gprs[0]); ASSERT_EQ(skeyvalue, sync_regs->gprs[1]); uc_assert_diag44(self);
@@ -628,7 +616,7 @@ TEST_F(uc_kvm, uc_skey) run->kvm_dirty_regs |= KVM_SYNC_GPRS; ASSERT_EQ(0, uc_run_once(self)); ASSERT_EQ(false, uc_handle_exit(self)); - ASSERT_EQ(4, sync_regs->gprs[0]); + ASSERT_EQ(3, sync_regs->gprs[0]); /* assert R reset but rest of skey unchanged */ ASSERT_EQ(skeyvalue & 0xfa, sync_regs->gprs[1]); ASSERT_EQ(0, sync_regs->gprs[1] & 0x04);
Fixup the uc_attr_mem_limit test case to also cover the KVM_HAS_DEVICE_ATTR ioctl.
Signed-off-by: Christoph Schlameuss schlameuss@linux.ibm.com --- tools/testing/selftests/kvm/s390x/ucontrol_test.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kvm/s390x/ucontrol_test.c b/tools/testing/selftests/kvm/s390x/ucontrol_test.c index 755cd31e6387..e4a24dc7c7a6 100644 --- a/tools/testing/selftests/kvm/s390x/ucontrol_test.c +++ b/tools/testing/selftests/kvm/s390x/ucontrol_test.c @@ -206,10 +206,13 @@ TEST_F(uc_kvm, uc_attr_mem_limit) struct kvm_device_attr attr = { .group = KVM_S390_VM_MEM_CTRL, .attr = KVM_S390_VM_MEM_LIMIT_SIZE, - .addr = (unsigned long)&limit, + .addr = (u64)&limit, }; int rc;
+ rc = ioctl(self->vm_fd, KVM_HAS_DEVICE_ATTR, &attr); + EXPECT_EQ(0, rc); + rc = ioctl(self->vm_fd, KVM_GET_DEVICE_ATTR, &attr); EXPECT_EQ(0, rc); EXPECT_EQ(~0UL, limit);
linux-kselftest-mirror@lists.linaro.org
-
Christoph Schlameuss -
Claudio Imbrenda -
Janosch Frank