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

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