linux-kernel - Re: [PATCH] irqchip: irq-gic-v4: return real error code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ca0dabab-e141-8591-d0f8-d7343147574f@arm.com>
Date:   Mon, 19 Mar 2018 08:30:49 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     Peng Hao <peng.hao2@....com.cn>, tglx@...utronix.de,
        jason@...edaemon.net
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] irqchip: irq-gic-v4: return real error code

On 19/03/18 11:38, Peng Hao wrote:
> __irq_domain_alloc_irqs will return some different error code, so we
> should return real error code in its_alloc_vcpu_irqs.

What do we gain by doing so? Do we end-up treating the error in a
different way? What does this actually improve? This is the kind of
information I'm looking for in a commit message. Not something that
describes the patch.

> 
> Signed-off-by: Peng Hao <peng.hao2@....com.cn>
> ---
>  drivers/irqchip/irq-gic-v4.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
> index dba9d67..ecd170d 100644
> --- a/drivers/irqchip/irq-gic-v4.c
> +++ b/drivers/irqchip/irq-gic-v4.c
> @@ -99,7 +99,7 @@
>  
>  int its_alloc_vcpu_irqs(struct its_vm *vm)
>  {
> -	int vpe_base_irq, i;
> +	int vpe_base_irq, i, ret = -ENOMEM;
>  
>  	vm->fwnode = irq_domain_alloc_named_id_fwnode("GICv4-vpe",
>  						      task_pid_nr(current));
> @@ -120,8 +120,10 @@ int its_alloc_vcpu_irqs(struct its_vm *vm)
>  	vpe_base_irq = __irq_domain_alloc_irqs(vm->domain, -1, vm->nr_vpes,
>  					       NUMA_NO_NODE, vm,
>  					       false, NULL);
> -	if (vpe_base_irq <= 0)
> +	if (vpe_base_irq <= 0) {
> +		ret = vpe_base_irq;
>  		goto err;

Given that a return value of zero denotes an actual error, we can now
pretend that everything went smoothly, except that none of the doorbells
are initialized. KVM will then try and request the interrupts, which
will fail (because 0 is not a valid interrupt number).

We thus went from a situation where the error was completely unambiguous
(failed to allocate a bunch of interrupts) to a situation where things
fail in a bizarre way because we cannot request the interrupts that we
just allocated (something that shouldn't really fail).

I cannot call this change a real improvement.

> +	}
>  
>  	for (i = 0; i < vm->nr_vpes; i++)
>  		vm->vpes[i]->irq = vpe_base_irq + i;
> @@ -134,7 +136,7 @@ int its_alloc_vcpu_irqs(struct its_vm *vm)
>  	if (vm->fwnode)
>  		irq_domain_free_fwnode(vm->fwnode);
>  
> -	return -ENOMEM;
> +	return ret;
>  }
>  
>  void its_free_vcpu_irqs(struct its_vm *vm)
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...