linux-kernel - Re: [PATCH] virt: kvm: arm: vgic: Process the failure case when kvm_register_device

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 for Android: free password hash cracker in your pocket

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

Date:	Fri, 14 Nov 2014 22:27:03 +0800
From:	Chen Gang <gang.chen.5i5j@...il.com>
To:	Marc Zyngier <marc.zyngier@....com>
CC:	"christoffer.dall@...aro.org" <christoffer.dall@...aro.org>,
	"gleb@...nel.org" <gleb@...nel.org>,
	Paolo Bonzini <pbonzini@...hat.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] virt: kvm: arm: vgic: Process the failure case when kvm_register_device_ops()
 fails

On 11/14/2014 10:09 PM, Marc Zyngier wrote:
> On 14/11/14 14:05, Chen Gang wrote:
>> On 11/13/2014 11:30 PM, Marc Zyngier wrote:
>>> On 13/11/14 15:04, Chen Gang wrote:
>>>> When kvm_register_device_ops() fails, also need call free_percpu_irq()
>>>> just like others have down within kvm_vgic_hyp_init().
>>>>
>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@...il.com>
>>>> ---
>>>>  virt/kvm/arm/vgic.c | 10 ++++++++--
>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>> index 3aaca49..b799f17 100644
>>>> --- a/virt/kvm/arm/vgic.c
>>>> +++ b/virt/kvm/arm/vgic.c
>>>> @@ -2470,8 +2470,14 @@ int kvm_vgic_hyp_init(void)
>>>>  
>>>>  	on_each_cpu(vgic_init_maintenance_interrupt, NULL, 1);
>>>>  
>>>> -	return kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
>>>> -				       KVM_DEV_TYPE_ARM_VGIC_V2);
>>>> +	ret = kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
>>>> +				      KVM_DEV_TYPE_ARM_VGIC_V2);
>>>> +	if (ret) {
>>>> +		kvm_err("Cannot register device ops\n");
>>>> +		goto out_free_irq;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>>  
>>>>  out_free_irq:
>>>>  	free_percpu_irq(vgic->maint_irq, kvm_get_running_vcpus());
>>>>
>>>
>>> Awesome. You're now freeing a per-cpu interrupt after just after having
>>> enabled it on all CPUs. What could possibly go wrong?
>>>
>>
>> OK, thanks. What you said sound reasonable to me. Need call on_each_cpu
>> for disable_percpu_irq(). Also need call __unregister_cpu_notifier(),
>> and need a new function vgic_arch_unsetup() for arm64.
> 
> No. Just look at the code. Why don't you just move the
> kvm_register_device_ops call *before* enabling the interrupt?
> 

Only based on the current code, what you said is reasonable to me.

But in the normal initializing sequence, firstly for architecture
dependence features, then for common cpu features, at last for other
devices (at least, other devices need be the last).

So for me, we need still remain current initializing sequence for
extensible in the future.


Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/