Re: [patch 1/4] KVM: MMU audit: update count_writable_mappings / count_rmaps

Avi Kivity
2009-06-08T09:24:09Z

Marcelo Tosatti wrote:
> Under testing, count_writable_mappings returns a value that is 2 integers
> larger than what count_rmaps returns.
>
> Suspicion is that either of the two functions is counting a duplicate (either
> positively or negatively).
>
> Modifying check_writable_mappings_rmap to check for rmap existance on
> all present MMU pages fails to trigger an error, which should keep Avi
> happy.
>
> Also introduce mmu_spte_walk to invoke a callback on all present sptes visible
> to the current vcpu, might be useful in the future.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Index: kvm/arch/x86/kvm/mmu.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/mmu.c
> +++ kvm/arch/x86/kvm/mmu.c
> @@ -3017,6 +3017,55 @@ static gva_t canonicalize(gva_t gva)
> return gva;
> }
>
> +
> +typedef void (*inspect_spte_fn) (struct kvm *kvm, struct kvm_mmu_page *sp,
> + u64 *sptep);
> +
> +static void __mmu_spte_walk(struct kvm *kvm, struct kvm_mmu_page *sp,
> + inspect_spte_fn fn)
> +{
> + int i;
> +
> + for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
> + u64 ent = sp->spt[i];
> +
> + if (is_shadow_present_pte(ent)) {
> + if (sp->role.level > 1) {
>

I think this is broken wrt large pages. We should recurse if role.level
> 1 or the G bit is set.

> + if (*sptep & PT_WRITABLE_MASK) {
> + rev_sp = page_header(__pa(sptep));
> + gfn = rev_sp->gfns[sptep - rev_sp->spt];
> +
> + if (!gfn_to_memslot(kvm, gfn)) {
> + printk(KERN_ERR "%s: no memslot for gfn %ld\n",
> + audit_msg, gfn);
> + printk(KERN_ERR "%s: index %ld of sp (gfn=%lx)\n",
> + audit_msg, sptep - rev_sp->spt,
> + rev_sp->gfn);
> + dump_stack();
> + return;
> + }
> +
> + rmapp = gfn_to_rmap(kvm, rev_sp->gfns[sptep - rev_sp->spt], 0);
> + if (!*rmapp) {
> + printk(KERN_ERR "%s: no rmap for writable spte %llx\n",
> + audit_msg, *sptep);
> + dump_stack();
> + }
> + }
>

Semi-related: we should set up a new exit code to halt the VM so it can
be inspected, otherwise all those printks and dump_stack()s will quickly
overwhelm the logging facilities.

--
error compiling committee.c: too many arguments to function