Re: [PATCH net-next v2 3/3] dinghai: add async event queue for firmware notifications

[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=3
---
dinghai: add async event queue for firmware notifications

This commit adds an event queue table and an async event queue for firmware
notifications. The queue claims an MSI-X vector from the async pool and maps
firmware events to notifier chains.

> diff --git a/drivers/net/ethernet/zte/dinghai/en_pf.c b/drivers/net/ethernet/zte/dinghai/en_pf.c
> --- a/drivers/net/ethernet/zte/dinghai/en_pf.c
> +++ b/drivers/net/ethernet/zte/dinghai/en_pf.c
[ ... ]
> @@ -612,12 +612,26 @@ static int zxdh_pf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto err_irq_table;
>  	}
>  
> +	ret = zxdh_pf_eq_table_init(zxdh_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "zxdh_pf_eq_table_init failed: %d\n", ret);
> +		goto err_eq_table;
> +	}
> +
> +	ret = zxdh_pf_eq_table_create(zxdh_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "zxdh_pf_eq_table_create failed: %d\n", ret);
> +		goto err_eq_table;
> +	}
> +
>  	devlink_register(devlink);
>  
>  	return 0;
>  
> +err_eq_table:
> +	zxdh_pf_eq_table_destroy(zxdh_dev);
>  err_irq_table:
> -	kvfree(zxdh_dev->irq_table.priv);
> +	zxdh_pf_irq_table_destroy(zxdh_dev);
>  err_cfg_init:
>  	zxdh_pf_pci_close(zxdh_dev);

[Severity: Medium]
Does this error path leak the modern configuration MMIO mappings?

The zxdh_pf_modern_cfg_init() function successfully maps several capabilities.
If a subsequent step fails (such as zxdh_pf_eq_table_init() or other device
initialization checks), the code jumps to err_eq_table and falls through to
err_cfg_init. However, this label only calls zxdh_pf_pci_close() and misses
a call to zxdh_pf_modern_cfg_uninit(), leaving the memory mapped.

>  err_pci_init:
[ ... ]
> diff --git a/drivers/net/ethernet/zte/dinghai/zxdh_eq.c b/drivers/net/ethernet/zte/dinghai/zxdh_eq.c
> --- /dev/null
> +++ b/drivers/net/ethernet/zte/dinghai/zxdh_eq.c
[ ... ]
> +static u16 zxdh_eq_event_id_get(struct zxdh_core_dev *zxdh_dev)
> +{
> +	struct zxdh_pf_dev *pf_dev = zxdh_dev->priv;
> +	void __iomem *subchan;
> +
> +	subchan = pf_dev->pci_ioremap_addr[0] + ZXDH_BAR_MSG_SUBCHAN_RECV;
> +
> +	return ioread32(subchan) >> 16;
> +}

[Severity: High]
Is there a risk of an out-of-bounds MMIO read here?

The device blindly maps BAR 0 based on pci_resource_len() during
zxdh_pf_pci_init(). If the hardware or an emulated environment exposes a
truncated BAR 0 length that is smaller than 0x2804 bytes, reading from
ZXDH_BAR_MSG_SUBCHAN_RECV (offset 0x2800) could exceed the mapped boundaries
and cause a page fault or oops.

[ ... ]
> +int zxdh_pf_eq_table_create(struct zxdh_core_dev *zxdh_dev)
> +{
> +	struct zxdh_pf_eq_table *pf_eq_table = zxdh_dev->eq_table.priv;
> +	struct zxdh_eq_table *table = &zxdh_dev->eq_table;
> +	struct zxdh_eq_async *eq = &pf_eq_table->riscv_eq;
> +	int err;
> +
> +	mutex_lock(&table->lock);
> +
> +	eq->priv = zxdh_dev;
> +	eq->irq = zxdh_pf_async_irq_request(zxdh_dev);
> +	if (IS_ERR(eq->irq)) {
> +		err = PTR_ERR(eq->irq);
> +		goto unlock;
> +	}

[Severity: High]
Could this lead to an invalid pointer dereference on the cleanup path?

If zxdh_pf_async_irq_request() fails (e.g., due to memory exhaustion), it
returns an error pointer like ERR_PTR(-ENOMEM). This error pointer is
assigned to eq->irq but is not cleared to NULL before jumping to unlock.

When the error propagates, zxdh_pf_probe() jumps to the err_eq_table label
and calls zxdh_pf_eq_table_destroy().

Because eq->irq contains an error pointer, the cleanup check in
zxdh_pf_eq_table_destroy():

    if (eq->irq) {
        zxdh_irq_detach_nb(eq->irq, &eq->irq_nb);
        ...

will evaluate to true, and zxdh_irq_detach_nb() will attempt to dereference
the error pointer, causing a kernel panic.

> +
> +	eq->irq_nb.notifier_call = zxdh_eq_async_riscv_int;
> +	err = zxdh_irq_attach_nb(eq->irq, &eq->irq_nb);
[ ... ]