Re: [PATCH 01/20] KVM: Treat the device list as an rculist

[Sean Christopherson <seanjc@google.com>]

On Wed, Feb 28, 2024, Oliver Upton wrote:
> On Wed, Feb 28, 2024 at 09:18:28AM -0800, Sean Christopherson wrote:
> > On Tue, Feb 27, 2024, Oliver Upton wrote:
> > > A subsequent change to KVM/arm64 will necessitate walking the device
> > > list outside of the kvm->lock. Prepare by converting to an rculist.
> > > Note that this has zero effect the destruction path, as every reader
> > > should be protected by a valid reference on the KVM struct.
> > 
> > This should leave the destruction path alone then.  Simply using list_del_rcu()
> > doesn't convert this to an rculist, you need to actually synchronize against RCU
> > for this to have any meaning/protection.
> 
> Did anything about this diff give you the impression this wasn't
> thoroughly understood?

I'm taking exception to the "Prepare by converting to an rculist." statement.
This is not an RCU-protected list, it's a list that abuses rcu_list_add() and
list_for_each_rcu() to allow readers to run concurrently with insertion.  E.g.
IIUC, if it weren't for PROVE_RCU, the rcu_read_(un)lock() in the reader could
be omitted and everything would work just fine.

Ah, but it's a moot point, because kvm_device_release() does delete from the list,
and does not do so in an RCU-safe manner.  So that needs to be fixed, and then
this is indeed an RCU-protected list.

  static int kvm_device_release(struct inode *inode, struct file *filp)
  {
	struct kvm_device *dev = filp->private_data;
	struct kvm *kvm = dev->kvm;

	if (dev->ops->release) {
		mutex_lock(&kvm->lock);
		list_del(&dev->vm_node);
		dev->ops->release(dev);
		mutex_unlock(&kvm->lock);
	}

	kvm_put_kvm(kvm);
	return 0;
  }

> > The above alludes to this, and the
> > comment in kvm_destroy_devices() helps a little, but the code itself is actively
> > misleading.
> 
> Which is what comments are for. I deliberately chose to use a consistent
> programming model (albeit unnecessary to anyone who actually understands
> how this works) with the hope that _if_ someone comes along and needs to
> delete from the list later on they consider the requirements of RCU
> protection.

Ya, I see where you're coming from.

> list_del() or list_del_rcu() will fail in an equally-miserable manner if
> the previously stated expectation of readers is violated. Poisoning the
> forward pointer would be nice from a debugging POV, but readers could
> still hit a use-after-free.

My primary concern is not what happens on failure, I'm concerned about misleading
readers by implying that this is a proper RCU-protected list.  But as above, that's
a moot point.

As far as the failure mode, my preference is to poison the forward pointer.  It's
not just debug friendly; hitting a fault (#GP on x86) is a "safer" failure mode
than UAF, e.g. UAF could result in data corruption if the freed memory is
re-allocated before the rogue write happens.

> > I don't know how I feel about using list_add_rcu() in combination with list_del(),
> > but I like it a lot better than using list_del_rcu() in a way that is blatantly
> > wrong.
> > 
> > So if we don't have a better option, I would much rather do only the list_add_rcu(),
> > and add a comment _there_ explaining why KVM inserts with RCU protection, but
> > frees using regular ol' list_del().
> 
> Describing our destruction rules on the insertion path?

I was advocating we document the insertion rules.  The destruction rules for devices
aren't novel, they follow all of KVM's existing rules for destroying a VM. 

But again, moot point, because I was looking at this from the perspetive of a
list that isn't truly RCU-protected.  With this being a true RCU-protected list,
I agree that adding a comment to the insertion path would be redundant/unnecessary.

> I feel like that borders on contempt for the reader.

IMO, RCU is one of the most difficult things to use _safely_.  (Ab)Using RCU in
a way that _looks_ unsafe is setting people up to fail.

Bug once again, moot point :-)

> I'm happy to leave list_del() as-is but I'm going to insist on documenting
> destructor behavior in the destructor.

Ya.