Re: [PATCH v10 3/7] KVM: Support dirty ring in conjunction with bitmap

[Sean Christopherson <seanjc@google.com>]

On Sat, Nov 12, 2022, Gavin Shan wrote:
> Hi Sean,
> 
> On 11/12/22 7:00 AM, Sean Christopherson wrote:
> > On Sat, Nov 12, 2022, Gavin Shan wrote:
> > > On 11/11/22 11:19 PM, Marc Zyngier wrote:
> > > > On Thu, 10 Nov 2022 23:47:41 +0000,
> > > > Gavin Shan <gshan@redhat.com> wrote:
> > > > But that I don't get. Or rather, I don't get the commit message that
> > > > matches this hunk. Do we want to catch the case where all of the
> > > > following are true:
> > > > 
> > > > - we don't have a vcpu,
> > > > - we're allowed to log non-vcpu dirtying
> > > > - we *only* have the ring?
> > 
> > As written, no, because the resulting WARN will be user-triggerable.  As mentioned
> > earlier in the thread[*], if ARM rejects KVM_DEV_ARM_ITS_SAVE_TABLES when dirty
> > logging is enabled with a bitmap, then this code can WARN.
> > 
> 
> I assume you're saying to reject the command when dirty ring is enabled
> __without__ a bitmap. vgic/its is the upper layer of dirty dirty.

I was stating that that is an option.  I was not opining anything, I truly don't
care whether or not KVM_DEV_ARM_ITS_SAVE_TABLES is rejected.

> To me, it's a bad idea for the upper layer needs to worry too much about the
> lower layer.

That ship sailed when we added kvm_arch_allow_write_without_running_vcpu().
Arguably, it sailed when the dirty ring was added, which solidified the requirement
that writing guest memory "must" be done with a running vCPU.

> > > > If so, can we please capture that in the commit message?
> > > > 
> > > 
> > > Nice catch! This particular case needs to be warned explicitly. Without
> > > the patch, kernel crash is triggered. With this patch applied, the error
> > > or warning is dropped silently. We either check memslot->dirty_bitmap
> > > in mark_page_dirty_in_slot(), or check it in kvm_arch_allow_write_without_running_vcpu().
> > > I personally the later one. Let me post a formal patch on top of your
> > > 'next' branch where the commit log will be improved accordingly.
> > 
> > As above, a full WARN is not a viable option unless ARM commits to rejecting
> > KVM_DEV_ARM_ITS_SAVE_TABLES in this scenario.  IMO, either reject the ITS save
> > or silently ignore the goof.  Adding a pr_warn_ratelimited() to alert the user
> > that they shot themselves in the foot after the fact seems rather pointless if
> > KVM could have prevented the self-inflicted wound in the first place.
> > 
> > [*] https://lore.kernel.org/all/Y20q3lq5oc2gAqr+@google.com
> > 
> 
> Without a message printed by WARN, kernel crash or pr_warn_ratelimited(), it
> will be hard for userspace to know what's going on, because the dirty bits
> have been dropped silently.I think we still survive since we have WARN
> message for other known cases where no running vcpu context exists.

That WARN is to catch KVM bugs.  No KVM bugs, no WARN.  WARNs must not be user
triggerable in the absence of kernel bugs.  This is a kernel rule, not a KVM thing,
e.g. see panic_on_warn.

printk() is useless for running at any kind of scale as userspace can't take action
on "failure", e.g. unless userspace has a priori knowledge of the _exact_ error
message then human intervention is required (there are other issues as well).

A ratelimited printk() makes things even worse because then a failing VM may not
get its "failure" logged, i.e. the printk() is even less actionable.

And user triggerable printks() need to be ratelimited to prevent a malicious or
broken userspace from flooding the kernel log.  Thus, this "failure" would need
to be ratelimited, making it all but useless for anyone but developers.

> So if I'm correct, what we need to do is to improve the commit message to
> address Marc's concerns here? :)

Yes, Marc is saying that it's not strictly wrong for userspace to not dirty log
the ITS save, so rejecting KVM_DEV_ARM_ITS_SAVE_TABLES is a bad option.