linux-kernel - Re: [Patch] KVM: x86/mmu: Make optimized __handle_changed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y9gH3OHA4ftegU7X@google.com>
Date:   Mon, 30 Jan 2023 18:09:32 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vipin Sharma <vipinsh@...gle.com>
Cc:     Ben Gardon <bgardon@...gle.com>,
        David Matlack <dmatlack@...gle.com>, pbonzini@...hat.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Patch] KVM: x86/mmu: Make optimized __handle_changed_spte() for
 clear dirty log

On Sat, Jan 28, 2023, Vipin Sharma wrote:
> On Wed, Jan 25, 2023 at 5:49 PM Sean Christopherson <seanjc@...gle.com> wrote:
> > -static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> > -                               u64 old_spte, u64 new_spte, int level,
> > -                               bool shared)
> > -{
> > -       __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level,
> > -                             shared);
> >         handle_changed_spte_acc_track(old_spte, new_spte, level);
> > -       handle_changed_spte_dirty_log(kvm, as_id, gfn, old_spte,
> > -                                     new_spte, level);
> > +
> > +       /* COMMENT GOES HERE. */
> 
> Current "shared" callers are not making a page dirty. If a new
> "shared" caller makes a page dirty then make sure
> handle_changed_spte_dirty_log is called.
> 
> How is this?

I was hoping for a more definitive "rule" than "KVM doesn't currently do XYZ".

> > +       if (!shared)
> > +               handle_changed_spte_dirty_log(kvm, as_id, gfn, old_spte,
> > +                                             new_spte, level);
> >  }
> >
> >  /*
> >   * tdp_mmu_set_spte_atomic - Set a TDP MMU SPTE atomically
> > - * and handle the associated bookkeeping.  Do not mark the page dirty
> > - * in KVM's dirty bitmaps.
> > + * and handle the associated bookkeeping.
> >   *
> >   * If setting the SPTE fails because it has changed, iter->old_spte will be
> >   * refreshed to the current value of the spte.

...

> > @@ -1703,9 +1657,11 @@ static void clear_dirty_pt_masked(struct kvm *kvm, struct kvm_mmu_page *root,
> >                                 new_spte = iter.old_spte & ~shadow_dirty_mask;
> >                         else
> >                                 continue;
> > +
> > +                       kvm_set_pfn_dirty(spte_to_pfn(iter.old_spte));
> >                 }
> >
> 
> Shouldn't we handle spte_ad_need_write_protect(iter.old_spte)
> separately and if this function returns true then on clearing
> PT_WRITABLE_MASK, kvm_set_pfn_dirty be called?
> My understanding is that the spte_ad_need_write_protect() will return
> true for nested VM sptes when PML mode is enabled.

Ah rats.  I missed that is_dirty_spte() checks WRITABLE in that case.  So yeah,
kvm_set_pfn_dirty() should be called in both paths.  I was thinking KVM would mark
the page dirty when faulting the PFN for write, but I have my flows all mixed up.