KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

[RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

Posted by Will Deacon 8 months, 3 weeks ago

kvm_pgtable_stage2_free_unlinked() does the final put_page() on the
root page of the sub-tree before returning, so remove the additional
put_page() invocations in the callers.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>
Cc: Ricardo Koller <ricarkol@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---

Hi folks,

Sending this as an RFC as I only spotted it from code inspection and I'm
surprised others aren't seeing fireworks if it's a genuine bug. I also
couldn't come up with a sensible Fixes tag, as all of:

 e7c05540c694b ("KVM: arm64: Add helper for creating unlinked stage2 subtrees")
 8f5a3eb7513fc ("KVM: arm64: Add kvm_pgtable_stage2_split()")
 f6a27d6dc51b2 ("KVM: arm64: Drop last page ref in kvm_pgtable_stage2_free_removed()")

are actually ok in isolation. Hrm. Please tell me I'm wrong?

 arch/arm64/kvm/hyp/pgtable.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index c651df904fe3..ab9d05fcf98b 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1419,7 +1419,6 @@ kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt,
 				 level + 1);
 	if (ret) {
 		kvm_pgtable_stage2_free_unlinked(mm_ops, pgtable, level);
-		mm_ops->put_page(pgtable);
 		return ERR_PTR(ret);
 	}
 
@@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
 
 	if (!stage2_try_break_pte(ctx, mmu)) {
 		kvm_pgtable_stage2_free_unlinked(mm_ops, childp, level);
-		mm_ops->put_page(childp);
 		return -EAGAIN;
 	}
 
-- 
2.43.0.687.g38aa6559b0-goog

Re: [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

Posted by Marc Zyngier 8 months, 2 weeks ago

On Mon, 12 Feb 2024 19:30:52 +0000, Will Deacon wrote:
> kvm_pgtable_stage2_free_unlinked() does the final put_page() on the
> root page of the sub-tree before returning, so remove the additional
> put_page() invocations in the callers.

Applied to fixes, thanks!

[1/1] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()
      commit: c60d847be7b8e69e419e02a2b3d19c2842a3c35d

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

Posted by Oliver Upton 8 months, 2 weeks ago

On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote:
> kvm_pgtable_stage2_free_unlinked() does the final put_page() on the
> root page of the sub-tree before returning, so remove the additional
> put_page() invocations in the callers.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: Ricardo Koller <ricarkol@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
> 
> Hi folks,
> 
> Sending this as an RFC as I only spotted it from code inspection and I'm
> surprised others aren't seeing fireworks if it's a genuine bug. I also
> couldn't come up with a sensible Fixes tag, as all of:
> 
>  e7c05540c694b ("KVM: arm64: Add helper for creating unlinked stage2 subtrees")
>  8f5a3eb7513fc ("KVM: arm64: Add kvm_pgtable_stage2_split()")
>  f6a27d6dc51b2 ("KVM: arm64: Drop last page ref in kvm_pgtable_stage2_free_removed()")
> 
> are actually ok in isolation. Hrm. Please tell me I'm wrong?
> 
>  arch/arm64/kvm/hyp/pgtable.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index c651df904fe3..ab9d05fcf98b 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1419,7 +1419,6 @@ kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt,
>  				 level + 1);
>  	if (ret) {
>  		kvm_pgtable_stage2_free_unlinked(mm_ops, pgtable, level);
> -		mm_ops->put_page(pgtable);
>  		return ERR_PTR(ret);
>  	}

AFAICT, this entire branch is effectively dead code, unless there's a
KVM bug lurking behind the page table walk. The sub-tree isn't visible
to other software or hardware walkers yet, so none of the PTE races
could cause this to pop.

So while this is very obviously a bug, it might be pure luck that folks
haven't seen smoke here. Perhaps while fixing the bug we should take the
opportunity to promote the condition to WARN_ON_ONCE().

> @@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
>  
>  	if (!stage2_try_break_pte(ctx, mmu)) {
>  		kvm_pgtable_stage2_free_unlinked(mm_ops, childp, level);
> -		mm_ops->put_page(childp);
>  		return -EAGAIN;
>  	}

This, on the other hand, seems possible. There exists a race where an
old block PTE could have the AF set on it and the underlying cmpxchg()
could fail. There shouldn't be a race with any software walkers, as we
hold the MMU lock for write here.

-- 
Thanks,
Oliver

Re: [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

Posted by Marc Zyngier 8 months, 2 weeks ago

On Mon, 12 Feb 2024 20:14:37 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote:
> > kvm_pgtable_stage2_free_unlinked() does the final put_page() on the
> > root page of the sub-tree before returning, so remove the additional
> > put_page() invocations in the callers.
> > 
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Oliver Upton <oliver.upton@linux.dev>
> > Cc: Ricardo Koller <ricarkol@google.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> > 
> > Hi folks,
> > 
> > Sending this as an RFC as I only spotted it from code inspection and I'm
> > surprised others aren't seeing fireworks if it's a genuine bug. I also
> > couldn't come up with a sensible Fixes tag, as all of:
> > 
> >  e7c05540c694b ("KVM: arm64: Add helper for creating unlinked stage2 subtrees")
> >  8f5a3eb7513fc ("KVM: arm64: Add kvm_pgtable_stage2_split()")
> >  f6a27d6dc51b2 ("KVM: arm64: Drop last page ref in kvm_pgtable_stage2_free_removed()")

I'd blame it on the last commit, as we really ought to have it if we
have the others.

> >
> > are actually ok in isolation. Hrm. Please tell me I'm wrong?
> > 
> >  arch/arm64/kvm/hyp/pgtable.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > index c651df904fe3..ab9d05fcf98b 100644
> > --- a/arch/arm64/kvm/hyp/pgtable.c
> > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > @@ -1419,7 +1419,6 @@ kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt,
> >  				 level + 1);
> >  	if (ret) {
> >  		kvm_pgtable_stage2_free_unlinked(mm_ops, pgtable, level);
> > -		mm_ops->put_page(pgtable);
> >  		return ERR_PTR(ret);
> >  	}
> 
> AFAICT, this entire branch is effectively dead code, unless there's a
> KVM bug lurking behind the page table walk. The sub-tree isn't visible
> to other software or hardware walkers yet, so none of the PTE races
> could cause this to pop.
> 
> So while this is very obviously a bug, it might be pure luck that folks
> haven't seen smoke here. Perhaps while fixing the bug we should take the
> opportunity to promote the condition to WARN_ON_ONCE().

Can't you construct a case where an allocation fails during the walk
(memcache empty), and we end up on this exact path?

> 
> > @@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
> >  
> >  	if (!stage2_try_break_pte(ctx, mmu)) {
> >  		kvm_pgtable_stage2_free_unlinked(mm_ops, childp, level);
> > -		mm_ops->put_page(childp);
> >  		return -EAGAIN;
> >  	}
> 
> This, on the other hand, seems possible. There exists a race where an
> old block PTE could have the AF set on it and the underlying cmpxchg()
> could fail. There shouldn't be a race with any software walkers, as we
> hold the MMU lock for write here.

AF update is indeed a likely candidate.

In any case, this patch looks good to me as it is, and we can always
have a separate tweak to adjust the severity of the first case as
required. Unless anyone objects, I'd like to queue it shortly.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

Posted by Oliver Upton 8 months, 2 weeks ago

On Tue, Feb 13, 2024 at 11:12:34AM +0000, Marc Zyngier wrote:
> On Mon, 12 Feb 2024 20:14:37 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote:
> > > kvm_pgtable_stage2_free_unlinked() does the final put_page() on the
> > > root page of the sub-tree before returning, so remove the additional
> > > put_page() invocations in the callers.
> > > 
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Oliver Upton <oliver.upton@linux.dev>
> > > Cc: Ricardo Koller <ricarkol@google.com>
> > > Signed-off-by: Will Deacon <will@kernel.org>
> > > ---
> > > 
> > > Hi folks,
> > > 
> > > Sending this as an RFC as I only spotted it from code inspection and I'm
> > > surprised others aren't seeing fireworks if it's a genuine bug. I also
> > > couldn't come up with a sensible Fixes tag, as all of:
> > > 
> > >  e7c05540c694b ("KVM: arm64: Add helper for creating unlinked stage2 subtrees")
> > >  8f5a3eb7513fc ("KVM: arm64: Add kvm_pgtable_stage2_split()")
> > >  f6a27d6dc51b2 ("KVM: arm64: Drop last page ref in kvm_pgtable_stage2_free_removed()")
> 
> I'd blame it on the last commit, as we really ought to have it if we
> have the others.
> 
> > >
> > > are actually ok in isolation. Hrm. Please tell me I'm wrong?
> > > 
> > >  arch/arm64/kvm/hyp/pgtable.c | 2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > > index c651df904fe3..ab9d05fcf98b 100644
> > > --- a/arch/arm64/kvm/hyp/pgtable.c
> > > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > > @@ -1419,7 +1419,6 @@ kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt,
> > >  				 level + 1);
> > >  	if (ret) {
> > >  		kvm_pgtable_stage2_free_unlinked(mm_ops, pgtable, level);
> > > -		mm_ops->put_page(pgtable);
> > >  		return ERR_PTR(ret);
> > >  	}
> > 
> > AFAICT, this entire branch is effectively dead code, unless there's a
> > KVM bug lurking behind the page table walk. The sub-tree isn't visible
> > to other software or hardware walkers yet, so none of the PTE races
> > could cause this to pop.
> > 
> > So while this is very obviously a bug, it might be pure luck that folks
> > haven't seen smoke here. Perhaps while fixing the bug we should take the
> > opportunity to promote the condition to WARN_ON_ONCE().
> 
> Can't you construct a case where an allocation fails during the walk
> (memcache empty), and we end up on this exact path?

Possibly, but AFAICT that can only happen if there was a bug in KVM. We
don't start the walk at all if userspace set the split chunk size to 0,
and otherwise we expect it to be at least PMD_SIZE, which will top up
the cache to 1 every pass. stage2_split_walker() will 'do the right
thing' if there aren't enough preallocated pages to get down to level 3.

It really doesn't matter either way, I'm just trying to convince myself
of the reasons why we haven't seen this explode yet :)

> > 
> > > @@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
> > >  
> > >  	if (!stage2_try_break_pte(ctx, mmu)) {
> > >  		kvm_pgtable_stage2_free_unlinked(mm_ops, childp, level);
> > > -		mm_ops->put_page(childp);
> > >  		return -EAGAIN;
> > >  	}
> > 
> > This, on the other hand, seems possible. There exists a race where an
> > old block PTE could have the AF set on it and the underlying cmpxchg()
> > could fail. There shouldn't be a race with any software walkers, as we
> > hold the MMU lock for write here.
> 
> AF update is indeed a likely candidate.
> 
> In any case, this patch looks good to me as it is, and we can always
> have a separate tweak to adjust the severity of the first case as
> required. Unless anyone objects, I'd like to queue it shortly.

Agreed, happy with the way this looks and should've added:

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

the first time around.

-- 
Thanks,
Oliver

Re: [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

Posted by Marc Zyngier 8 months, 2 weeks ago

On Tue, 13 Feb 2024 16:29:42 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Tue, Feb 13, 2024 at 11:12:34AM +0000, Marc Zyngier wrote:
> > On Mon, 12 Feb 2024 20:14:37 +0000,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote:
> > > > kvm_pgtable_stage2_free_unlinked() does the final put_page() on the
> > > > root page of the sub-tree before returning, so remove the additional
> > > > put_page() invocations in the callers.
> > > > 
> > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > Cc: Oliver Upton <oliver.upton@linux.dev>
> > > > Cc: Ricardo Koller <ricarkol@google.com>
> > > > Signed-off-by: Will Deacon <will@kernel.org>
> > > > ---
> > > > 
> > > > Hi folks,
> > > > 
> > > > Sending this as an RFC as I only spotted it from code inspection and I'm
> > > > surprised others aren't seeing fireworks if it's a genuine bug. I also
> > > > couldn't come up with a sensible Fixes tag, as all of:
> > > > 
> > > >  e7c05540c694b ("KVM: arm64: Add helper for creating unlinked stage2 subtrees")
> > > >  8f5a3eb7513fc ("KVM: arm64: Add kvm_pgtable_stage2_split()")
> > > >  f6a27d6dc51b2 ("KVM: arm64: Drop last page ref in kvm_pgtable_stage2_free_removed()")
> > 
> > I'd blame it on the last commit, as we really ought to have it if we
> > have the others.
> > 
> > > >
> > > > are actually ok in isolation. Hrm. Please tell me I'm wrong?
> > > > 
> > > >  arch/arm64/kvm/hyp/pgtable.c | 2 --
> > > >  1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > > > index c651df904fe3..ab9d05fcf98b 100644
> > > > --- a/arch/arm64/kvm/hyp/pgtable.c
> > > > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > > > @@ -1419,7 +1419,6 @@ kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt,
> > > >  				 level + 1);
> > > >  	if (ret) {
> > > >  		kvm_pgtable_stage2_free_unlinked(mm_ops, pgtable, level);
> > > > -		mm_ops->put_page(pgtable);
> > > >  		return ERR_PTR(ret);
> > > >  	}
> > > 
> > > AFAICT, this entire branch is effectively dead code, unless there's a
> > > KVM bug lurking behind the page table walk. The sub-tree isn't visible
> > > to other software or hardware walkers yet, so none of the PTE races
> > > could cause this to pop.
> > > 
> > > So while this is very obviously a bug, it might be pure luck that folks
> > > haven't seen smoke here. Perhaps while fixing the bug we should take the
> > > opportunity to promote the condition to WARN_ON_ONCE().
> > 
> > Can't you construct a case where an allocation fails during the walk
> > (memcache empty), and we end up on this exact path?
> 
> Possibly, but AFAICT that can only happen if there was a bug in KVM. We
> don't start the walk at all if userspace set the split chunk size to 0,
> and otherwise we expect it to be at least PMD_SIZE, which will top up
> the cache to 1 every pass. stage2_split_walker() will 'do the right
> thing' if there aren't enough preallocated pages to get down to level 3.
> 
> It really doesn't matter either way, I'm just trying to convince myself
> of the reasons why we haven't seen this explode yet :)

Yeah, that's probably very unlikely to hit given the current
conditions.

> 
> > > 
> > > > @@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
> > > >  
> > > >  	if (!stage2_try_break_pte(ctx, mmu)) {
> > > >  		kvm_pgtable_stage2_free_unlinked(mm_ops, childp, level);
> > > > -		mm_ops->put_page(childp);
> > > >  		return -EAGAIN;
> > > >  	}
> > > 
> > > This, on the other hand, seems possible. There exists a race where an
> > > old block PTE could have the AF set on it and the underlying cmpxchg()
> > > could fail. There shouldn't be a race with any software walkers, as we
> > > hold the MMU lock for write here.
> > 
> > AF update is indeed a likely candidate.
> > 
> > In any case, this patch looks good to me as it is, and we can always
> > have a separate tweak to adjust the severity of the first case as
> > required. Unless anyone objects, I'd like to queue it shortly.
> 
> Agreed, happy with the way this looks and should've added:
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
> 
> the first time around.

Thanks for that. I'll queue that shortly and send (another) PR.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

Posted by Will Deacon 8 months, 2 weeks ago

On Tue, Feb 13, 2024 at 11:12:34AM +0000, Marc Zyngier wrote:
> On Mon, 12 Feb 2024 20:14:37 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote:
> > > Sending this as an RFC as I only spotted it from code inspection and I'm
> > > surprised others aren't seeing fireworks if it's a genuine bug. I also
> > > couldn't come up with a sensible Fixes tag, as all of:
> > > 
> > >  e7c05540c694b ("KVM: arm64: Add helper for creating unlinked stage2 subtrees")
> > >  8f5a3eb7513fc ("KVM: arm64: Add kvm_pgtable_stage2_split()")
> > >  f6a27d6dc51b2 ("KVM: arm64: Drop last page ref in kvm_pgtable_stage2_free_removed()")
> 
> I'd blame it on the last commit, as we really ought to have it if we
> have the others.

Yes, that's probably the best approach if you're adding a Fixes tag.

> > > @@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
> > >  
> > >  	if (!stage2_try_break_pte(ctx, mmu)) {
> > >  		kvm_pgtable_stage2_free_unlinked(mm_ops, childp, level);
> > > -		mm_ops->put_page(childp);
> > >  		return -EAGAIN;
> > >  	}
> > 
> > This, on the other hand, seems possible. There exists a race where an
> > old block PTE could have the AF set on it and the underlying cmpxchg()
> > could fail. There shouldn't be a race with any software walkers, as we
> > hold the MMU lock for write here.
> 
> AF update is indeed a likely candidate.
> 
> In any case, this patch looks good to me as it is, and we can always
> have a separate tweak to adjust the severity of the first case as
> required. Unless anyone objects, I'd like to queue it shortly.

Fine by me! Even though I found it by inspection, I have taken the patch
for a spin to check that I (somehow) didn't break something else.

Will

Re: [RFC PATCH] KVM: arm64: Fix double-free following kvm_pgtable_stage2_free_unlinked()

Posted by Oliver Upton 8 months, 2 weeks ago

On Mon, Feb 12, 2024 at 08:14:41PM +0000, Oliver Upton wrote:
> On Mon, Feb 12, 2024 at 07:30:52PM +0000, Will Deacon wrote:
> > kvm_pgtable_stage2_free_unlinked() does the final put_page() on the
> > root page of the sub-tree before returning, so remove the additional
> > put_page() invocations in the callers.
> > 
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Oliver Upton <oliver.upton@linux.dev>
> > Cc: Ricardo Koller <ricarkol@google.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> > 
> > Hi folks,
> > 
> > Sending this as an RFC as I only spotted it from code inspection and I'm
> > surprised others aren't seeing fireworks if it's a genuine bug. I also
> > couldn't come up with a sensible Fixes tag, as all of:
> > 
> >  e7c05540c694b ("KVM: arm64: Add helper for creating unlinked stage2 subtrees")
> >  8f5a3eb7513fc ("KVM: arm64: Add kvm_pgtable_stage2_split()")
> >  f6a27d6dc51b2 ("KVM: arm64: Drop last page ref in kvm_pgtable_stage2_free_removed()")
> > 
> > are actually ok in isolation. Hrm. Please tell me I'm wrong?
> > 
> >  arch/arm64/kvm/hyp/pgtable.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > index c651df904fe3..ab9d05fcf98b 100644
> > --- a/arch/arm64/kvm/hyp/pgtable.c
> > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > @@ -1419,7 +1419,6 @@ kvm_pte_t *kvm_pgtable_stage2_create_unlinked(struct kvm_pgtable *pgt,
> >  				 level + 1);
> >  	if (ret) {
> >  		kvm_pgtable_stage2_free_unlinked(mm_ops, pgtable, level);
> > -		mm_ops->put_page(pgtable);
> >  		return ERR_PTR(ret);
> >  	}
> 
> AFAICT, this entire branch is effectively dead code, unless there's a
> KVM bug lurking behind the page table walk. The sub-tree isn't visible
> to other software or hardware walkers yet, so none of the PTE races
> could cause this to pop.
> 
> So while this is very obviously a bug, it might be pure luck that folks
> haven't seen smoke here. Perhaps while fixing the bug we should take the
> opportunity to promote the condition to WARN_ON_ONCE().
> 
> > @@ -1502,7 +1501,6 @@ static int stage2_split_walker(const struct kvm_pgtable_visit_ctx *ctx,
> >  
> >  	if (!stage2_try_break_pte(ctx, mmu)) {
> >  		kvm_pgtable_stage2_free_unlinked(mm_ops, childp, level);
> > -		mm_ops->put_page(childp);
> >  		return -EAGAIN;
> >  	}
> 
> This, on the other hand, seems possible. There exists a race where an
> old block PTE could have the AF set on it and the underlying cmpxchg()
> could fail.

That is to say, a race with a *HW* update to the PTE that sets AF.

> There shouldn't be a race with any software walkers, as we
> hold the MMU lock for write here.
> 
> -- 
> Thanks,
> Oliver

-- 
Thanks,
Oliver