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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 30 Aug 2017 15:46:12 +0100 From: Marc Zyngier <marc.zyngier@....com> To: Christoffer Dall <cdall@...aro.org> Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu, kvm@...r.kernel.org, Christoffer Dall <christoffer.dall@...aro.org>, Thomas Gleixner <tglx@...utronix.de>, Jason Cooper <jason@...edaemon.net>, Eric Auger <eric.auger@...hat.com>, Shanker Donthineni <shankerd@...eaurora.org>, Mark Rutland <mark.rutland@....com>, Shameerali Kolothum Thodi <shameerali.kolothum.thodi@...wei.com> Subject: Re: [PATCH v3 46/59] KVM: arm/arm64: GICv4: Handle MOVALL applied to a vPE On 28/08/17 19:18, Christoffer Dall wrote: > On Mon, Jul 31, 2017 at 06:26:24PM +0100, Marc Zyngier wrote: >> The current implementation of MOVALL doesn't allow us to call >> into the core ITS code as we hold a number of spinlocks. >> >> Let's try a method used in other parts of the code, were we copy >> the intids of the candicate interrupts, and then do whatever >> we need to do with them outside of the critical section. >> >> This allows us to move the interrupts one by one, at the expense >> of a bit of CPU time. Who cares? MOVALL is such a stupid command >> anyway... >> >> Signed-off-by: Marc Zyngier <marc.zyngier@....com> >> --- >> virt/kvm/arm/vgic/vgic-its.c | 27 ++++++++++++++++++++------- >> 1 file changed, 20 insertions(+), 7 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index 2c065c970ba0..65cc77fde609 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -1147,11 +1147,12 @@ static int vgic_its_cmd_handle_invall(struct kvm *kvm, struct vgic_its *its, >> static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its, >> u64 *its_cmd) >> { >> - struct vgic_dist *dist = &kvm->arch.vgic; >> u32 target1_addr = its_cmd_get_target_addr(its_cmd); >> u32 target2_addr = its_cmd_mask_field(its_cmd, 3, 16, 32); >> struct kvm_vcpu *vcpu1, *vcpu2; >> struct vgic_irq *irq; >> + u32 *intids; >> + int irq_count, i; >> >> if (target1_addr >= atomic_read(&kvm->online_vcpus) || >> target2_addr >= atomic_read(&kvm->online_vcpus)) >> @@ -1163,19 +1164,31 @@ static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its, >> vcpu1 = kvm_get_vcpu(kvm, target1_addr); >> vcpu2 = kvm_get_vcpu(kvm, target2_addr); >> >> - spin_lock(&dist->lpi_list_lock); >> + irq_count = vgic_copy_lpi_list(vcpu1, &intids); >> + if (irq_count < 0) >> + return irq_count; >> >> - list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { >> - spin_lock(&irq->irq_lock); >> + for (i = 0; i < irq_count; i++) { >> + irq = vgic_get_irq(kvm, NULL, intids[i]); >> + if (!irq) >> + continue; > > Getting irq == NULL means that we've removed this LPI since > vgic_copy_lpi_list, right? Can this really happen while we hold the its > mutex? A disappearing LPI can only be the result of a DISCARD, which cannot happen, as we indeed hold the ITS lock. > Also, we don't check this in its_sync_lpi_pending_table which would > indicate that we either have a bug there or are being overly careful > here (or should change the continue to BUG). Let's aim for consistency. I'll drop this test. > > >> >> if (irq->target_vcpu == vcpu1) >> irq->target_vcpu = vcpu2; >> >> - spin_unlock(&irq->irq_lock); > > Is it safe to modify target_vcpu without holding the irq_lock? Unintentional regression. I'll fix that. But I wonder if there is an actual point in testing testing the target_vcpu here. Since we hold the ITS lock, we're damn sure that the affinity can't be changed, right? Thanks, M. -- Jazz is not dead. It just smells funny...
Powered by blists - more mailing lists