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

[Jijie Shao <shaojijie@huawei.com>]

on 2026/11/3 18:18, netdev-bot+sashiko@kernel.org wrote:
> 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()?

Acked. The NULL deref in hns3_nic_dealloc_vector_data() cannot
happen because the failure path jumps past it. The real reason
for the reset is to protect code that indexes priv->tqp_vector
by priv->vector_num while the netdev stays alive after a failed
reset re-init (e.g. hns3_dump_coal_info()). Will adjust the
commit message in v4.