Re: [PATCH v2 05/13] irqchip/gic-v3-its: Add support for the ITS emulation setup

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The intended API design dictates calling `on_each_cpu()` while hardware interrupts are disabled, which triggers SMP deadlocks.
- [High] Page allocations are performed while holding raw spinlocks, violating PREEMPT_RT lock nesting rules and causing 'sleeping while atomic' panics.
- [High] Partial failure during ITS emulation setup leaves the system in a broken, partially-emulated state with no rollback.
- [Medium] Defensive check `WARN_ON(!flags)` in `its_emulate_release_locks` leads to a NULL pointer dereference and unbalanced unlocks on the error path.
- [Medium] Acquiring an unbounded number of identical locks triggers lockdep's same-lock-class tracking.
--

--- Patch [5]: [PATCH v2 05/13] irqchip/gic-v3-its: Add support for the ITS emulation setup ---
commit 96beb9b6951a65e060abceadeb7be17ef57b5218
Author: Sebastian Ene <[email protected]>

irqchip/gic-v3-its: Add support for the ITS emulation setup

This commit introduces helper functions to lock the ITS and duplicate its state
for pKVM emulation. It allows an emulation layer to hide original hardware
tables and command queues from the host.

> pkvm_drop_host_privileges()
>         its_emulate_acquire_locks(&flags);
>                 on_each_cpu(_kvm_host_prot_finalize, &ret, 1);
>         its_emulate_release_locks(ret, &flags, pkvm_its_emulate_setup);

[Severity: High]
Will this sequence trigger a deadlock? its_emulate_acquire_locks() uses
raw_spin_lock_irqsave(), disabling local interrupts. Calling on_each_cpu()
sends cross-CPU IPIs and waits for completion, which is strictly
prohibited while interrupts are disabled because the local CPU cannot
process incoming IPIs.

> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c

[ ... ]

> +static struct its_host_state *its_snapshot_host_state(struct its_node *its)
> +{
> +	void *page;
> +	struct its_host_state *snapshot;
> +	int i;
> +
> +	page = its_alloc_pages_node(its->numa_node, GFP_ATOMIC | __GFP_ZERO, 0);

[Severity: High]
Is it safe to call the page allocator here? This function is called from
its_emulate_switch_queues_locked() inside the its_emulate_release_locks()
loop, where both the global its_lock and the local its->lock raw spinlocks
are held.

Even with GFP_ATOMIC, the page allocator acquires zone->lock, which is a
sleeping rt_mutex on PREEMPT_RT kernels. Acquiring a sleepable lock while
holding a raw spinlock causes a sleeping while atomic panic.

[ ... ]

> +void its_emulate_acquire_locks(unsigned long *flags)
> +{
> +	struct its_node *its;
> +
> +	if (WARN_ON(!flags))
> +		return;
> +
> +	raw_spin_lock_irqsave(&its_lock, *flags);
> +
> +	list_for_each_entry(its, &its_nodes, entry)
> +		raw_spin_lock(&its->lock);
> +}

[Severity: Medium]
Does holding all these identical locks concurrently trigger lockdep? Since
all its->lock instances belong to the same lock class, acquiring an
unbounded number of them sequentially without unlocking or using nested
annotations will trigger lockdep's same-lock-class tracking and disable
further lockdep checks.

> +int its_emulate_release_locks(int ret_pkvm_finalize, unsigned long *flags, its_emulate_setup cb)
> +{
> +	struct its_node *its;
> +	int ret = 0;
> +
> +	if (WARN_ON(!flags || !cb))
> +		ret = -EINVAL;
> +
> +	list_for_each_entry(its, &its_nodes, entry) {
> +		if (!ret_pkvm_finalize && !ret)
> +			ret = its_emulate_switch_queues_locked(its, cb);
> +
> +		raw_spin_unlock(&its->lock);
> +	}
> +
> +	raw_spin_unlock_irqrestore(&its_lock, *flags);
> +
> +	return ret;
> +}

[Severity: Medium]
If flags is NULL, the WARN_ON triggers and sets ret = -EINVAL, but execution
continues. Doesn't this lead to a NULL pointer dereference in
raw_spin_unlock_irqrestore() at the end of the function, as well as
unbalanced unlocks for all its->lock instances (since the acquire
function returns early)?

[Severity: High]
If its_emulate_switch_queues_locked() fails for a node, ret is populated
and subsequent nodes are skipped. However, what happens to the nodes that
were already successfully switched? 

Without rolling back the hardware state for those previous nodes, the
system is left partially emulated, which would cause a loss of
synchronization between the driver and hardware and lead to IOMMU or
interrupt faults.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5