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

From: Oliver Upton <oliver.upton@linux.dev>
To: Marc Zyngier <maz@kernel.org>
Cc: kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 19/23] KVM: selftests: Add a minimal library for interacting with an ITS
Date: Wed, 14 Feb 2024 12:55:11 -0800	[thread overview]
Message-ID: <Zc0orzU-CeKEyx3j@linux.dev> (raw)
In-Reply-To: <86v86q4xkf.wl-maz@kernel.org>

On Wed, Feb 14, 2024 at 08:09:52PM +0000, Marc Zyngier wrote:
> > 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.

Ah, fair, I missed the documentation here. If we require userspace to
write CTLR last then we _should_ be fine, but damn is this a tricky set
of expectations.

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

Well, we would need to run all userspace reads and writes through the
cmd_lock in this case, which is what we already do for the CREADR
uaccess hook. To me the 'racy' queue accessors only make sense for guest
accesses, since the driver is expecting to poll for completion in that
case.

Otherwise we decide the existing rules for restoring the ITS are fine
and I get to keep my funky driver :)

-- 
Thanks,
Oliver