linux-kernel - Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CANgfPd_8SpHkCd=NyBKtRFWKkczx4SMxPLRon-kx9Oc6P7b=Ew@mail.gmail.com>
Date:   Wed, 7 Oct 2020 09:53:18 -0700
From:   Ben Gardon <bgardon@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     LKML <linux-kernel@...r.kernel.org>, kvm <kvm@...r.kernel.org>,
        Cannon Matthews <cannonmatthews@...gle.com>,
        Peter Xu <peterx@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Peter Shier <pshier@...gle.com>,
        Peter Feiner <pfeiner@...gle.com>,
        Junaid Shahid <junaids@...gle.com>,
        Jim Mattson <jmattson@...gle.com>,
        Yulei Zhang <yulei.kernel@...il.com>,
        Wanpeng Li <kernellwp@...il.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Xiao Guangrong <xiaoguangrong.eric@...il.com>
Subject: Re: [PATCH 15/22] kvm: mmu: Support changed pte notifier in tdp MMU

On Mon, Sep 28, 2020 at 8:11 AM Paolo Bonzini <pbonzini@...hat.com> wrote:
>
> On 25/09/20 23:22, Ben Gardon wrote:
> > +             *iter.sptep = 0;
> > +             handle_changed_spte(kvm, as_id, iter.gfn, iter.old_spte,
> > +                                 new_spte, iter.level);
> > +
>
> Can you explain why new_spte is passed here instead of 0?

That's just a bug. Thank you for catching it.

>
> All calls to handle_changed_spte are preceded by "*something =
> new_spte" except this one, so I'm thinking of having a change_spte
> function like
>
> static void change_spte(struct kvm *kvm, int as_id, gfn_t gfn,
>                         u64 *sptep, u64 new_spte, int level)
> {
>         u64 old_spte = *sptep;
>         *sptep = new_spte;
>
>         __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level);
>         handle_changed_spte_acc_track(old_spte, new_spte, level);
>         handle_changed_spte_dirty_log(kvm, as_id, gfn, old_spte, new_spte, level);
> }
>
> in addition to the previously-mentioned cleanup of always calling
> handle_changed_spte instead of special-casing calls to two of the
> three functions.  It would be a nice place to add the
> trace_kvm_mmu_set_spte tracepoint, too.

I'm not sure we can avoid special casing calls to the access tracking
and dirty logging handler functions. At least in the past that's
created bugs with things being marked dirty or accessed when they
shouldn't be. I'll revisit those assumptions. It would certainly be
nice to get rid of that complexity.

I agree that putting the SPTE assignment and handler functions in a
helper function would clean up the code. I'll do that. I got some
feedback on the RFC I sent last year which led me to open-code a lot
more, but I think this is still a good cleanup.

Re tracepoints, I was planning to just insert them all once this code
is stabilized, if that's alright.

>
> Paolo
>