| Message ID | 20230522161249.800829-2-aleksandr.mikhalitsyn@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SVM: small tweaks for sev_hardware_setup | expand |
On Mon, May 22, 2023, Alexander Mikhalitsyn wrote: > If misc_cg_set_capacity() fails for some reason then we have > a memleak for sev_reclaim_asid_bitmap/sev_asid_bitmap. It's > not a case right now, because misc_cg_set_capacity() just can't > fail and check inside it is always successful. > > But let's fix that for code consistency. > > Cc: Sean Christopherson <seanjc@google.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: St�phane Graber <stgraber@ubuntu.com> > Cc: kvm@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > --- > arch/x86/kvm/svm/sev.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 69ae5e1b3120..cc832a8d1bca 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2216,8 +2216,13 @@ void __init sev_hardware_setup(void) > } > > sev_asid_count = max_sev_asid - min_sev_asid + 1; > - if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) > + if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) { > + bitmap_free(sev_reclaim_asid_bitmap); > + sev_reclaim_asid_bitmap = NULL; > + bitmap_free(sev_asid_bitmap); > + sev_asid_bitmap = NULL; > goto out; > + } Blech, didn't look close enough at v1. I think I'd rather yell and continue on. If misc_cg_set_capacity() were to fail, debugging would be unnecessarily painful, and at least as things stand today, there's nothing userspace can do to remedy the problem except by manually disabling SEV and/or SEV-ES. --- From: Sean Christopherson <seanjc@google.com> Date: Tue, 6 Jun 2023 11:34:28 -0700 Subject: [PATCH] KVM: SVM: WARN, but continue, if misc_cg_set_capacity() fails WARN and continue if misc_cg_set_capacity() fails, as the only scenario in which it can fail is if the specified resource is invalid, which should never happen when CONFIG_KVM_AMD_SEV=y. Deliberately not bailing "fixes" a theoretical bug where KVM would leak the ASID bitmaps on failure, which again can't happen. If the impossible should happen, the end result is effectively the same with respect to SEV and SEV-ES (they are unusable), while continuing on has the advantage of letting KVM load, i.e. userspace can still run non-SEV guests. Reported-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/svm/sev.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index d65578d8784d..07756b7348ae 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2216,9 +2216,7 @@ void __init sev_hardware_setup(void) } sev_asid_count = max_sev_asid - min_sev_asid + 1; - if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) - goto out; - + WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)); sev_supported = true; /* SEV-ES support requested? */ @@ -2243,9 +2241,7 @@ void __init sev_hardware_setup(void) goto out; sev_es_asid_count = min_sev_asid - 1; - if (misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count)) - goto out; - + WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count)); sev_es_supported = true; out: base-commit: 6d1bc9754b04075d938b47cf7f7800814b8911a7 --
On Tue, Jun 6, 2023 at 8:45 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, May 22, 2023, Alexander Mikhalitsyn wrote: > > If misc_cg_set_capacity() fails for some reason then we have > > a memleak for sev_reclaim_asid_bitmap/sev_asid_bitmap. It's > > not a case right now, because misc_cg_set_capacity() just can't > > fail and check inside it is always successful. > > > > But let's fix that for code consistency. > > > > Cc: Sean Christopherson <seanjc@google.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: St�phane Graber <stgraber@ubuntu.com> > > Cc: kvm@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > > --- > > arch/x86/kvm/svm/sev.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > > index 69ae5e1b3120..cc832a8d1bca 100644 > > --- a/arch/x86/kvm/svm/sev.c > > +++ b/arch/x86/kvm/svm/sev.c > > @@ -2216,8 +2216,13 @@ void __init sev_hardware_setup(void) > > } > > > > sev_asid_count = max_sev_asid - min_sev_asid + 1; > > - if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) > > + if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) { > > + bitmap_free(sev_reclaim_asid_bitmap); > > + sev_reclaim_asid_bitmap = NULL; > > + bitmap_free(sev_asid_bitmap); > > + sev_asid_bitmap = NULL; > > goto out; > > + } > > Blech, didn't look close enough at v1. I think I'd rather yell and continue on. > If misc_cg_set_capacity() were to fail, debugging would be unnecessarily painful, > and at least as things stand today, there's nothing userspace can do to remedy > the problem except by manually disabling SEV and/or SEV-ES. Hi Sean, I agree with your point. Let's go this way! Kind regards, Alex > > --- > From: Sean Christopherson <seanjc@google.com> > Date: Tue, 6 Jun 2023 11:34:28 -0700 > Subject: [PATCH] KVM: SVM: WARN, but continue, if misc_cg_set_capacity() fails > > WARN and continue if misc_cg_set_capacity() fails, as the only scenario > in which it can fail is if the specified resource is invalid, which should > never happen when CONFIG_KVM_AMD_SEV=y. Deliberately not bailing "fixes" > a theoretical bug where KVM would leak the ASID bitmaps on failure, which > again can't happen. > > If the impossible should happen, the end result is effectively the same > with respect to SEV and SEV-ES (they are unusable), while continuing on > has the advantage of letting KVM load, i.e. userspace can still run > non-SEV guests. > > Reported-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/svm/sev.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index d65578d8784d..07756b7348ae 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2216,9 +2216,7 @@ void __init sev_hardware_setup(void) > } > > sev_asid_count = max_sev_asid - min_sev_asid + 1; > - if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) > - goto out; > - > + WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)); > sev_supported = true; > > /* SEV-ES support requested? */ > @@ -2243,9 +2241,7 @@ void __init sev_hardware_setup(void) > goto out; > > sev_es_asid_count = min_sev_asid - 1; > - if (misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count)) > - goto out; > - > + WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count)); > sev_es_supported = true; > > out: > > base-commit: 6d1bc9754b04075d938b47cf7f7800814b8911a7 > -- >
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 69ae5e1b3120..cc832a8d1bca 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2216,8 +2216,13 @@ void __init sev_hardware_setup(void) } sev_asid_count = max_sev_asid - min_sev_asid + 1; - if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) + if (misc_cg_set_capacity(MISC_CG_RES_SEV, sev_asid_count)) { + bitmap_free(sev_reclaim_asid_bitmap); + sev_reclaim_asid_bitmap = NULL; + bitmap_free(sev_asid_bitmap); + sev_asid_bitmap = NULL; goto out; + } pr_info("SEV supported: %u ASIDs\n", sev_asid_count); sev_supported = true;
If misc_cg_set_capacity() fails for some reason then we have a memleak for sev_reclaim_asid_bitmap/sev_asid_bitmap. It's not a case right now, because misc_cg_set_capacity() just can't fail and check inside it is always successful. But let's fix that for code consistency. Cc: Sean Christopherson <seanjc@google.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Stéphane Graber <stgraber@ubuntu.com> Cc: kvm@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com> --- arch/x86/kvm/svm/sev.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)