| Message ID | 20240809190319.1710470-23-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Fix multiple #PF RO infinite loop bugs | expand |
On 8/9/24 21:03, Sean Christopherson wrote: > Explicitly query the list of to-be-zapped shadow pages when checking to > see if unprotecting a gfn for retry has succeeded, i.e. if KVM should > retry the faulting instruction. > > Add a comment to explain why the list needs to be checked before zapping, > which is the primary motivation for this change. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/mmu/mmu.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index 300a47801685..50695eb2ee22 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -2731,12 +2731,15 @@ bool __kvm_mmu_unprotect_gfn_and_retry(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, > goto out; > } > > - r = false; > write_lock(&kvm->mmu_lock); > - for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa)) { > - r = true; > + for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa)) > kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list); > - } > + > + /* > + * Snapshot the result before zapping, as zapping will remove all list > + * entries, i.e. checking the list later would yield a false negative. > + */ Hmm, the comment is kinda overkill? Maybe just /* Return whether there were sptes to zap. */ r = !list_empty(&invalid_test); I'm not sure about patch 21 - I like the simple kvm_mmu_unprotect_page() function. Maybe rename it to kvm_mmu_zap_gfn() and make it static in the same patch? Either way, this small cleanup applies even if the function is not inlined. Thanks, Paolo > + r = !list_empty(&invalid_list); > kvm_mmu_commit_zap_page(kvm, &invalid_list); > write_unlock(&kvm->mmu_lock); >
On Wed, Aug 14, 2024, Paolo Bonzini wrote: > On 8/9/24 21:03, Sean Christopherson wrote: > > Explicitly query the list of to-be-zapped shadow pages when checking to > > see if unprotecting a gfn for retry has succeeded, i.e. if KVM should > > retry the faulting instruction. > > > > Add a comment to explain why the list needs to be checked before zapping, > > which is the primary motivation for this change. > > > > No functional change intended. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/mmu/mmu.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > > index 300a47801685..50695eb2ee22 100644 > > --- a/arch/x86/kvm/mmu/mmu.c > > +++ b/arch/x86/kvm/mmu/mmu.c > > @@ -2731,12 +2731,15 @@ bool __kvm_mmu_unprotect_gfn_and_retry(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, > > goto out; > > } > > - r = false; > > write_lock(&kvm->mmu_lock); > > - for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa)) { > > - r = true; > > + for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa)) > > kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list); > > - } > > + > > + /* > > + * Snapshot the result before zapping, as zapping will remove all list > > + * entries, i.e. checking the list later would yield a false negative. > > + */ > > Hmm, the comment is kinda overkill? Maybe just > > /* Return whether there were sptes to zap. */ > r = !list_empty(&invalid_test); I would strongly prefer to keep the verbose comment. I was "this" close to removing the local variable and checking list_empty() after the commit phase. If we made that goof, it would only show up at the worst time, i.e. when a guest triggers retry and gets stuck. And the logical outcome of fixing such a bug would be to add a comment to prevent it from happening again, so I say just add the comment straightaway. > I'm not sure about patch 21 - I like the simple kvm_mmu_unprotect_page() > function. From a code perspective, I kinda like having a separate helper too. As you likely suspect given your below suggestion, KVM should never unprotect a gfn without retry protection, i.e. there should never be another caller, and I want to enforce that. > Maybe rename it to kvm_mmu_zap_gfn() and make it static in the same patch? kvm_mmu_zap_gfn() would be quite misleading. Unlike kvm_zap_gfn_range(), it only zaps non-leaf shadow pages. E.g. the name would suggest that it could be used by __kvm_set_or_clear_apicv_inhibit(), but it would do the complete wrong thing. kvm_mmu_zap_shadow_pages() is the least awful I can come up with (it needs to be plural because it zaps all SPs related to the gfn), but that's something confusing too since it would take in a single gfn. So I think my vote is to keep patch 21 and dodge the naming entirely.
On Thu, Aug 15, 2024, Sean Christopherson wrote: > On Wed, Aug 14, 2024, Paolo Bonzini wrote: > > On 8/9/24 21:03, Sean Christopherson wrote: > > > Explicitly query the list of to-be-zapped shadow pages when checking to > > > see if unprotecting a gfn for retry has succeeded, i.e. if KVM should > > > retry the faulting instruction. > > > > > > Add a comment to explain why the list needs to be checked before zapping, > > > which is the primary motivation for this change. > > > > > > No functional change intended. > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > arch/x86/kvm/mmu/mmu.c | 11 +++++++---- > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > > > index 300a47801685..50695eb2ee22 100644 > > > --- a/arch/x86/kvm/mmu/mmu.c > > > +++ b/arch/x86/kvm/mmu/mmu.c > > > @@ -2731,12 +2731,15 @@ bool __kvm_mmu_unprotect_gfn_and_retry(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, > > > goto out; > > > } > > > - r = false; > > > write_lock(&kvm->mmu_lock); > > > - for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa)) { > > > - r = true; > > > + for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa)) > > > kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list); > > > - } > > > + > > > + /* > > > + * Snapshot the result before zapping, as zapping will remove all list > > > + * entries, i.e. checking the list later would yield a false negative. > > > + */ > > > > Hmm, the comment is kinda overkill? Maybe just > > > > /* Return whether there were sptes to zap. */ > > r = !list_empty(&invalid_test); > > I would strongly prefer to keep the verbose comment. I was "this" close to > removing the local variable and checking list_empty() after the commit phase. > If we made that goof, it would only show up at the worst time, i.e. when a guest > triggers retry and gets stuck. And the logical outcome of fixing such a bug > would be to add a comment to prevent it from happening again, so I say just add > the comment straightaway. > > > I'm not sure about patch 21 - I like the simple kvm_mmu_unprotect_page() > > function. > > >From a code perspective, I kinda like having a separate helper too. As you > likely suspect given your below suggestion, KVM should never unprotect a gfn > without retry protection, i.e. there should never be another caller, and I want > to enforce that. Oh, another argument for eliminating the separate helper is that having a separate helper makes it really hard to write a comment for why reading indirect_shadow_pages outside of mmu_lock is ok (it reads/looks weird if mmu_lock is taken in a different helper).
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index 300a47801685..50695eb2ee22 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -2731,12 +2731,15 @@ bool __kvm_mmu_unprotect_gfn_and_retry(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, goto out; } - r = false; write_lock(&kvm->mmu_lock); - for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa)) { - r = true; + for_each_gfn_valid_sp_with_gptes(kvm, sp, gpa_to_gfn(gpa)) kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list); - } + + /* + * Snapshot the result before zapping, as zapping will remove all list + * entries, i.e. checking the list later would yield a false negative. + */ + r = !list_empty(&invalid_list); kvm_mmu_commit_zap_page(kvm, &invalid_list); write_unlock(&kvm->mmu_lock);
Explicitly query the list of to-be-zapped shadow pages when checking to see if unprotecting a gfn for retry has succeeded, i.e. if KVM should retry the faulting instruction. Add a comment to explain why the list needs to be checked before zapping, which is the primary motivation for this change. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/mmu/mmu.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)