KVM: arm64: Don't eagerly teardown the vgic on init error

As there is very little ordering in the KVM API, userspace can instanciate a half-baked GIC (missing its memory map, for example) at almost any time.

This means that, with the right timing, a thread running vcpu-0 can enter the kernel without a GIC configured and get a GIC created behind its back by another thread. Amusingly, it will pick up that GIC and start messing with the data structures without the GIC having been fully initialised.

Similarly, a thread running vcpu-1 can enter the kernel, and try to init the GIC that was previously created. Since this GIC isn't properly configured (no memory map), it fails to correctly initialise.

And that's the point where we decide to teardown the GIC, freeing all its resources. Behind vcpu-0's back. Things stop pretty abruptly, with a variety of symptoms. Clearly, this isn't good, we should be a bit more careful about this.

It is obvious that this guest is not viable, as it is missing some important part of its configuration. So instead of trying to tear bits of it down, let's just mark it as *dead*. It means that any further interaction from userspace will result in -EIO. The memory will be released on the "normal" path, when userspace gives up.

Cc [email protected]

Reported-by Alexander Potapenko <[email protected]>

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/arm.c            | 3 +++
 arch/arm64/kvm/vgic/vgic-init.c | 6 +++---
 2 files changed, 6 insertions(+), 3 deletions(-)

arch/arm64/kvm/arm.c +3 -0

index a0d01c46e4084..b97ada19f06a7 100644
@@ -997,6 +997,9 @@ static int kvm_vcpu_suspend(struct kvm_vcpu *vcpu)
 static int check_vcpu_requests(struct kvm_vcpu *vcpu)
 {
 	if (kvm_request_pending(vcpu)) {
+		if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu))
+			return -EIO;
+
 		if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
 			kvm_vcpu_sleep(vcpu);
 

arch/arm64/kvm/vgic/vgic-init.c +3 -3

index e7c53e8af3d16..c4cbf798e71a4 100644
@@ -536,10 +536,10 @@ int kvm_vgic_map_resources(struct kvm *kvm)
 out:
 	mutex_unlock(&kvm->arch.config_lock);
 out_slots:
-	mutex_unlock(&kvm->slots_lock);
-
 	if (ret)
-		kvm_vgic_destroy(kvm);
+		kvm_vm_dead(kvm);
+
+	mutex_unlock(&kvm->slots_lock);
 
 	return ret;
 }

Signature · 1 line

2.39.2

Quoted 9 lines · Marc Zyngier

As there is very little ordering in the KVM API, userspace can instanciate a half-baked GIC (missing its memory map, for example) at almost any time.

This means that, with the right timing, a thread running vcpu-0 can enter the kernel without a GIC configured and get a GIC created behind its back by another thread. Amusingly, it will pick up that GIC and start messing with the data structures without the GIC having been fully initialised.

Huh, I'm definitely missing something. Could you remind me where we open up this race between KVM_RUN && kvm_vgic_create()?

I'd thought the fact that the latter takes all the vCPU mutexes and checks if any vCPU in the VM has run would be enough to guard against such a race, but clearly not...

Quoted 14 lines

Similarly, a thread running vcpu-1 can enter the kernel, and try to init the GIC that was previously created. Since this GIC isn't properly configured (no memory map), it fails to correctly initialise.

And that's the point where we decide to teardown the GIC, freeing all its resources. Behind vcpu-0's back. Things stop pretty abruptly, with a variety of symptoms. Clearly, this isn't good, we should be a bit more careful about this.

It is obvious that this guest is not viable, as it is missing some important part of its configuration. So instead of trying to tear bits of it down, let's just mark it as *dead*. It means that any further interaction from userspace will result in -EIO. The memory will be released on the "normal" path, when userspace gives up.

Quoted 5 lines · Oliver Upton

Quoted 4 lines · Marc Zyngier

As there is very little ordering in the KVM API, userspace can instanciate a half-baked GIC (missing its memory map, for example) at almost any time.

Quoted 8 lines

Quoted 5 lines

This means that, with the right timing, a thread running vcpu-0 can enter the kernel without a GIC configured and get a GIC created behind its back by another thread. Amusingly, it will pick up that GIC and start messing with the data structures without the GIC having been fully initialised.

Huh, I'm definitely missing something. Could you remind me where we open up this race between KVM_RUN && kvm_vgic_create()?

Quoted 3 lines

I'd thought the fact that the latter takes all the vCPU mutexes and checks if any vCPU in the VM has run would be enough to guard against such a race, but clearly not...

Any chance that fixing bugs where vCPU0 can be accessed (and run!) before its fully online help?

E.g. if that closes the vCPU0 hole, maybe the vCPU1 case can be handled a bit more gracefully?

Quoted 6 lines · Sean Christopherson

Quoted 5 lines · Oliver Upton

Quoted 4 lines · Marc Zyngier

As there is very little ordering in the KVM API, userspace can instanciate a half-baked GIC (missing its memory map, for example) at almost any time.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

This means that, with the right timing, a thread running vcpu-0 can enter the kernel without a GIC configured and get a GIC created behind its back by another thread. Amusingly, it will pick up that GIC and start messing with the data structures without the GIC having been fully initialised.

Huh, I'm definitely missing something. Could you remind me where we open up this race between KVM_RUN && kvm_vgic_create()?

Ah, duh, I see it now. kvm_arch_vcpu_run_pid_change() doesn't serialize on a VM lock, and kvm_vgic_map_resources() has an early return for vgic_ready() letting it blow straight past the config_lock.

Then if we can't register the MMIO region for the distributor everything comes crashing down and a vCPU has made it into the KVM_RUN loop w/ the VGIC-shaped rug pulled out from under it. There's definitely another functional bug here where a vCPU's attempts to poke the

distributor wind up reaching userspace as MMIO exits. But we can worry about that another day.

If memory serves, kvm_vgic_map_resources() used to do all of this behind the config_lock to cure the race, but that wound up inverting lock ordering on srcu.

Note to self: Impose strict ordering on GIC initialization v. vCPU creation if/when we get a new flavor of irqchip.

Quoted 6 lines

Quoted 3 lines

I'd thought the fact that the latter takes all the vCPU mutexes and checks if any vCPU in the VM has run would be enough to guard against such a race, but clearly not...

Any chance that fixing bugs where vCPU0 can be accessed (and run!) before its fully online help?

That's an equally gross bug, but kvm_vgic_create() should still be safe w.r.t. vCPU creation since both hold the kvm->lock in the right spot. That is, since kvm_vgic_create() is called under the lock any vCPUs visible to userspace should exist in the vCPU xarray.

The crappy assumption here is kvm_arch_vcpu_run_pid_change() and its callees are allowed to destroy VM-scoped structures in error handling.

Quoted 2 lines

E.g. if that closes the vCPU0 hole, maybe the vCPU1 case can be handled a bit more gracefully?

I think this is about as graceful as we can be. The sorts of screw-ups that precipitate this error handling may involve stupidity across several KVM ioctls, meaning it is highly unlikely to be attributable / recoverable.

Signature · 2 lines

Thanks,
Oliver

Quoted 6 lines · Sean Christopherson

Quoted 5 lines · Oliver Upton

Quoted 4 lines · Marc Zyngier

As there is very little ordering in the KVM API, userspace can instanciate a half-baked GIC (missing its memory map, for example) at almost any time.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

This means that, with the right timing, a thread running vcpu-0 can enter the kernel without a GIC configured and get a GIC created behind its back by another thread. Amusingly, it will pick up that GIC and start messing with the data structures without the GIC having been fully initialised.

Huh, I'm definitely missing something. Could you remind me where we open up this race between KVM_RUN && kvm_vgic_create()?

Ah, duh, I see it now. kvm_arch_vcpu_run_pid_change() doesn't serialize on a VM lock, and kvm_vgic_map_resources() has an early return for vgic_ready() letting it blow straight past the config_lock.

Then if we can't register the MMIO region for the distributor everything comes crashing down and a vCPU has made it into the KVM_RUN loop w/ the VGIC-shaped rug pulled out from under it. There's definitely another functional bug here where a vCPU's attempts to poke the distributor wind up reaching userspace as MMIO exits. But we can worry about that another day.

If memory serves, kvm_vgic_map_resources() used to do all of this behind the config_lock to cure the race, but that wound up inverting lock ordering on srcu.

Note to self: Impose strict ordering on GIC initialization v. vCPU creation if/when we get a new flavor of irqchip.

Quoted 7 lines

Quoted 4 lines

I'd thought the fact that the latter takes all the vCPU mutexes and checks if any vCPU in the VM has run would be enough to guard against such a race, but clearly not...

Any chance that fixing bugs where vCPU0 can be accessed (and run!) before its fully online help?

That's an equally gross bug, but kvm_vgic_create() should still be safe w.r.t. vCPU creation since both hold the kvm->lock in the right spot. That is, since kvm_vgic_create() is called under the lock any vCPUs visible to userspace should exist in the vCPU xarray.

The crappy assumption here is kvm_arch_vcpu_run_pid_change() and its callees are allowed to destroy VM-scoped structures in error handling.

Quoted 2 lines

E.g. if that closes the vCPU0 hole, maybe the vCPU1 case can be handled a bit more gracefully?

I think this is about as graceful as we can be. The sorts of screw-ups that precipitate this error handling may involve stupidity across several KVM ioctls, meaning it is highly unlikely to be attributable / recoverable.

Quoted 6 lines · Oliver Upton

Then if we can't register the MMIO region for the distributor everything comes crashing down and a vCPU has made it into the KVM_RUN loop w/ the VGIC-shaped rug pulled out from under it. There's definitely another functional bug here where a vCPU's attempts to poke the distributor wind up reaching userspace as MMIO exits. But we can worry about that another day.

I don't think that one is that bad. Userspace got us here, and they now see an MMIO exit for something that it is not prepared to handle. Suck it up and die (on a black size M t-shirt, please).

Quoted 3 lines

If memory serves, kvm_vgic_map_resources() used to do all of this behind the config_lock to cure the race, but that wound up inverting lock ordering on srcu.

Probably something like that. We also used to hold the kvm lock, which made everything much simpler, but awfully wrong.

Quoted 2 lines

Note to self: Impose strict ordering on GIC initialization v. vCPU creation if/when we get a new flavor of irqchip.

One of the things we should have done when introducing GICv3 is to impose that at KVM_DEV_ARM_VGIC_CTRL_INIT, the GIC memory map is final. I remember some push-back on the QEMU side of things, as they like to decouple things, but this has proved to be a nightmare.

Quoted 2 lines

The crappy assumption here is kvm_arch_vcpu_run_pid_change() and its callees are allowed to destroy VM-scoped structures in error handling.

I think this is symptomatic of more general issue: we perform VM-wide configuration in the context of a vcpu. We have tons of this stuff to paper over the lack of a "this VM is fully configured" barrier.

I wonder whether we could sidestep things by punting the finalisation of the VM to a different context (workqueue?) and simply return -EAGAIN or -EINTR to userspace while we're processing it. That doesn't solve the "I'm missing parts of the address map and I'm going to die" part though.

Quoted 11 lines · Marc Zyngier

Quoted 6 lines · Oliver Upton

Then if we can't register the MMIO region for the distributor everything comes crashing down and a vCPU has made it into the KVM_RUN loop w/ the VGIC-shaped rug pulled out from under it. There's definitely another functional bug here where a vCPU's attempts to poke the distributor wind up reaching userspace as MMIO exits. But we can worry about that another day.

I don't think that one is that bad. Userspace got us here, and they now see an MMIO exit for something that it is not prepared to handle. Suck it up and die (on a black size M t-shirt, please).

LOL, I'll remember that.

The situation I have in mind is a bit harder to blame on userspace, though. Supposing that the whole VM was set up correctly, multiple vCPUs entering KVM_RUN concurrently could cause this race and have 'unexpected' MMIO exits go out to userspace.

	vcpu-0				vcpu-1
	======				======
	kvm_vgic_map_resources()
	  dist->ready = true
	  mutex_unlock(config_lock)
	  				kvm_vgic_map_resources()
					  if (vgic_ready())
					    return 0

					< enter guest >
					typer = writel(0, GICD_CTLR)

					< data abort >
					kvm_io_bus_write(...)	<= No GICD, out to userspace

       vgic_register_dist_iodev()

A small but stupid window to race with.

Quoted 11 lines

Quoted 4 lines

If memory serves, kvm_vgic_map_resources() used to do all of this behind the config_lock to cure the race, but that wound up inverting lock ordering on srcu.

Probably something like that. We also used to hold the kvm lock, which made everything much simpler, but awfully wrong.

Quoted 2 lines

Note to self: Impose strict ordering on GIC initialization v. vCPU creation if/when we get a new flavor of irqchip.

Quoted 4 lines

One of the things we should have done when introducing GICv3 is to impose that at KVM_DEV_ARM_VGIC_CTRL_INIT, the GIC memory map is final. I remember some push-back on the QEMU side of things, as they like to decouple things, but this has proved to be a nightmare.

Pushing more of the initialization complexity into userspace feels like the right thing. Since we clearly have no idea what we're doing :)

Quoted 8 lines

Quoted 3 lines

The crappy assumption here is kvm_arch_vcpu_run_pid_change() and its callees are allowed to destroy VM-scoped structures in error handling.

I think this is symptomatic of more general issue: we perform VM-wide configuration in the context of a vcpu. We have tons of this stuff to paper over the lack of a "this VM is fully configured" barrier.

Quoted 5 lines

I wonder whether we could sidestep things by punting the finalisation of the VM to a different context (workqueue?) and simply return -EAGAIN or -EINTR to userspace while we're processing it. That doesn't solve the "I'm missing parts of the address map and I'm going to die" part though.

Throwing it back at userspace would be nice, but unfortunately for ABI I think we need to block/spin vCPUs in the kernel til the VM is in fully working condition. A fragile userspace could explode for a 'spurious' EAGAIN/EINTR where there wasn't one before.

Quoted 1 line · Oliver Upton

A small but stupid window to race with.

Ah, gotcha. I guess getting rid of the early-out in kvm_vgic_map_resources() would plug that one. Want to post a fix for that?

Quoted 16 lines

Quoted 15 lines

Quoted 4 lines

If memory serves, kvm_vgic_map_resources() used to do all of this behind the config_lock to cure the race, but that wound up inverting lock ordering on srcu.

Probably something like that. We also used to hold the kvm lock, which made everything much simpler, but awfully wrong.

Quoted 2 lines

Note to self: Impose strict ordering on GIC initialization v. vCPU creation if/when we get a new flavor of irqchip.

One of the things we should have done when introducing GICv3 is to impose that at KVM_DEV_ARM_VGIC_CTRL_INIT, the GIC memory map is final. I remember some push-back on the QEMU side of things, as they like to decouple things, but this has proved to be a nightmare.

Quoted 2 lines

Pushing more of the initialization complexity into userspace feels like the right thing. Since we clearly have no idea what we're doing :)

KVM APIv2?

Quoted 14 lines

Quoted 13 lines

Quoted 3 lines

The crappy assumption here is kvm_arch_vcpu_run_pid_change() and its callees are allowed to destroy VM-scoped structures in error handling.

I think this is symptomatic of more general issue: we perform VM-wide configuration in the context of a vcpu. We have tons of this stuff to paper over the lack of a "this VM is fully configured" barrier.

I wonder whether we could sidestep things by punting the finalisation of the VM to a different context (workqueue?) and simply return -EAGAIN or -EINTR to userspace while we're processing it. That doesn't solve the "I'm missing parts of the address map and I'm going to die" part though.

Quoted 4 lines

Throwing it back at userspace would be nice, but unfortunately for ABI I think we need to block/spin vCPUs in the kernel til the VM is in fully working condition. A fragile userspace could explode for a 'spurious' EAGAIN/EINTR where there wasn't one before.

EINTR needs to be handled already, as this is how you report preemption by a signal.

As there is very little ordering in the KVM API, userspace can instanciate a half-baked GIC (missing its memory map, for example) at almost any time.

This means that, with the right timing, a thread running vcpu-0 can enter the kernel without a GIC configured and get a GIC created behind its back by another thread. Amusingly, it will pick up that GIC and start messing with the data structures without the GIC having been fully initialised.

[...]

As there is very little ordering in the KVM API, userspace can instanciate a half-baked GIC (missing its memory map, for example) at almost any time.

[1 <text/plain; us-ascii (7bit)>]

Quoted 3 lines · Marc Zyngier

As there is very little ordering in the KVM API, userspace can instanciate a half-baked GIC (missing its memory map, for example) at almost any time.

The vgic_init selftest has started failing in mainline on multiple platforms, with a bisect pointing at this patch which is present there as commit df5fd75ee305cb5. The test reports:

# selftests: kvm: vgic_init
# Random seed: 0x6b8b4567
# Running GIC_v3 tests.
# ==== Test Assertion Failure ====
# lib/kvm_util.c:724: false
# pid=1947 tid=1947 errno=5 - Input/output error

#      1	0x0000000000404edb: __vm_mem_region_delete at kvm_util.c:724 (discriminator 5)
#      2	0x0000000000405d0b: kvm_vm_free at kvm_util.c:762 (discriminator 12)
#      3	0x0000000000402d5f: vm_gic_destroy at vgic_init.c:101
#      4	 (inlined by) test_vcpus_then_vgic at vgic_init.c:368
#      5	 (inlined by) run_tests at vgic_init.c:720
#      6	0x0000000000401a6f: main at vgic_init.c:748
#      7	0x0000ffffa7b37543: ?? ??:0
#      8	0x0000ffffa7b37617: ?? ??:0
#      9	0x0000000000401b6f: _start at ??:?

# KVM killed/bugged the VM, check the kernel log for clues
not ok 10 selftests: kvm: vgic_init # exit=254

which does rather look like a test bug rather than a problem in the change itself.

Quoted 14 lines · Mark Brown

# ==== Test Assertion Failure ====
# lib/kvm_util.c:724: false
# pid=1947 tid=1947 errno=5 - Input/output error

#      1	0x0000000000404edb: __vm_mem_region_delete at kvm_util.c:724 (discriminator 5)
#      2	0x0000000000405d0b: kvm_vm_free at kvm_util.c:762 (discriminator 12)
#      3	0x0000000000402d5f: vm_gic_destroy at vgic_init.c:101
#      4	 (inlined by) test_vcpus_then_vgic at vgic_init.c:368
#      5	 (inlined by) run_tests at vgic_init.c:720
#      6	0x0000000000401a6f: main at vgic_init.c:748
#      7	0x0000ffffa7b37543: ?? ??:0
#      8	0x0000ffffa7b37617: ?? ??:0
#      9	0x0000000000401b6f: _start at ??:?

# KVM killed/bugged the VM, check the kernel log for clues
not ok 10 selftests: kvm: vgic_init # exit=254

Quoted 2 lines

which does rather look like a test bug rather than a problem in the change itself.

Well, the test tries to do braindead things, and then the test infrastructure seems surprised that KVM tells it to bugger off...

I can paper over it with this (see below), but frankly, someone who actually cares about this crap should take a look (and ownership).

Quoted 16 lines · Marc Zyngier

Quoted 14 lines · Mark Brown

# ==== Test Assertion Failure ====
# lib/kvm_util.c:724: false
# pid=1947 tid=1947 errno=5 - Input/output error

#      1	0x0000000000404edb: __vm_mem_region_delete at kvm_util.c:724 (discriminator 5)
#      2	0x0000000000405d0b: kvm_vm_free at kvm_util.c:762 (discriminator 12)
#      3	0x0000000000402d5f: vm_gic_destroy at vgic_init.c:101
#      4	 (inlined by) test_vcpus_then_vgic at vgic_init.c:368
#      5	 (inlined by) run_tests at vgic_init.c:720
#      6	0x0000000000401a6f: main at vgic_init.c:748
#      7	0x0000ffffa7b37543: ?? ??:0
#      8	0x0000ffffa7b37617: ?? ??:0
#      9	0x0000000000401b6f: _start at ??:?

# KVM killed/bugged the VM, check the kernel log for clues
not ok 10 selftests: kvm: vgic_init # exit=254

Quoted 8 lines

Quoted 2 lines

which does rather look like a test bug rather than a problem in the change itself.

Well, the test tries to do braindead things, and then the test infrastructure seems surprised that KVM tells it to bugger off...

I can paper over it with this (see below), but frankly, someone who actually cares about this crap should take a look (and ownership).

I'm not even sure that's a terrible fix, looking at the changelog I get the impression the test is deliberately looking to do problematic things with the goal of making sure that the kernel handles them appropriately. That's not interacting well with the KVM selftest framework's general assert early assert often approach but it's a reasonable thing to want

to test so relaxing the asserts like this is one way of squaring the circile.

Quoted 5 lines · Mark Brown

I'm not even sure that's a terrible fix, looking at the changelog I get the impression the test is deliberately looking to do problematic things with the goal of making sure that the kernel handles them appropriately. That's not interacting well with the KVM selftest framework's general assert early assert often approach but it's a reasonable thing to want

Can you elaborate on the "assert early assert often approach". What shall this test rather do according to you?

I am OoO next week but I can have a look afterwards. On which machine is it failing?

Quoted 10 lines · Eric Auger

Quoted 5 lines · Mark Brown

I'm not even sure that's a terrible fix, looking at the changelog I get the impression the test is deliberately looking to do problematic things with the goal of making sure that the kernel handles them appropriately. That's not interacting well with the KVM selftest framework's general assert early assert often approach but it's a reasonable thing to want

Can you elaborate on the "assert early assert often approach". What shall this test rather do according to you?

In general the KVM selftests are filled with asserts which just immediately cause the test to exit with a backtrace. That's certainly an approach that can be taken with testsuites, but it does make things very fagile. This means that if the test is deliberately doing something which is liable to cause errors and put the VM in a bad state then it seems relatively likely that some part of a partial cleanup will run into a spurious error caused by the earlier testing putting the VM in an error state.

Quoted 3 lines

I am OoO next week but I can have a look afterwards. On which machine is it failing?

It was failing on a wide range of arm64 machines, I think every one I test (which would track with the change being very generic).

[1 <text/plain; us-ascii (7bit)>]

Quoted 17 lines · Marc Zyngier

Quoted 14 lines · Mark Brown

# ==== Test Assertion Failure ====
# lib/kvm_util.c:724: false
# pid=1947 tid=1947 errno=5 - Input/output error

#      1	0x0000000000404edb: __vm_mem_region_delete at kvm_util.c:724 (discriminator 5)
#      2	0x0000000000405d0b: kvm_vm_free at kvm_util.c:762 (discriminator 12)
#      3	0x0000000000402d5f: vm_gic_destroy at vgic_init.c:101
#      4	 (inlined by) test_vcpus_then_vgic at vgic_init.c:368
#      5	 (inlined by) run_tests at vgic_init.c:720
#      6	0x0000000000401a6f: main at vgic_init.c:748
#      7	0x0000ffffa7b37543: ?? ??:0
#      8	0x0000ffffa7b37617: ?? ??:0
#      9	0x0000000000401b6f: _start at ??:?

# KVM killed/bugged the VM, check the kernel log for clues
not ok 10 selftests: kvm: vgic_init # exit=254

Quoted 8 lines

Quoted 2 lines

which does rather look like a test bug rather than a problem in the change itself.

Well, the test tries to do braindead things, and then the test infrastructure seems surprised that KVM tells it to bugger off...

I can paper over it with this (see below), but frankly, someone who actually cares about this crap should take a look (and ownership).

I'm not even sure that's a terrible fix, looking at the changelog I get the impression the test is deliberately looking to do problematic things with the goal of making sure that the kernel handles them appropriately. That's not interacting well with the KVM selftest framework's general assert early assert often approach but it's a reasonable thing to want to test so relaxing the asserts like this is one way of squaring the circile.

Quoted 7 lines · Mark Brown

I'm not even sure that's a terrible fix, looking at the changelog I get the impression the test is deliberately looking to do problematic things with the goal of making sure that the kernel handles them appropriately. That's not interacting well with the KVM selftest framework's general assert early assert often approach but it's a reasonable thing to want to test so relaxing the asserts like this is one way of squaring the circile.

It *is* a terrible fix, since it makes no effort in finding out whether the VM is be dead for a good or a bad reason. In a way, the fix is worse than the current error, because it silently hide the crap.