Re: [PATCH net-next v2 2/3] dinghai: add MSI-X interrupt pools

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260911115929435mXJzcTF08xlUAdjnT8qqm@xxxxxxxxxx?part=2
---
dinghai: add MSI-X interrupt pools

This commit introduces a driver for ZTE DingHai Ethernet, specifically
allocating the fixed MSI-X vector layout of the device. It manages the
vectors in per-purpose pools for async event queues and vq queue pairs.

[Severity: Medium]
The commit message states:

  manage the vectors in per-purpose pools: one pool for the async event queues
  and one for the vq queue pairs, with a range reserved for RDMA in between.

Was the VQ pool allocation omitted from this patch by mistake? Looking at
zxdh_pf_irq_pools_init(), only the async pool is allocated.

> diff --git a/drivers/net/ethernet/zte/dinghai/zxdh_irq.c b/drivers/net/ethernet/zte/dinghai/zxdh_irq.c
> new file mode 100644
> index 0000000000000..437344329c7e8
> --- /dev/null
> +++ b/drivers/net/ethernet/zte/dinghai/zxdh_irq.c
[ ... ]
> +static void zxdh_irq_release(struct kref *kref)
> +{
> +	struct zxdh_irq *irq = container_of(kref, struct zxdh_irq, refcount);
> +	struct zxdh_irq_pool *pool = irq->pool;
> +
> +	lockdep_assert_held(&pool->lock);
> +	xa_erase(&pool->irqs, irq->index);
> +	/* free_irq() requires the affinity hint to be cleared before it is
> +	 * called; the asymmetry with the set path in zxdh_irq_alloc() is
> +	 * intentional.
> +	 */
> +	irq_update_affinity_hint(irq->irqn, NULL);
> +	free_cpumask_var(irq->mask);
> +	free_irq(irq->irqn, irq);
> +	kfree(irq);
> +}

[Severity: High]
Does this code leak the logical counters in pool->irqs_per_cpu?

When an IRQ is allocated, zxdh_irq_pool_request_irq() increments
pool->irqs_per_cpu[cpu]. It seems zxdh_irq_release() misses a call to
zxdh_cpu_put(), which will permanently skew load balancing and trigger the
WARN_ON(pool->irqs_per_cpu[cpu]) during zxdh_irq_pool_free().

[ ... ]
> +static struct zxdh_irq *zxdh_irq_alloc(struct zxdh_irq_pool *pool, int vecidx,
> +				       const struct cpumask *affinity)
> +{
> +	struct zxdh_core_dev *zxdh_dev = pool->dev;
> +	struct zxdh_irq *irq;
> +	int err;
> +	int cpu;
> +
> +	irq = kzalloc_obj(*irq, GFP_KERNEL);
> +	if (!irq)
> +		return ERR_PTR(-ENOMEM);
> +
> +	irq->pool = pool;
> +	irq->irqn = pci_irq_vector(zxdh_dev->pdev, vecidx);
> +	if (irq->irqn < 0) {
> +		err = irq->irqn;
> +		goto err_irqn;
> +	}
> +
> +	ATOMIC_INIT_NOTIFIER_HEAD(&irq->nh);
> +	snprintf(irq->name, ZXDH_MAX_IRQ_NAME, "async_%d@pci:%s", vecidx,
> +		 pci_name(zxdh_dev->pdev));

[Severity: Low]
Is this hardcoded name prefix intentional?

This ignores the configured pool->name. If this generic library is reused for
other pools, such as the VQ pool, they will all show up as "async_" in
/proc/interrupts.

> +
> +	err = request_irq(irq->irqn, zxdh_irq_int_handler, 0, irq->name, irq);
> +	if (err) {
> +		dev_err(zxdh_dev->device, "request_irq failed: %d\n", err);
> +		goto err_irqn;
> +	}
> +
> +	if (!zalloc_cpumask_var(&irq->mask, GFP_KERNEL)) {
> +		dev_err(zxdh_dev->device, "zalloc_cpumask_var failed\n");
> +		err = -ENOMEM;
> +		goto err_cpumask;
> +	}
> +
> +	if (affinity) {
> +		cpumask_copy(irq->mask, affinity);
> +	} else {
> +		/* No preference requested; spread over all online CPUs. */
> +		for_each_online_cpu(cpu)
> +			cpumask_set_cpu(cpu, irq->mask);
> +	}
> +	irq_update_affinity_hint(irq->irqn, irq->mask);

[Severity: Medium]
Will this actually configure the SMP hardware routing for the IRQ?

The driver uses irq_update_affinity_hint() which only sets a userspace hint
via procfs. Should this be irq_set_affinity_and_hint() instead to actually
enforce the intended hardware IRQ load balancing?

[ ... ]
> +static struct zxdh_irq *zxdh_irq_pool_request_irq(struct zxdh_irq_pool *pool,
> +						  const struct cpumask *req_mask)
> +{
> +	const struct cpumask *affinity = req_mask;
> +	cpumask_var_t auto_mask;
> +	struct zxdh_irq *irq;
> +	u32 irq_index;
> +	int err;
> +
> +	if (!zalloc_cpumask_var(&auto_mask, GFP_KERNEL))
> +		return ERR_PTR(-ENOMEM);
> +
> +	err = xa_alloc(&pool->irqs, &irq_index, NULL, pool->xa_num_irqs,
> +		       GFP_KERNEL);

[Severity: High]
Will this xa_alloc() call fail unconditionally?

The XArray API strictly forbids NULL entries for allocation because NULL
represents an unallocated slot. Passing NULL as the entry triggers a
WARN_ON_ONCE(!entry) and returns -EINVAL.

[ ... ]
> +void zxdh_irq_pool_free(struct zxdh_irq_pool *pool)
> +{
> +	struct zxdh_irq *irq;
> +	unsigned long index;
> +	u32 cpu;
> +
> +	/* On a fast teardown the table may still hold IRQs; release
> +	 * whatever is left.
> +	 */
> +	mutex_lock(&pool->lock);
> +	xa_for_each(&pool->irqs, index, irq)
> +		zxdh_irq_release(&irq->refcount);
> +	mutex_unlock(&pool->lock);

[Severity: High]
Can this manual iteration bypass the reference counting and cause a
use-after-free?

If zxdh_irq_pool_free() executes while active consumers still hold an IRQ
reference during a fast teardown, it will forcefully execute
zxdh_irq_release() on the irq object. When the consumer eventually calls
zxdh_irq_put() and accesses the freed irq and its freed pool->lock, will
this trigger a use-after-free?

[ ... ]