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

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

On Wed, Feb 28, 2024 at 11:13:52AM -0800, Sean Christopherson wrote:
> 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.

Fair, if we wanted to be exact about it this change relies on the
pointer assignment properties of RCU, but not necessarily full-blown RCU
protection. But...

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

Shows how much thought I threw at the problem :D Nice catch, I'll fold
in a fix next time around.

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

Same -- what I was getting at here is that the moment our house of cards
falls apart we cannot guarantee all readers have seen the poison
(instead of the stale ->next pointer), so failure patterns are not
predictable.

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

Fair enough. Coming along and saying "I know better, let's do X" is
equally (if not more) contemptuous :)

-- 
Thanks,
Oliver