Re: [PATCH v3 net 1/4] net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data

[netdev-bot+sashiko@kernel.org]

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 does not give the real reason for one line of
  the fix.

--

net: hns3: fix vector resource leak in hns3_nic_alloc_vector_data

This patch fixes the error path in hns3_nic_alloc_vector_data() for when
devm_kcalloc() of priv->tqp_vector fails. The new path calls put_vector()
for every vector that get_vector() acquired, and resets priv->vector_num
to 0 before returning.

> Call put_vector() for every acquired vector before returning the
> error, and reset priv->vector_num to 0 to avoid a NULL deref in the
> subsequent hns3_nic_dealloc_vector_data(), which iterates
> priv->tqp_vector (still NULL after the kcalloc failure) using
> priv->vector_num.

[Severity: Low]
Is this the right reason for the priv->vector_num reset? As far as I can
tell, no path calls hns3_nic_dealloc_vector_data() after
hns3_nic_alloc_vector_data() fails.

In hns3_client_init(), the failure jumps to a label that comes after the
dealloc call:

drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_client_init() {
    ...
	ret = hns3_nic_alloc_vector_data(priv);
	if (ret) {
		ret = -ENOMEM;
		goto out_alloc_vector_data;
	}
    ...
out_init_vector_data:
	hns3_nic_dealloc_vector_data(priv);
out_alloc_vector_data:
    ...
}

hns3_reset_notify_init_enet() works the same way:

drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_reset_notify_init_enet() {
    ...
	ret = hns3_nic_alloc_vector_data(priv);
	if (ret)
		goto err_put_ring;
    ...
err_dealloc_vector:
	hns3_nic_dealloc_vector_data(priv);
err_put_ring:
    ...
}

hns3_client_uninit() and hns3_reset_notify_uninit_enet() both start with
test_and_clear_bit(HNS3_NIC_STATE_INITED, &priv->state). They bail out
before calling hns3_nic_dealloc_vector_data() if the bit is clear.

HNS3_NIC_STATE_INITED is only set once init succeeds, so it is still
clear after this failure. That means the NULL deref described above
cannot happen, with or without this patch.

The reset may still be useful for other code that indexes
priv->tqp_vector by priv->vector_num while the netdev stays alive after
a failed reset re-init. One example is hns3_dump_coal_info() in
hns3_debugfs.c:

	for (i = 0; i < priv->vector_num; i++) {
		tqp_vector = &priv->tqp_vector[i];

Should the commit message name the code that actually depends on
priv->vector_num being 0, rather than hns3_nic_dealloc_vector_data()?

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