Re: [PATCH v3 net 4/4] net: hns3: fix use-after-free in debugfs read during reset/unload

[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 6 potential
> issue(s) to consider.
>
> Critical: 0 · High: 2 · Medium: 1 (1) · Low: 3
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] Within this series, the earlier commit "net: hns3: fix interface
>    stuck after OOM with full hardware rx queue" makes…
> - [High] net: hns3: uncanceled oom_task in hns3_client_uninit() and reset
>    uninit
> - [Medium] The init-side writers that rebuild and republish the debugfs-
>    visible resources never take dbg_mutex, and they publish readiness…
> - [Low] hns3_set_ringparam() is the third dbg_mutex writer, and it does
>    not follow the writer protocol that the commit message and comments…
> - [Low] The patch changes what userspace sees from several hns3 debugfs
>    files, and the permanent commit body does not mention it.
> - [Low] Sibling debugfs readers now report the same device state with
>    different errno values.
>
> Pre-existing issues:
> - [Medium] In hns3_set_ringparam(), when hns3_init_all_ring() fails, ret
>    holds the error and the old ring configuration is restored.
>
> --
>
> net: hns3: fix use-after-free in debugfs read during reset/unload
>
> This patch adds a dbg_mutex to struct hnae3_handle. The enet-layer
> debugfs show callbacks take it, and so do the reset uninit, driver unload
> and ethtool ring resize paths while they free or rebuild rings and
> vectors. Readers also re-check the INITED/RESETTING state bits on every
> loop iteration through a new helper, hns3_dbg_is_device_busy().
>
> Closes: https://lore.kernel.org/all/a0853cd9-cab5-441d-b181-8ba97f2f58b0@huawei.com/
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
>> index 1347edac76993..ec08672372f03 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
> [ ... ]
>
>> @@ -448,18 +454,34 @@ static void hns3_dump_coal_info(struct seq_file *s, bool is_tx)
>>   	seq_puts(s, "HW_GL  HW_QL\n");
>>   
>>   	for (i = 0; i < priv->vector_num; i++) {
>> +		/* Per-iteration check bounds reset latency to one iteration. */
>> +		if (hns3_dbg_is_device_busy(priv))
>> +			return -EBUSY;
>> +
>>   		tqp_vector = &priv->tqp_vector[i];
>>   		hns3_get_coal_info(tqp_vector, s, i, is_tx);
>>   	}
> [Severity: Medium]
> Is the state check enough to order these readers against the reset init
> side?
>
> The uninit writers take dbg_mutex, but the init path that republishes
> these resources does not. hns3_reset_notify_init_enet() assigns new
> priv->ring, priv->tqp_vector, priv->vector_num and ring->desc without
> dbg_mutex. It then publishes readiness with a plain bitop:
>
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_reset_notify_init_enet() {
>      ...
> 	ret = hns3_get_ring_config(priv);
>      ...
> 	ret = hns3_nic_alloc_vector_data(priv);
>      ...
> 	ret = hns3_init_all_ring(priv);
>      ...
> 	set_bit(HNS3_NIC_STATE_INITED, &priv->state);
>      ...
> }
>
> Later, hns3_reset_notify_up_enet() runs
> clear_bit(HNS3_NIC_STATE_RESETTING, &priv->state), again without
> dbg_mutex.
>
> Neither set_bit() nor clear_bit() is ordered, and the test_bit() calls in
> hns3_dbg_is_device_busy() have no acquire semantics. The reader's
> mutex_lock() pairs only with an earlier unlock by an uninit writer or
> another reader. It never pairs with the init writer.
>
> On arm64, could a reader see INITED set and RESETTING clear but still load
> the old priv->tqp_vector here? hns3_nic_dealloc_vector_data() frees that
> array with devm_kfree() and does not clear the pointer, so the stale value
> would point to freed memory. For priv->ring, the stale value would be NULL.
>
> The window is probably very small in practice. Still, the publish side
> relies only on plain state bits, which is the pattern the commit message
> calls insufficient.
>
> Would it work to set INITED and clear RESETTING while holding dbg_mutex?
> Another option is to pair test_bit_acquire() on the reader side with
> release ordering on the writer side.

Acknowledged, pre-existing (already reported on v2 3/3). Not
addressed in this series.

>
> [ ... ]
>
>> @@ -504,27 +526,22 @@ static int hns3_dbg_rx_queue_info(struct seq_file *s, void *data)
>>   	struct hns3_enet_ring *ring;
>>   	u32 i;
>>   
>> -	if (!priv->ring) {
>> -		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
>> -		return -EFAULT;
>> -	}
>> -
>> +	mutex_lock(&priv->ae_handle->dbg_mutex);
>>   	seq_puts(s, "QUEUE_ID  BD_NUM  BD_LEN  TAIL  HEAD  FBDNUM  ");
>>   	seq_puts(s, "PKTNUM     COPYBREAK  RING_EN  RX_RING_EN  BASE_ADDR\n");
>>   
>>   	for (i = 0; i < h->kinfo.num_tqps; i++) {
>> -		/* Each cycle needs to determine whether the instance is reset,
>> -		 * to prevent reference to invalid memory. And need to ensure
>> -		 * that the following code is executed within 100ms.
>> -		 */
>> -		if (!test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
>> -		    test_bit(HNS3_NIC_STATE_RESETTING, &priv->state))
>> -			return -EPERM;
>> +		/* Per-iteration check bounds reset latency to one iteration. */
>> +		if (hns3_dbg_is_device_busy(priv)) {
>> +			mutex_unlock(&priv->ae_handle->dbg_mutex);
>> +			return -EBUSY;
>> +		}
> [Severity: Low]
> This changes what userspace sees from several debugfs files. Only the
> v2/v3 notes below the --- line mention it, and those notes are dropped
> when the patch is applied.
>
> When the per-iteration check fails, hns3_dbg_rx_queue_info(),
> hns3_dbg_tx_queue_info() and hns3_dbg_page_pool_info() now return -EBUSY
> instead of -EPERM.
>
> The "priv->ring is NULL" dev_err() messages are gone from the rx/tx queue
> and page_pool readers.
>
> hns3_dbg_page_pool_info() also used to fail with -EFAULT and log "page
> pool is not initialized" when the first rx ring had no page_pool. It now
> succeeds and hns3_dump_page_pool_info() prints a "page pool not
> initialized" line for each ring.
>
> Could the commit message mention these changes?

Will add a short paragraph to the v4 commit message (errno
EPERM -> EBUSY, removal of dev_err() logs, page_pool reader
per-ring "page pool not initialized" line).

>
> [ ... ]
>
>> @@ -796,13 +828,9 @@ static int hns3_dbg_page_pool_info(struct seq_file *s, void *data)
>>   	struct hns3_enet_ring *ring;
>>   	u32 i;
>>   
>> +	mutex_lock(&priv->ae_handle->dbg_mutex);
>>   	if (!priv->ring) {
>> -		dev_err(&h->pdev->dev, "priv->ring is NULL\n");
>> -		return -EFAULT;
>> -	}
>> -
>> -	if (!priv->ring[h->kinfo.num_tqps].page_pool) {
>> -		dev_err(&h->pdev->dev, "page pool is not initialized\n");
>> +		mutex_unlock(&priv->ae_handle->dbg_mutex);
>>   		return -EFAULT;
>>   	}
> [Severity: Low]
> Should this return -EBUSY to match the sibling readers?
>
> INITED is clear in every window where priv->ring is NULL. With their
> pre-loop !priv->ring checks removed, hns3_dbg_rx_queue_info() and
> hns3_dbg_tx_queue_info() now return -EBUSY in that state.
>
> This reader keeps its pre-loop check and still returns -EFAULT for the
> same state. One example is after hns3_reset_notify_uninit_enet() has
> called hns3_put_ring_config() and before init has rebuilt the rings.
>
> No user buffer is involved here, so EFAULT ("Bad address") looks like the
> wrong errno.

ok.

>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> index 14642f81efe1a..7005d8b2d010b 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
>> @@ -1252,6 +1252,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
>>   	if (if_running)
>>   		ndev->netdev_ops->ndo_stop(ndev);
>>   
>> +	mutex_lock(&h->dbg_mutex);
>>   	hns3_change_all_ring_bd_num(priv, new_ringparam.tx_desc_num,
>>   				    new_ringparam.rx_desc_num);
> [Severity: Low]
> Does this writer follow the protocol described in the commit message? The
> commit message says:
>
>    Writers clear INITED first to signal readers, then acquire
>    the mutex before freeing resources.
>
> The reader loops also say "Per-iteration check bounds reset latency to one
> iteration."
>
> hns3_set_ringparam() only calls ndo_stop(), which sets
> HNS3_NIC_STATE_DOWN. Before it takes dbg_mutex and rebuilds or frees the
> rings, it never clears HNS3_NIC_STATE_INITED or sets
> HNS3_NIC_STATE_RESETTING. As a result, hns3_dbg_is_device_busy() never
> fires during a ring resize.
>
> The mutex alone still keeps readers away from the rings, so there is no
> use-after-free here. However, ethtool -G holds rtnl while it waits for a
> whole show() pass, not one iteration. That pass can be a full tx_bd_info
> or rx_bd_info dump of desc_num entries.
>
> The dbg_mutex field comment in hnae3.h ("protect debugfs against
> reset/unload") does not mention ring resize either.
>
> Should the commit message and comments be updated, or should this path
> also signal readers through the state bits?

Not an issue. hns3_set_ringparam() calls ndo_stop() before taking
dbg_mutex, so the netdev is already down while the rings are
being rebuilt. Readers are blocked by the mutex and the device is
stopped; clearing INITED/setting RESETTING is not needed on this
path.

>
>> @@ -1270,6 +1271,7 @@ static int hns3_set_ringparam(struct net_device *ndev,
>>   		for (i = 0; i < h->kinfo.num_tqps * 2; i++)
>>   			hns3_fini_ring(&tmp_rings[i]);
>>   	}
> [Severity: High]
> Can this path leak priv->rx_ring_oom_bm, or leave it NULL?
>
> The bitmap comes from the earlier patch in this series, "net: hns3: fix
> interface stuck after OOM with full hardware rx queue". That patch makes
> hns3_init_all_ring() always allocate it at the start:
>
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c:hns3_init_all_ring() {
> 	...
> 	priv->rx_ring_oom_bm = bitmap_zalloc(h->kinfo.num_tqps, GFP_KERNEL);
> 	if (!priv->rx_ring_oom_bm)
> 		return -ENOMEM;
> 	...
> out_when_alloc_ring_memory:
> 	for (j = i - 1; j >= 0; j--)
> 		hns3_fini_ring(&priv->ring[j]);
>
> 	bitmap_free(priv->rx_ring_oom_bm);
> 	priv->rx_ring_oom_bm = NULL;
> 	return -ENOMEM;
> }
>
> hns3_set_ringparam() calls hns3_init_all_ring() on a live priv, where
> rx_ring_oom_bm already points to the bitmap in use.
>
> On success, the old rings are released only through hns3_fini_ring(),
> which does not touch rx_ring_oom_bm. Only hns3_uninit_all_ring() frees
> the bitmap. Does every successful ethtool -G leak the previous bitmap?
>
> On failure, the old pointer has already been overwritten, and the new
> bitmap is freed and set to NULL. The old rings are then copied back, and
> the interface is reopened (or brought up later) with
> rx_ring_oom_bm == NULL. Every NAPI rx poll then runs:
>
> hns3_clean_rx_ring()
>      if (unlikely(failure || hns3_ring_is_oom_state(ring)))
>          hns3_oom_task_schedule(ring);
>
> hns3_ring_is_oom_state()
>      return test_bit(ring->queue_index, priv->rx_ring_oom_bm);
>
> Wouldn't the first packet received after a failed resize cause a NULL
> pointer dereference in softirq context? hns3_ring_set_oom_state() and
> hns3_oom_task() dereference the same pointer.

The High issues overlap with the 3/4 review
(rx_ring_oom_bm in set_ringparam, oom_task cancel) and are
addressed there

>
>> +	mutex_unlock(&h->dbg_mutex);
>>   
>>   	kfree(tmp_rings);
>>   
> [Severity: Medium]
> This is a pre-existing issue that was not introduced by this patch, but
> it is in the same function.
>
> When hns3_init_all_ring() fails above, ret holds the error and the old
> ring configuration is restored. The code after this hunk then does:
>
> 	if (if_running)
> 		ret = ndev->netdev_ops->ndo_open(ndev);
>
> 	return ret;
>
> If hns3_nic_net_open() brings the restored rings back up and returns 0,
> the earlier error is overwritten. Does ethtool -G then report success even
> though the requested ring parameters were not applied?

Acknowledged, pre-existing. Not addressed in this series.

>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> @@ -5601,9 +5604,11 @@ static void hns3_client_uninit(struct hnae3_handle *handle, bool reset)
>>   	hns3_uninit_all_ring(priv);
>>   
>>   	hns3_put_ring_config(priv);
>> +	mutex_unlock(&handle->dbg_mutex);
>>   
>>   out_netdev_free:
>>   	hns3_dbg_uninit(handle);
>> +	mutex_destroy(&handle->dbg_mutex);
>>   	free_netdev(netdev);
>>   }
>>   
> [Severity: High]
> Can priv->oom_task still be pending when this teardown runs?
>
> The oom work and priv->rx_ring_oom_bm look like they come from the
> earlier patch in this series, "net: hns3: fix interface stuck after
> OOM with full hardware rx queue". This patch reworks both teardown
> paths, though, so it seems worth raising here.
>
> hns3_nic_init_vector_data() sets up the work with
> INIT_DELAYED_WORK(&priv->oom_task, hns3_oom_task). The only
> cancel_delayed_work_sync() is in hns3_nic_net_down(), and
> hns3_nic_uninit_vector_data() has no matching cancel.
>
> hns3_client_uninit() and hns3_reset_notify_uninit_enet() only reach
> hns3_nic_net_down() when HNS3_NIC_STATE_DOWN is clear. If DOWN is
> already set, nothing cancels the work before hns3_uninit_all_ring()
> frees priv->rx_ring_oom_bm and sets it to NULL. Nothing cancels it
> before hns3_client_uninit() calls free_netdev() either, which frees
> priv.
>
> The work can be armed while DOWN is set. One case is the ethtool
> offline selftest on a down interface. hns3_lp_run_test() goes through
> hns3_lb_check_rx_ring() into hns3_clean_rx_ring(), which calls
> hns3_oom_task_schedule() when an rx buffer or skb allocation fails.
> hns3_selftest_restore() then leaves the interface down with the work
> queued.
>
> Another case is hns3_external_lb_prepare(). It sets DOWN with
> test_and_set_bit() without going through hns3_nic_net_down(). If
> hns3_external_lb_restore() bails out because of a concurrent reset or
> a hns3_nic_reset_all_ring() failure, DOWN stays set and the work is
> still pending.
>
> If a reset or driver unload lands inside the delay window, the work
> then runs:
>
> hns3_oom_task()
>      if (test_bit(HNS3_NIC_STATE_DOWN, &priv->state)) {
>          bitmap_zero(priv->rx_ring_oom_bm, h->kinfo.num_tqps);
>          return;
>      }
>
> After reset uninit, that is a NULL pointer dereference in
> bitmap_zero(). After hns3_client_uninit(), the read of priv->state is
> already a use-after-free of the freed netdev private area.
>
> Should the work be cancelled unconditionally, either in
> hns3_nic_uninit_vector_data() or in both uninit paths before
> hns3_uninit_all_ring()? If so, does hns3_oom_task() take any lock
> these paths already hold, such as rtnl or dbg_mutex? That would make a
> synchronous cancel at that point deadlock.

The High issues overlap with the 3/4 review
(rx_ring_oom_bm in set_ringparam, oom_task cancel) and are
addressed there.


pw-bot: cr