Re: [PATCH v2 net 1/3] net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says it sets `priv->vector_num = 0` in the new
  `err_put_vector` path of `hns3_nic_alloc_vector_data()`…

Pre-existing issues:
- [Medium] `hns3_nic_alloc_vector_data()` accepts a 0 return from
  `get_vector()` as success.

--

net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data

This patch fixes the devm_kcalloc() failure path in
hns3_nic_alloc_vector_data(). On that path, the MSI-X vectors obtained
from get_vector() are now handed back with put_vector() and
priv->vector_num is reset to 0. The success and error exits are also
split.

> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> index 47788be64be68..2f3aeade558cd 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> @@ -4854,7 +4854,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv)
>  			     GFP_KERNEL);
>  	if (!priv->tqp_vector) {
>  		ret = -ENOMEM;
> -		goto out;
> +		goto err_put_vector;
>  	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. What
happens if get_vector() returns 0 here?

hns3_nic_alloc_vector_data() {
    ...
	vector_num = h->ae_algo->ops->get_vector(h, vector_num, vector);

	priv->vector_num = vector_num;
	priv->tqp_vector = (struct hns3_enet_tqp_vector *)
		devm_kcalloc(&pdev->dev, vector_num, sizeof(*priv->tqp_vector),
			     GFP_KERNEL);
    ...
}

With a zero count, devm_kcalloc() returns ZERO_SIZE_PTR rather than NULL.
The -ENOMEM check above would not fire, and the function would return
success.

hns3_nic_init_vector_data() would then run:

	for (i = 0; i < h->kinfo.num_tqps; i++) {
		u16 vector_i = i % priv->vector_num;
		...
		tqp_vector = &priv->tqp_vector[vector_i];

Wouldn't that divide by zero, with priv->tqp_vector pointing at
ZERO_SIZE_PTR?

This looks hard to hit in practice. In hclge_get_vector(), a zero return
needs num_msi_left (or num_nic_msi - 1) to be 0. The HNAE3_MIN_VECTOR_NUM
checks and the pci_alloc_irq_vectors() minimum normally prevent that. The
leak closed by this patch was the only known way to drain num_msi_left.

Would a zero check on the get_vector() result still be worth adding?

[ ... ]

> -out:
> +	devm_kfree(&pdev->dev, vector);
> +	return 0;
> +
> +err_put_vector:
> +	for (i = 0; i < vector_num; i++)
> +		h->ae_algo->ops->put_vector(h, vector[i].vector);
> +	priv->vector_num = 0;

[Severity: Low]
The commit message says:

    Call put_vector() for every vector acquired by get_vector() before
    returning an error, and reset priv->vector_num so the dealloc path
    cannot dereference a NULL priv->tqp_vector.

Is the dealloc path reachable after hns3_nic_alloc_vector_data() fails?

On failure, hns3_client_init() jumps to out_alloc_vector_data, and
hns3_reset_notify_init_enet() jumps to err_put_ring:

	ret = hns3_nic_alloc_vector_data(priv);
	if (ret)
		goto err_put_ring;

Both labels are below the calls to hns3_nic_uninit_vector_data() and
hns3_nic_dealloc_vector_data(), so neither helper runs.

The later teardown paths, hns3_reset_notify_uninit_enet() and
hns3_client_uninit(), return early. HNS3_NIC_STATE_INITED is only set
after init fully succeeds:

	if (!test_and_clear_bit(HNS3_NIC_STATE_INITED, &priv->state)) {
		netdev_warn(netdev, "already uninitialized\n");
		return 0;
	}

hns3_change_channels() also only calls UNINIT_CLIENT once INIT_CLIENT has
succeeded.

The reader that does benefit from the reset seems to be the debugfs
coalesce dump. hns3_dump_coal_info() loops over priv->vector_num and
indexes priv->tqp_vector[i] without an INITED check:

	for (i = 0; i < priv->vector_num; i++) {
		tqp_vector = &priv->tqp_vector[i];
		hns3_get_coal_info(tqp_vector, s, i, is_tx);
	}

Could the commit message name that path instead of the dealloc path?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021121426.3096838-1-shaojijie%40huawei.com