Date:   Tue, 14 Dec 2021 03:12:33 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Ignat Korchagin <ignat@cloudflare.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        linux-kernel <linux-kernel@vger.kernel.org>, kvm@vger.kernel.org,
        bgardon@google.com, dmatlack@google.com, stevensd@chromium.org,
        kernel-team <kernel-team@cloudflare.com>
Subject: Re: [PATCH 0/2] KVM: x86: Fix dangling page reference in TDP MMU
Message-ID: <YbgLoZjy1zuhIgK7@google.com>
References: <20211213112514.78552-1-pbonzini@redhat.com>
 <CALrw=nEM6LEAD8LA1Bd15=8BK=TFwwwAMKy_DWRrDkD=r+1Tqg@mail.gmail.com>
 <Ybd5JJ/IZvcW/b2Y@google.com>
 <YbeiiT9b350lYBiR@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YbeiiT9b350lYBiR@google.com>
Precedence: bulk

On Mon, Dec 13, 2021, Sean Christopherson wrote:
> On Mon, Dec 13, 2021, Sean Christopherson wrote:
> > On Mon, Dec 13, 2021, Ignat Korchagin wrote:
> > > Unfortunately, this patchset does not fix the original issue reported in [1].
> > 
> > Can you provide your kernel config?  And any other version/config info that might
> > be relevant, e.g. anything in gvisor or runsc?
> 
> Scratch that, I've reproduced this, with luck I'll have a root cause by end of day.

Ok, the root cause is comically simple compared to all the theories we came up with.
If tdp_mmu_iter_cond_resched() drops mmu_lock and restarts the iterator, the
"continue" in the caller triggers tdp_iter_next().  tdp_iter_next() does what it's
told and advances the iterator.  Because all users call tdp_mmu_iter_cond_resched()
at the very beginning of the loop, this has the effect of skipping the current SPTE.

E.g. in the "zap all" case, where iter->level == iter->min_level == iter->root_level,
we effectively end up with code like this, which is obviously wrong once the
complexity of traversing a tree is simplified down to walking an array of SPTEs.

	gfn_t end = tdp_mmu_max_gfn_host();
        gfn_t start = 0;
        gfn_t last;

        for (i = last = start; i < end; i += 8, last = i) {
                if (cond_resched()) {
                        i = last;
                        continue;
                }

                sp = &root->spt[i];
                zap(sp);
        }

Patch incoming...