Re: [PATCH v2 19/23] KVM: selftests: Add a minimal library for interacting with an ITS

[Marc Zyngier <maz@kernel.org>]

On Wed, 14 Feb 2024 19:00:00 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Wed, Feb 14, 2024 at 05:32:25PM +0000, Marc Zyngier wrote:
> > On Tue, 13 Feb 2024 09:41:14 +0000,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > A prerequisite of testing LPI injection performance is of course
> > > instantiating an ITS for the guest. Add a small library for creating an
> > > ITS and interacting with it *from userspace*.
> > > 
> > > Yep, you read that right. KVM unintentionally allows userspace to send
> > > commands to the virtual ITS via the command queue. Besides adding test
> > > coverage for an elusive UAPI, interacting with the ITS in userspace
> > > simplifies the handling of commands that need to allocate memory, like a
> > > MAPD command with an ITT.
> > 
> > I don't mean to derail the party, but I really think we should plug
> > this hole. Either that, or we make it an official interface for state
> > restore. And don't we all love to have multiple interfaces to do the
> > same thing?
> 
> Ok, I've thought about it a bit more and I'm fully convinced we need to
> shut the door on this stupidity.
> 
> We expect CREADR == CWRITER at the time userspace saves the ITS
> registers, but we have a *hideous* ordering issue on the restore path.
> 
> If the order of restore from userspace is CBASER, CWRITER, CREADR then
> we **wind up replaying the entire command queue**. While insane, I'm
> pretty sure it is legal for the guest to write garbage after the read
> pointer has moved past a particular command index.
> 
> Fsck!!!

This is documented Documentation/virt/kvm/devices/arm-vgic-its.rst to
some extent, and it is allowed for the guest to crap itself on behalf
of userspace if the ordering isn't respected.

> So, how about we do this:
> 
>  - Provide a uaccess hook for CWRITER that changes the write-pointer
>    without processing any commands
> 
>  - Assert an invariant that at any time CWRITER or CREADR are read from
>    userspace that CREADR == CWRITER. Fail the ioctl and scream if that
>    isn't the case, so that way we never need to worry about processing
>    'in-flight' commands at the destination.

Are we guaranteed that we cannot ever see CWRITER != CREADR at VM
dumping time? I'm not convinced that we cannot preempt the vcpu thread
at the right spot, specially given that you can have an arbitrary
large batch of commands to execute.

Just add a page-fault to the mix, and a signal pending. Pronto, you
see a guest exit and you should be able to start dumping things
without the ITS having processed much. I haven't tried, but that
doesn't seem totally unlikely.

	M.

-- 
Without deviation from the norm, progress is not possible.