Patchwork KVM: fix error handling in svm_cpu_init

login
register
mail settings
Submitter Li RongQing
Date March 12, 2019, 7:28 a.m.
Message ID <1552375733-24985-1-git-send-email-lirongqing@baidu.com>
Download mbox | patch
Permalink /patch/746763/
State New
Headers show

Comments

Li RongQing - March 12, 2019, 7:28 a.m.
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(-)
Brijesh Singh - March 13, 2019, 3:28 p.m.
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.
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.
> -----邮件原件-----

> 发件人: 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

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)