| Message ID | 20241105010558.1266699-2-dionnaglaze@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support SEV firmware hotloading | expand |
On 11/4/24 19:05, Dionna Glaze wrote: > Ensure that snp gctx page allocation is adequately deallocated on > failure during snp_launch_start. > > Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command") > > CC: Sean Christopherson <seanjc@google.com> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: Ingo Molnar <mingo@redhat.com> > CC: Borislav Petkov <bp@alien8.de> > CC: Dave Hansen <dave.hansen@linux.intel.com> > CC: Ashish Kalra <ashish.kalra@amd.com> > CC: Tom Lendacky <thomas.lendacky@amd.com> > CC: John Allen <john.allen@amd.com> > CC: Herbert Xu <herbert@gondor.apana.org.au> > CC: "David S. Miller" <davem@davemloft.net> > CC: Michael Roth <michael.roth@amd.com> > CC: Luis Chamberlain <mcgrof@kernel.org> > CC: Russ Weight <russ.weight@linux.dev> > CC: Danilo Krummrich <dakr@redhat.com> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > CC: "Rafael J. Wysocki" <rafael@kernel.org> > CC: Tianfei zhang <tianfei.zhang@intel.com> > CC: Alexey Kardashevskiy <aik@amd.com> > > Signed-off-by: Dionna Glaze <dionnaglaze@google.com> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/kvm/svm/sev.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 714c517dd4b72..f6e96ec0a5caa 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > if (sev->snp_context) > return -EINVAL; > > - sev->snp_context = snp_context_create(kvm, argp); > - if (!sev->snp_context) > - return -ENOTTY; > - > if (params.flags) > return -EINVAL; > > @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET) > return -EINVAL; > > + sev->snp_context = snp_context_create(kvm, argp); > + if (!sev->snp_context) > + return -ENOTTY; > + > start.gctx_paddr = __psp_pa(sev->snp_context); > start.policy = params.policy; > memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
KVM: SVM: In the future, please post bug fixes separately from new features series, especially when the fix has very little to do with the rest of the series (AFAICT, this has no relation whatsoever beyond SNP). On Tue, Nov 05, 2024, Dionna Glaze wrote: > Ensure that snp gctx page allocation is adequately deallocated on > failure during snp_launch_start. > > Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command") This needs Cc: stable@vger.kernel.org especially if it doesn't get into 6.12. > CC: Sean Christopherson <seanjc@google.com> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: Ingo Molnar <mingo@redhat.com> > CC: Borislav Petkov <bp@alien8.de> > CC: Dave Hansen <dave.hansen@linux.intel.com> > CC: Ashish Kalra <ashish.kalra@amd.com> > CC: Tom Lendacky <thomas.lendacky@amd.com> > CC: John Allen <john.allen@amd.com> > CC: Herbert Xu <herbert@gondor.apana.org.au> > CC: "David S. Miller" <davem@davemloft.net> > CC: Michael Roth <michael.roth@amd.com> > CC: Luis Chamberlain <mcgrof@kernel.org> > CC: Russ Weight <russ.weight@linux.dev> > CC: Danilo Krummrich <dakr@redhat.com> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > CC: "Rafael J. Wysocki" <rafael@kernel.org> > CC: Tianfei zhang <tianfei.zhang@intel.com> > CC: Alexey Kardashevskiy <aik@amd.com> > > Signed-off-by: Dionna Glaze <dionnaglaze@google.com> Acked-by: Sean Christopherson <seanjc@google.com> Paolo, do you want to grab this one for 6.12 too? > --- > arch/x86/kvm/svm/sev.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 714c517dd4b72..f6e96ec0a5caa 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > if (sev->snp_context) > return -EINVAL; > > - sev->snp_context = snp_context_create(kvm, argp); > - if (!sev->snp_context) > - return -ENOTTY; > - > if (params.flags) > return -EINVAL; > > @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET) > return -EINVAL; > > + sev->snp_context = snp_context_create(kvm, argp); > + if (!sev->snp_context) > + return -ENOTTY; Related to this fix, the return values from snp_context_create() are garbage. It should return ERR_PTR(), not NULL. -ENOTTY on an OOM scenatio is blatantly wrong, as -ENOTTY on any SEV_CMD_SNP_GCTX_CREATE failure is too. > + > start.gctx_paddr = __psp_pa(sev->snp_context); > start.policy = params.policy; > memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw)); > -- > 2.47.0.199.ga7371fff76-goog >
On Wed, Nov 6, 2024 at 6:34 AM Sean Christopherson <seanjc@google.com> wrote: > > KVM: SVM: > > In the future, please post bug fixes separately from new features series, especially > when the fix has very little to do with the rest of the series (AFAICT, this has > no relation whatsoever beyond SNP). > Understood. Are dependent series best shared through links to a dev branch containing all patches? > On Tue, Nov 05, 2024, Dionna Glaze wrote: > > Ensure that snp gctx page allocation is adequately deallocated on > > failure during snp_launch_start. > > > > Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command") > > This needs > > Cc: stable@vger.kernel.org > > especially if it doesn't get into 6.12. > > > CC: Sean Christopherson <seanjc@google.com> > > CC: Paolo Bonzini <pbonzini@redhat.com> > > CC: Thomas Gleixner <tglx@linutronix.de> > > CC: Ingo Molnar <mingo@redhat.com> > > CC: Borislav Petkov <bp@alien8.de> > > CC: Dave Hansen <dave.hansen@linux.intel.com> > > CC: Ashish Kalra <ashish.kalra@amd.com> > > CC: Tom Lendacky <thomas.lendacky@amd.com> > > CC: John Allen <john.allen@amd.com> > > CC: Herbert Xu <herbert@gondor.apana.org.au> > > CC: "David S. Miller" <davem@davemloft.net> > > CC: Michael Roth <michael.roth@amd.com> > > CC: Luis Chamberlain <mcgrof@kernel.org> > > CC: Russ Weight <russ.weight@linux.dev> > > CC: Danilo Krummrich <dakr@redhat.com> > > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > CC: "Rafael J. Wysocki" <rafael@kernel.org> > > CC: Tianfei zhang <tianfei.zhang@intel.com> > > CC: Alexey Kardashevskiy <aik@amd.com> > > > > Signed-off-by: Dionna Glaze <dionnaglaze@google.com> > > Acked-by: Sean Christopherson <seanjc@google.com> > > Paolo, do you want to grab this one for 6.12 too? > > > --- > > arch/x86/kvm/svm/sev.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > > index 714c517dd4b72..f6e96ec0a5caa 100644 > > --- a/arch/x86/kvm/svm/sev.c > > +++ b/arch/x86/kvm/svm/sev.c > > @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > > if (sev->snp_context) > > return -EINVAL; > > > > - sev->snp_context = snp_context_create(kvm, argp); > > - if (!sev->snp_context) > > - return -ENOTTY; > > - > > if (params.flags) > > return -EINVAL; > > > > @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > > if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET) > > return -EINVAL; > > > > + sev->snp_context = snp_context_create(kvm, argp); > > + if (!sev->snp_context) > > + return -ENOTTY; > > Related to this fix, the return values from snp_context_create() are garbage. It > should return ERR_PTR(), not NULL. -ENOTTY on an OOM scenatio is blatantly wrong, > as -ENOTTY on any SEV_CMD_SNP_GCTX_CREATE failure is too. > I caught this too. I'll be changing that behavior with the new gctx management API from ccp in v5, i.e., /** * sev_snp_create_context - allocates an SNP context firmware page * * Associates the created context with the ASID that an activation * call after SNP_LAUNCH_START will commit. The association is needed * to track active GCTX pages to refresh during firmware hotload. * * @asid: The ASID allocated to the caller that will be used in a subsequent SNP_ACTIVATE. * @psp_ret: sev command return code. * * Returns: * A pointer to the SNP context page, or an ERR_PTR of * -%ENODEV if the PSP device is not available * -%ENOTSUPP if PSP device does not support SEV * -%ETIMEDOUT if the SEV command timed out * -%EIO if PSP device returned a non-zero return code */ void *sev_snp_create_context(int asid, int *psp_ret); /** * sev_snp_activate_asid - issues SNP_ACTIVATE for the asid and associated GCTX page. * * @asid: The ASID to activate. * @psp_ret: sev command return code. * * Returns: * 0 if the SEV device successfully processed the command * -%ENODEV if the PSP device is not available * -%ENOTSUPP if PSP device does not support SEV * -%ETIMEDOUT if the SEV command timed out * -%EIO if PSP device returned a non-zero return code */ int sev_snp_activate_asid(int asid, int *psp_ret); /** * sev_snp_guest_decommission - issues SNP_DECOMMISSION for an asid's GCTX page and frees it. * * @asid: The ASID to activate. * @psp_ret: sev command return code. * * Returns: * 0 if the SEV device successfully processed the command * -%ENODEV if the PSP device is not available * -%ENOTSUPP if PSP device does not support SEV * -%ETIMEDOUT if the SEV command timed out * -%EIO if PSP device returned a non-zero return code */ int sev_snp_guest_decommission(int asid, int *psp_ret); > > + > > start.gctx_paddr = __psp_pa(sev->snp_context); > > start.policy = params.policy; > > memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw)); > > -- > > 2.47.0.199.ga7371fff76-goog > >
On Wed, Nov 06, 2024, Dionna Amalie Glaze wrote: > On Wed, Nov 6, 2024 at 6:34 AM Sean Christopherson <seanjc@google.com> wrote: > > > > KVM: SVM: > > > > In the future, please post bug fixes separately from new features series, especially > > when the fix has very little to do with the rest of the series (AFAICT, this has > > no relation whatsoever beyond SNP). > > > > Understood. Are dependent series best shared through links to a dev > branch containing all patches? I don't follow. There is no dependency here. If this series were moving snp_context_create() out of KVM, then that would be a different story, i.e. then it _would_ be appropriate to include the fix at the front of the series. If you end up a situation where a dependency is created after the initial posting, e.g. you post this fix, then later decide to move snp_context_create() out of KVM, then simply call that out in the cover letter and provide a lore.kernel.org link. For large scale dependencies, e.g. multi-patch series that build on other multi-patch series, then providing a link to a git branch is helpful. But for something this trivial, it's overkill. > > > --- > > > arch/x86/kvm/svm/sev.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > > > index 714c517dd4b72..f6e96ec0a5caa 100644 > > > --- a/arch/x86/kvm/svm/sev.c > > > +++ b/arch/x86/kvm/svm/sev.c > > > @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > > > if (sev->snp_context) > > > return -EINVAL; > > > > > > - sev->snp_context = snp_context_create(kvm, argp); > > > - if (!sev->snp_context) > > > - return -ENOTTY; > > > - > > > if (params.flags) > > > return -EINVAL; > > > > > > @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > > > if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET) > > > return -EINVAL; > > > > > > + sev->snp_context = snp_context_create(kvm, argp); > > > + if (!sev->snp_context) > > > + return -ENOTTY; > > > > Related to this fix, the return values from snp_context_create() are garbage. It > > should return ERR_PTR(), not NULL. -ENOTTY on an OOM scenatio is blatantly wrong, > > as -ENOTTY on any SEV_CMD_SNP_GCTX_CREATE failure is too. > > I caught this too. I'll be changing that behavior with the new gctx > management API from ccp in v5, i.e., Please fix the KVM flaws before moving code out of KVM, i.e. ensure the flaws are cleaned up even if we opt not to go the route of moving the code out of KVM (which I assume is what you plan to do with sev_snp_create_context()).
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 714c517dd4b72..f6e96ec0a5caa 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2212,10 +2212,6 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) if (sev->snp_context) return -EINVAL; - sev->snp_context = snp_context_create(kvm, argp); - if (!sev->snp_context) - return -ENOTTY; - if (params.flags) return -EINVAL; @@ -2230,6 +2226,10 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp) if (params.policy & SNP_POLICY_MASK_SINGLE_SOCKET) return -EINVAL; + sev->snp_context = snp_context_create(kvm, argp); + if (!sev->snp_context) + return -ENOTTY; + start.gctx_paddr = __psp_pa(sev->snp_context); start.policy = params.policy; memcpy(start.gosvw, params.gosvw, sizeof(params.gosvw));
Ensure that snp gctx page allocation is adequately deallocated on failure during snp_launch_start. Fixes: 136d8bc931c8 ("KVM: SEV: Add KVM_SEV_SNP_LAUNCH_START command") CC: Sean Christopherson <seanjc@google.com> CC: Paolo Bonzini <pbonzini@redhat.com> CC: Thomas Gleixner <tglx@linutronix.de> CC: Ingo Molnar <mingo@redhat.com> CC: Borislav Petkov <bp@alien8.de> CC: Dave Hansen <dave.hansen@linux.intel.com> CC: Ashish Kalra <ashish.kalra@amd.com> CC: Tom Lendacky <thomas.lendacky@amd.com> CC: John Allen <john.allen@amd.com> CC: Herbert Xu <herbert@gondor.apana.org.au> CC: "David S. Miller" <davem@davemloft.net> CC: Michael Roth <michael.roth@amd.com> CC: Luis Chamberlain <mcgrof@kernel.org> CC: Russ Weight <russ.weight@linux.dev> CC: Danilo Krummrich <dakr@redhat.com> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> CC: "Rafael J. Wysocki" <rafael@kernel.org> CC: Tianfei zhang <tianfei.zhang@intel.com> CC: Alexey Kardashevskiy <aik@amd.com> Signed-off-by: Dionna Glaze <dionnaglaze@google.com> --- arch/x86/kvm/svm/sev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)