Hi Sean,
On 11/12/22 8:18 AM, Sean Christopherson wrote:
On Sat, Nov 12, 2022, Gavin Shan wrote:
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@xxxxxxxxxx> 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.
Ok.
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.
Well, sort of. I don't expect large mount of dirty pages will be tracked by
the backup bitmap :)
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+@xxxxxxxxxx
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.
Thanks for the details to explain. Right, It's true for WARN to catch KVM bugs
instead of user-triggerable bugs. I agree that none of WARN/printk/ratelimited_printk()
is applicable in this case.
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.
Ok. I've posted formal patch for 'next' branch, with the improved commit message.
Please take a look when you get a chance.
https://lore.kernel.org/kvmarm/20221112094322.21911-1-gshan@xxxxxxxxxx/T/#u
Thanks,
Gavin
