diff mbox series

KVM: fix error handling in svm_cpu_init

Message ID 1552375733-24985-1-git-send-email-lirongqing@baidu.com (mailing list archive)
State New, archived
Headers show
Series KVM: fix error handling in svm_cpu_init | expand

Commit Message

Li RongQing March 12, 2019, 7:28 a.m. UTC
sd->save_area should be freed in error path

Fixes: 70cd94e60c733 ("KVM: SVM: VMRUN should use associated ASID when SEV is enabled")
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 arch/x86/kvm/svm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Brijesh Singh March 13, 2019, 3:28 p.m. UTC | #1
On 3/12/19 2:28 AM, Li RongQing wrote:
> sd->save_area should be freed in error path
> 
> Fixes: 70cd94e60c733 ("KVM: SVM: VMRUN should use associated ASID when SEV is enabled")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>   arch/x86/kvm/svm.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 


Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

thanks
Vitaly Kuznetsov March 14, 2019, 1:41 p.m. UTC | #2
Li RongQing <lirongqing@baidu.com> writes:

> sd->save_area should be freed in error path
>
> Fixes: 70cd94e60c733 ("KVM: SVM: VMRUN should use associated ASID when SEV is enabled")
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  arch/x86/kvm/svm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index f13a3a24d360..1787a484d21c 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1006,17 +1006,18 @@ static int svm_cpu_init(int cpu)
>  					      sizeof(void *),
>  					      GFP_KERNEL);
>  		if (!sd->sev_vmcbs)
> -			goto err_1;
> +			goto err_2;

Generally, 'err_1', 'err_2' are not good names for labels: when function
becomes longer and you need to scroll it becomes hard to see if the
cleanup path is correct ("Which one do I need here? err_5 or err_6? What
are they doing"?)

I'd suggest to use something like "err_free_saved_area" and
"err_free_sd" here instead.

>  	}
>  
>  	per_cpu(svm_data, cpu) = sd;
>  
>  	return 0;
>  
> +err_2:
> +	__free_page(sd->save_area);
>  err_1:
>  	kfree(sd);
>  	return r;
> -
>  }
>  
>  static bool valid_msr_intercept(u32 index)
Li RongQing March 15, 2019, 1:08 a.m. UTC | #3
> -----邮件原件-----
> 发件人: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> 发送时间: 2019年3月14日 21:41
> 收件人: Li,Rongqing <lirongqing@baidu.com>; x86@kernel.org;
> kvm@vger.kernel.org
> 主题: Re: [PATCH] KVM: fix error handling in svm_cpu_init
> 
> Li RongQing <lirongqing@baidu.com> writes:
> 
> > sd->save_area should be freed in error path
> >
> > Fixes: 70cd94e60c733 ("KVM: SVM: VMRUN should use associated ASID
> when
> > SEV is enabled")
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> >  arch/x86/kvm/svm.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index
> > f13a3a24d360..1787a484d21c 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -1006,17 +1006,18 @@ static int svm_cpu_init(int cpu)
> >  					      sizeof(void *),
> >  					      GFP_KERNEL);
> >  		if (!sd->sev_vmcbs)
> > -			goto err_1;
> > +			goto err_2;
> 
> Generally, 'err_1', 'err_2' are not good names for labels: when function
> becomes longer and you need to scroll it becomes hard to see if the cleanup
> path is correct ("Which one do I need here? err_5 or err_6? What are they
> doing"?)
> 
> I'd suggest to use something like "err_free_saved_area" and "err_free_sd"
> here instead.
> 


OK, I will send v2

Thanks


-RongQing 

> >  	}
> >
> >  	per_cpu(svm_data, cpu) = sd;
> >
> >  	return 0;
> >
> > +err_2:
> > +	__free_page(sd->save_area);
> >  err_1:
> >  	kfree(sd);
> >  	return r;
> > -
> >  }
> >
> >  static bool valid_msr_intercept(u32 index)
> 
> --
> Vitaly
diff mbox series

Patch

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index f13a3a24d360..1787a484d21c 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1006,17 +1006,18 @@  static int svm_cpu_init(int cpu)
 					      sizeof(void *),
 					      GFP_KERNEL);
 		if (!sd->sev_vmcbs)
-			goto err_1;
+			goto err_2;
 	}
 
 	per_cpu(svm_data, cpu) = sd;
 
 	return 0;
 
+err_2:
+	__free_page(sd->save_area);
 err_1:
 	kfree(sd);
 	return r;
-
 }
 
 static bool valid_msr_intercept(u32 index)