[PATCH] KVM: arm64: timers: Fix resource leaks in kvm_timer_hyp_init()

[PATCH] KVM: arm64: timers: Fix resource leaks in kvm_timer_hyp_init()

[Dan Carpenter]

Smatch detected this bug:
    arch/arm64/kvm/arch_timer.c:1425 kvm_timer_hyp_init()
    warn: missing unwind goto?

There are a couple error paths which do not release their resources
correctly.  Fix them.

Fixes: 9e01dc76be6a ("KVM: arm/arm64: arch_timer: Assign the phys timer on VHE systems")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 arch/arm64/kvm/arch_timer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 05b022be885b..c2df8332d2bd 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -1422,7 +1422,7 @@ int __init kvm_timer_hyp_init(bool has_gic)
 		if (err) {
 			kvm_err("kvm_arch_timer: can't request ptimer interrupt %d (%d)\n",
 				host_ptimer_irq, err);
-			return err;
+			goto out_free_irq;
 		}
 
 		if (has_gic) {
@@ -1430,7 +1430,7 @@ int __init kvm_timer_hyp_init(bool has_gic)
 						    kvm_get_running_vcpus());
 			if (err) {
 				kvm_err("kvm_arch_timer: error setting vcpu affinity\n");
-				goto out_free_irq;
+				goto out_free_ptimer_irq;
 			}
 		}
 
@@ -1443,6 +1443,10 @@ int __init kvm_timer_hyp_init(bool has_gic)
 	}
 
 	return 0;
+
+out_free_ptimer_irq:
+	if (info->physical_irq > 0)
+		free_percpu_irq(host_ptimer_irq, kvm_get_running_vcpus());
 out_free_irq:
 	free_percpu_irq(host_vtimer_irq, kvm_get_running_vcpus());
 	return err;
-- 
2.39.2

Re: [PATCH] KVM: arm64: timers: Fix resource leaks in kvm_timer_hyp_init()

[Oliver Upton]

Hi Dan,

Thanks for fixing this. Couple of small comments:

On Mon, Jun 12, 2023 at 10:07:46AM +0300, Dan Carpenter wrote:
> Smatch detected this bug:
>     arch/arm64/kvm/arch_timer.c:1425 kvm_timer_hyp_init()
>     warn: missing unwind goto?
> 
> There are a couple error paths which do not release their resources
> correctly.  Fix them.
> 
> Fixes: 9e01dc76be6a ("KVM: arm/arm64: arch_timer: Assign the phys timer on VHE systems")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  arch/arm64/kvm/arch_timer.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 05b022be885b..c2df8332d2bd 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -1422,7 +1422,7 @@ int __init kvm_timer_hyp_init(bool has_gic)
>  		if (err) {
>  			kvm_err("kvm_arch_timer: can't request ptimer interrupt %d (%d)\n",
>  				host_ptimer_irq, err);
> -			return err;
> +			goto out_free_irq;
>  		}
>  
>  		if (has_gic) {
> @@ -1430,7 +1430,7 @@ int __init kvm_timer_hyp_init(bool has_gic)
>  						    kvm_get_running_vcpus());
>  			if (err) {
>  				kvm_err("kvm_arch_timer: error setting vcpu affinity\n");
> -				goto out_free_irq;
> +				goto out_free_ptimer_irq;
>  			}
>  		}
>  
> @@ -1443,6 +1443,10 @@ int __init kvm_timer_hyp_init(bool has_gic)
>  	}
>  
>  	return 0;
> +
> +out_free_ptimer_irq:
> +	if (info->physical_irq > 0)
> +		free_percpu_irq(host_ptimer_irq, kvm_get_running_vcpus());

nit: we shouldn't even jump to this label in the first place if
there was no ptimer irq to set up... Maybe just drop the condition?

>  out_free_irq:

I'd prefer this label be renamed 'out_free_vtimer_irq' to make it
unambiguous.

>  	free_percpu_irq(host_vtimer_irq, kvm_get_running_vcpus());
>  	return err;
> -- 
> 2.39.2
> 

-- 
Thanks,
Oliver

Re: [PATCH] KVM: arm64: timers: Fix resource leaks in kvm_timer_hyp_init()

[Dan Carpenter]

On Mon, Jun 12, 2023 at 07:32:52AM +0000, Oliver Upton wrote:
> Hi Dan,
> 
> Thanks for fixing this. Couple of small comments:
> 
> On Mon, Jun 12, 2023 at 10:07:46AM +0300, Dan Carpenter wrote:
> > Smatch detected this bug:
> >     arch/arm64/kvm/arch_timer.c:1425 kvm_timer_hyp_init()
> >     warn: missing unwind goto?
> > 
> > There are a couple error paths which do not release their resources
> > correctly.  Fix them.
> > 
> > Fixes: 9e01dc76be6a ("KVM: arm/arm64: arch_timer: Assign the phys timer on VHE systems")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  arch/arm64/kvm/arch_timer.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index 05b022be885b..c2df8332d2bd 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -1422,7 +1422,7 @@ int __init kvm_timer_hyp_init(bool has_gic)
> >  		if (err) {
> >  			kvm_err("kvm_arch_timer: can't request ptimer interrupt %d (%d)\n",
> >  				host_ptimer_irq, err);
> > -			return err;
> > +			goto out_free_irq;
> >  		}
> >  
> >  		if (has_gic) {
> > @@ -1430,7 +1430,7 @@ int __init kvm_timer_hyp_init(bool has_gic)
> >  						    kvm_get_running_vcpus());
> >  			if (err) {
> >  				kvm_err("kvm_arch_timer: error setting vcpu affinity\n");
> > -				goto out_free_irq;
> > +				goto out_free_ptimer_irq;
> >  			}
> >  		}
> >  
> > @@ -1443,6 +1443,10 @@ int __init kvm_timer_hyp_init(bool has_gic)
> >  	}
> >  
> >  	return 0;
> > +
> > +out_free_ptimer_irq:
> > +	if (info->physical_irq > 0)
> > +		free_percpu_irq(host_ptimer_irq, kvm_get_running_vcpus());
> 
> nit: we shouldn't even jump to this label in the first place if
> there was no ptimer irq to set up... Maybe just drop the condition?
> 

The condition is not necessary but I added it deliberately for
readability and in case we ever add more allocations to this function.
I want to keep it.

> >  out_free_irq:
> 
> I'd prefer this label be renamed 'out_free_vtimer_irq' to make it
> unambiguous.

I would prefer this too, but I left it out because I don't like to
rename things unnecessarily.  However, since we both prefer this, then
I will rename it.

regards,
dan carpenter

Re: [PATCH] KVM: arm64: timers: Fix resource leaks in kvm_timer_hyp_init()

[Oliver Upton]

On Mon, Jun 12, 2023 at 11:00:20AM +0300, Dan Carpenter wrote:
> On Mon, Jun 12, 2023 at 07:32:52AM +0000, Oliver Upton wrote:
> > > +
> > > +out_free_ptimer_irq:
> > > +	if (info->physical_irq > 0)
> > > +		free_percpu_irq(host_ptimer_irq, kvm_get_running_vcpus());
> > 
> > nit: we shouldn't even jump to this label in the first place if
> > there was no ptimer irq to set up... Maybe just drop the condition?
> > 
> 
> The condition is not necessary but I added it deliberately for
> readability and in case we ever add more allocations to this function.
> I want to keep it.

Fair enough. And if your fix is any indicator, we're liable to screw up
error handling again in the future :)

> > >  out_free_irq:
> > 
> > I'd prefer this label be renamed 'out_free_vtimer_irq' to make it
> > unambiguous.
> 
> I would prefer this too, but I left it out because I don't like to
> rename things unnecessarily.  However, since we both prefer this, then
> I will rename it.

Thanks! Please do send out a v2 when you have a moment, as I'd like to
pick this up for 6.5.

--
Thanks,
Oliver

Re: [PATCH] KVM: arm64: timers: Fix resource leaks in kvm_timer_hyp_init()

[Oliver Upton]

On Mon, Jun 12, 2023 at 11:00:20AM +0300, Dan Carpenter wrote:
> On Mon, Jun 12, 2023 at 07:32:52AM +0000, Oliver Upton wrote:
> > > +
> > > +out_free_ptimer_irq:
> > > +	if (info->physical_irq > 0)
> > > +		free_percpu_irq(host_ptimer_irq, kvm_get_running_vcpus());
> > 
> > nit: we shouldn't even jump to this label in the first place if
> > there was no ptimer irq to set up... Maybe just drop the condition?
> > 
> 
> The condition is not necessary but I added it deliberately for
> readability and in case we ever add more allocations to this function.
> I want to keep it.

Fair enough. And if your fix is any indicator, we're liable to screw up
error handling again in the future :)

> > >  out_free_irq:
> > 
> > I'd prefer this label be renamed 'out_free_vtimer_irq' to make it
> > unambiguous.
> 
> I would prefer this too, but I left it out because I don't like to
> rename things unnecessarily.  However, since we both prefer this, then
> I will rename it.

Thanks! Please do send out a v2 when you have a moment, as I'd like to
pick this up for 6.5.

--
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel