Re: [PATCH v3 0/1] KVM: arm64: vgic: Drop last_lr_irq and serialize overflow EOI replay

[Marc Zyngier]

On Sun, 20 Sep 2026 02:49:10 +0100,
Yuchao Zhang <ndaugoing@xxxxxxxxx> wrote:
> 
> Hi Marc, Oliver, Fuad, and KVM/arm64 maintainers,
> 
> Following up on the discussion around the remote LPI disable vs LR fold
> race [1], this series addresses the issue at its root: the last_lr_irq
> cursor introduced in commit 6da5e537f5af ("KVM: arm64: vgic: Pick EOIcount
> deactivations from AP-list tail").
> 
> Problem:
> vgic_v3_fold_lr_state() / vgic_v2_fold_lr_state() walk the overflow tail
> of the ap_list starting from *host_data_ptr(last_lr_irq) without holding
> ap_list_lock. Caching this raw pointer across the entire guest execution
> leaves it vulnerable to concurrent modification: when a remote vCPU
> disables LPIs via GICR_CTLR, vgic_flush_pending_lpis() unlinks the node
> with list_del() and drops its reference, leaving last_lr_irq pointing to
> a poisoned or freed object. When the vCPU exits, the walk dereferences
> corrupted memory, causing a kernel panic or UAF.

Why can't this be solved by simply taking a reference on the object
pointed to by last_lr_irq?

> 
> Oliver and Marc suggested taking ap_list_lock in
> vgic_v3_fold_lr_state() [2][3]. I tried that approach first, but it
> runs into the following lock-order problems:
> 1. kvm_notify_acked_irq() grabs regular spinlocks and can re-enter
>    vgic_queue_irq_unlock() (which takes ap_list_lock), causing deadlock.
> 2. vgic_put_irq() is a no-op for SPI/PPI, but for LPIs it calls
>    refcount_dec_and_lock_irqsave() which acquires dist->lpi_xa.xa_lock
>    when dropping the last reference. That lock sits above ap_list_lock
>    in the lock ordering, so calling vgic_put_irq() under ap_list_lock
>    causes lock inversion.

Which is why we have vgic_put_irq_norelease() and
vgic_release_deleted_lpis(), which allow deferring the release until
we're in a suitable context. But that's beside the point.

> Addressing these under a global fold lock requires deferring all EOI'ed
> SPI notifications to a stack bitmap and deferring LPI releases with

Which stack bitmap?

> vgic_put_irq_norelease(), penalizing the fast path for all exits even
> though folding hardware LRs does not touch ap_list at all. It also still
> requires pinning and clearing the per-CPU last_lr_irq pointer.

An uncontended atomic access is hardly an overhead, is it? Where is
the overhead? And I don't understand what you're saying about the LRs
not affecting the ap_list... They *always* do.

> 
> Changes since v2:
> - Replaced the skip-unlink approach of v2 with dropping the last_lr_irq
>   cursor entirely and serializing only the overflow EOI replay under
>   ap_list_lock (per Oliver and Marc's suggestion [2][3]).
> 
> The cleaner approach in this patch:
> 1. Drop the fragile last_lr_irq per-CPU cursor entirely.
> 2. The common fast path (folding hardware LRs) runs natively without
>    ap_list_lock. We record the INTIDs of the used LRs in a small stack
>    array (VGIC_V3_MAX_LRS / VGIC_V2_MAX_LRS entries).

Why is v2 even under consideration? LPIs are strictly v3 (ignoring
v5 here), and non-LPIs are statically allocated, meaning they can't
vanish under your feet.

> 3. If eoicount == 0 (the vast majority of guest exits), clear
>    cpuif->used_lrs = 0 and return immediately without taking ap_list_lock.
> 4. If unlikely(eoicount > 0), acquire ap_list_lock only to scan the ap_list
>    and pin (via vgic_get_irq_ref) up to eoicount active interrupts that
>    were not in hardware LRs. The scan is a linear walk over at most 16/64
>    LR INTIDs per candidate, on the rare eoicount > 0 path - bounded and

What makes you think this is acceptable? It really isn't. The point is
that this is not limited to 16 entries. That's the whole point of
EOIcount, which spans up to 31 simultaneously active priorities.

I have no idea what you describe is achieving, TBH. And looking at the
patch, I see a quadratic behaviour, which doesn't strike me as low
overhead...

>    acceptable. lr_intids[] only records INTIDs from the used_lrs range;
>    the extraction mask mirrors vgic_fold_lr() exactly
>    (ICH_LR_VIRTUAL_ID_MASK for GICv3, GICH_LR_VIRTUALID for GICv2),
>    so no stale or invalid slot can produce a false match.
> 5. Drop ap_list_lock immediately, and then replay their deactivations
>    outside the lock, naturally eliminating both eventfd re-entrancy and
>    lpi_xa lock inversions without changing any function signatures.
>    vgic_fold_lr() has no error path, so cpuif->used_lrs = 0 is always
>    reached after a complete fold, with no risk of partial cleanup.
> 
> Note on EOIcount hardware limits:
> ICH_HCR_EL2.EOIcount (GICv3) and GICH_HCR.EOICount (GICv2) are both
> 5-bit fields, giving a maximum value of 31. The targets[32] stack array
> and min_t(u32, eoicount, ARRAY_SIZE(targets)) bound together ensure no
> overflow even if hardware writes an unexpected value.
> 
> Note on EOIcount source:
> For GICv3, eoicount is read from cpuif->vgic_hcr, which is populated by
> __vgic_v3_save_state right before ICH_HCR_EL2 is cleared in hardware.
> For GICv2, vgic_v2_save_state reads GICH_HCR via MMIO into the same
> cpuif->vgic_hcr field (when LRENPIE is set) before writing 0 to GICH_HCR.
> In both cases the software copy is the only valid source; reading the
> hardware register after save would return 0.
> 
> Note on scope:
> This series fixes the use-after-free in the ap_list traversal caused by
> last_lr_irq. It does not address the separate concern raised by Oliver
> in [2] about a pending LPI still sitting in an LR when RWP=0 becomes
> visible to another vCPU; that may require a stronger approach (e.g.
> halting the VM) and I am happy to follow up separately.

This has all the hallmarks of an AI gone wild.

The problem is correctly described (last_lr_irq doesn't hold a
reference on the irq), but the proposed solution is completely
ignoring it, and implements... something else.

I've hacked something at [1], which:

- changes the behaviour of last_lr_irq to only be non-NULL when the
  LRs are full.

- take a reference on the irq flagged as last_lr_irq, and drop this
  reference in vgic_prune_ap_list(), contributing to the LPIs being
  freed once the ap_list_lock is dropped.

- stop the world when a vcpu disable LPIs. We could do slightly
  better, but it isn't worth the hassle for something that *never*
  happens.

I only boot tested a small nested guest (L1 + L2) with lockdep on my
laptop, and nothing caught fire. Please give it a go.

You also seem to have a reproducer for this, it'd be good if you could
turn it into a selftest.

Thanks,

	M.

[1] https://web.git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/vgic-last_lr_irq-fixes

-- 
Jazz isn't dead. It just smells funny.