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

[Oliver Upton <oliver.upton@linux.dev>]

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?

> 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.

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.

> 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 feel like that
borders on contempt for the reader. I'm happy to leave list_del() as-is
but I'm going to insist on documenting destructor behavior in the
destructor.

-- 
Thanks,
Oliver