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

[Jijie Shao <shaojijie@huawei.com>]

on 2026/9/19 23:37, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider:
>
> Critical: 0 · High: 1 · Medium: 3 · Low: 1
>
> - [High] On the PF side the new locking is inoperative.
> - [Medium] The commit message justifies the change with "The seq_file
>    refactoring of hns3 debugfs removed the per-file entry state checks"…
> - [Medium] The new device-busy gate is added to only one of the two
>    helpers that produce the output of the single `vlan_config` debugfs…
> - [Medium] The reader/writer handshake the patch relies on has no
>    publication ordering on the reset-reinitialization side.
> - [Low] The Fixes: tag cites only eced3d1c41db ("net: hns3: use seq_file
>    for files in queue/ in debugfs"), which is the correct origin for the…
>
> --
>
> net: hns3: fix use-after-free in debugfs read during reset/unload
>
> This adds a new mutex (dbg_mutex) to struct hnae3_handle, has the debugfs
> show callbacks take it plus a device-busy state check at entry and per
> loop iteration, and has the enet teardown/ring-resize paths take it before
> freeing rings and vectors. Two hclge (PF layer) debugfs callbacks also
> take the same mutex through hdev->vport[0].nic.
>
> Closes: https://lore.kernel.org/all/a0853cd9-cab5-441d-b181-8ba97f2f58b0@huawei.com/
>
> A few questions on the commit message and the code below.
>
>>      The seq_file refactoring of hns3 debugfs removed the per-file entry
>>      state checks. Without them, debugfs show callbacks can access
>>      priv->ring, priv->tqp_vector, and ring->desc while these resources
>>      are being freed by reset, driver unload, or ethtool ring resize.
> [Severity: Medium]
> Does this description cover the hclge_debugfs.c part of the patch?  At the
> baseline, grepping hclge_debugfs.c for HCLGE_STATE_RST_HANDLING or any
> test_bit(HCLGE_STATE...) returns no matches, so there were no per-file
> state checks there to be removed by the seq_file conversion.  The two
> guarded PF callbacks also do not touch priv->ring, priv->tqp_vector or
> ring->desc; they read hdev->* and hdev->vport[] and issue firmware
> commands, and hdev->vport is only freed in hclge_uninit_ae_dev().
>
> The selection also looks arbitrary.  Of the roughly fifty
> hclge_dbg_dump_* seq_file callbacks registered in hclge_dbg_cmd_func[],
> only hclge_dbg_dump_umv_info() and hclge_dbg_dump_vlan_offload_config()
> gained the new guard, while structurally identical ones that read
> hdev-owned state during reset (hclge_dbg_dump_tm_*, hclge_dbg_dump_fd_*,
> hclge_dbg_dump_mng_table, hclge_dbg_dump_mac_uc/mc, the register dumps)
> were left alone.
>
> If the PF layer needs this protection, should the rest of the callbacks
> get it too?  If it does not, could the hclge_debugfs.c hunks be dropped?

Right — the commit message omits the hclge_debugfs.c changes

Will be fixed in v2. The hclge_debugfs.c hunks will be removed entirely
(see High #1).

>
>>      Fixes: eced3d1c41db ("net: hns3: use seq_file for files in queue/ in debugfs")
> [Severity: Low]
> This tag matches the hns3_debugfs.c (enet) change, but the seq_file
> conversion of hclge_dbg_dump_umv_info() blames to 9e1545b48818e0
> ("net: hns3: use seq_file for debugfs in umv_info and vlan_config"-era
> sibling commit by Yonglong Liu), not to eced3d1c41db.  Should a second
> Fixes: tag be added so the hclge_debugfs.c hunks can be mapped to the
> commit that introduced them?

Resolved by removing the hclge_debugfs.c changes in v2. Only the one Fixes:
(eced3d1c41db) remains.

>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
>> index 4286af9239b01..e94cc33da8643 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
>> @@ -944,6 +944,7 @@ struct hnae3_handle {
>>   
>>   	u8 netdev_flags;
>>   	struct dentry *hnae3_dbgfs;
>> +	struct mutex dbg_mutex;	/* protect debugfs against reset/unload */
>>   
>>   	/* Network interface message level enabled bits */
>>   	u32 msg_enable;
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
>> index 1347edac76993..7d913302f4424 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_debugfs.c
>> @@ -389,6 +389,12 @@ static const char * const dim_state_str[] = { "START", "IN_PROG", "APPLY" };
>>   static const char * const
>>   dim_tune_stat_str[] = { "ON_TOP", "TIRED", "RIGHT", "LEFT" };
>>   
>> +static bool hns3_dbg_is_device_busy(struct hns3_nic_priv *priv)
>> +{
>> +	return !test_bit(HNS3_NIC_STATE_INITED, &priv->state) ||
>> +	       test_bit(HNS3_NIC_STATE_RESETTING, &priv->state);
>> +}
>> +
> [ ... ]
>
>> @@ -504,22 +524,16 @@ 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;
>> -	}
>> +	guard(mutex)(&priv->ae_handle->dbg_mutex);
>> +	if (hns3_dbg_is_device_busy(priv))
>> +		return -EBUSY;
> [ ... ]
>
>> @@ -604,9 +612,14 @@ static int hns3_dbg_queue_map(struct seq_file *s, void *data)
>>   
>>   	seq_puts(s, "local_queue_id  global_queue_id  vector_id\n");
>>   
>> +	guard(mutex)(&priv->ae_handle->dbg_mutex);
>> +	if (hns3_dbg_is_device_busy(priv))
>> +		return -EBUSY;
>> +
>>   	for (i = 0; i < h->kinfo.num_tqps; i++) {
>> -		if (!priv->ring || !priv->ring[i].tqp_vector)
>> -			continue;
>> +		if (hns3_dbg_is_device_busy(priv))
>> +			return -EBUSY;
>> +
>>   		seq_printf(s, "%-16u%-17u%d\n", i,
>>   			   h->ae_algo->ops->get_global_queue_id(h, i),
>>   			   priv->ring[i].tqp_vector->vector_irq);
> [Severity: Medium]
> Is the reader/writer handshake ordered on the reinitialization side?
>
> The teardown side does what the changelog describes:
>
> hns3_reset_notify_uninit_enet()
> 	test_and_clear_bit(HNS3_NIC_STATE_INITED, &priv->state);
> 	guard(mutex)(&handle->dbg_mutex);
> 	...
> 	hns3_put_ring_config(priv);	/* priv->ring = NULL */
>
> But the reinit side never takes dbg_mutex and publishes with a plain
> bitop:
>
> hns3_reset_notify_init_enet()
> 	... rebuilds priv->ring, priv->tqp_vector, descriptors ...
> 	set_bit(HNS3_NIC_STATE_INITED, &priv->state);
>
> hns3_reset_notify_up_enet()
> 	clear_bit(HNS3_NIC_STATE_RESETTING, &priv->state);
>
> Non-returning bitops carry no memory barriers
> (Documentation/atomic_bitops.txt), and since the initializer never holds
> dbg_mutex, the reader's mutex acquisition pairs only with the teardown
> release, which stored priv->ring = NULL.  On a weakly ordered machine can
> a reader observe INITED set and RESETTING clear while still reading the
> stale priv->ring, and then dereference priv->ring[i].tqp_vector here?
>
> The "!priv->ring || !priv->ring[i].tqp_vector" test that used to absorb
> that case is being deleted in this hunk, as are the "if (!priv->ring)"
> guards in hns3_dbg_rx_queue_info(), hns3_dbg_tx_queue_info() and
> hns3_dbg_page_pool_info().  Would either keeping those NULL checks or
> having hns3_reset_notify_init_enet() publish under dbg_mutex (or with
> release/acquire semantics) close this?

All priv->ring writes and NULL assignments happen under dbg_mutex. The
three paths setting priv->ring = NULL (hns3_put_ring_config in uninit and
reset) hold the mutex. After the mutex + busy check, priv->ring cannot be
NULL — the check is redundant.

No change in v2. The if (!priv->ring) checks will be removed.

>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> index 0c088feae03ca..bae8b32ffc5b8 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> [ ... ]
>
>> @@ -5875,6 +5880,7 @@ static int hns3_reset_notify_uninit_enet(struct hnae3_handle *handle)
>>   		return 0;
>>   	}
>>   
>> +	guard(mutex)(&handle->dbg_mutex);
>>   	hns3_free_rx_cpu_rmap(netdev);
>>   	hns3_nic_uninit_irq(priv);
>>   	hns3_clear_all_ring(handle, true);
>> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
>> index 9a4e29bfa1666..1e9d3e08b7ded 100644
>> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
>> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_debugfs.c
>> @@ -15,6 +15,12 @@
>>   #define hclge_seq_file_to_hdev(s)	\
>>   		(((struct hnae3_ae_dev *)hnae3_seq_file_to_ae_dev(s))->priv)
>>   
>> +static bool hclge_dbg_is_device_busy(struct hclge_dev *hdev)
>> +{
>> +	return test_bit(HCLGE_STATE_RST_HANDLING, &hdev->state) ||
>> +	       test_bit(HCLGE_STATE_RST_FAIL, &hdev->state);
>> +}
>> +
>>   static const char * const hclge_mac_state_str[] = {
>>   	"TO_ADD", "TO_DEL", "ACTIVE"
>>   };
>> @@ -2621,6 +2627,10 @@ static int hclge_dbg_dump_umv_info(struct seq_file *s, void *data)
>>   	struct hclge_vport *vport;
>>   	u8 i;
>>   
>> +	guard(mutex)(&hdev->vport[0].nic.dbg_mutex);
>> +	if (hclge_dbg_is_device_busy(hdev))
>> +		return -EBUSY;
>> +
> [Severity: High]
> Does this mutex exclude anything on the PF side?
>
> Grepping the driver for dbg_mutex after this patch gives:
>
>    hnae3.h:1
>    hns3_debugfs.c:6
>    hns3_enet.c:6
>    hns3_ethtool.c:1
>    hns3pf/hclge_debugfs.c:2
>
> hclge_main.c has no matches, so no writer of the state these two
> callbacks read ever takes dbg_mutex:
>
> hclge_reset_service_task()
> 	down(&hdev->reset_sem);
> 	set_bit(HCLGE_STATE_RST_HANDLING, &hdev->state);
> 	hclge_reset_subtask(hdev);
> 	clear_bit(HCLGE_STATE_RST_HANDLING, &hdev->state);
>
> and hclge_reset_err_handle() sets HCLGE_STATE_RST_FAIL the same way.  The
> only reset-side acquisition is hns3_reset_notify_uninit_enet(), which
> covers priv->ring and priv->tqp_vector, not the UMV counters or the
> per-function VLAN offload registers read here.
>
> So isn't hclge_dbg_is_device_busy() here exactly the check-then-use
> window the changelog says the mutex removes?  A reader admitted before
> RST_HANDLING is set keeps issuing cmdq queries across the reset boundary
> and prints values sampled from both sides of it.
>
> Two follow-on questions:
>
> Does the comment on the new struct member, "protect debugfs against
> reset/unload", hold for hclge state given the above?
>
> Does taking dbg_mutex and then hdev->vport_lock here create a new lock
> order that the writer side is not aware of?

You are correct. The dbg_mutex protects enet-layer resources (priv->ring,
priv->tqp_vector), not the hdev->* fields and firmware commands accessed by
the PF callbacks. The mutex in hclge_debugfs.c adds false protection.

Will be fixed in v2. All hclge_debugfs.c changes will be removed — the
patch will focus exclusively on the enet-layer UAF.

The lock ordering concern (dbg_mutex → vport_lock) also goes away with this.

>
>> @@ -2831,6 +2841,10 @@ static int hclge_dbg_dump_vlan_offload_config(struct hclge_dev *hdev,
>>   	int ret;
>>   	u8 i;
>>   
>> +	guard(mutex)(&hdev->vport[0].nic.dbg_mutex);
>> +	if (hclge_dbg_is_device_busy(hdev))
>> +		return -EBUSY;
>> +
> [Severity: Medium]
> Should the gate be in the show callback instead of in this helper?
>
> Both halves of the single vlan_config file run from one callback:
>
> hclge_dbg_dump_vlan_config()
> 	ret = hclge_dbg_dump_vlan_filter_config(hdev, s);
> 	if (ret)
> 		return ret;
>
> 	return hclge_dbg_dump_vlan_offload_config(hdev, s);
>
> hclge_dbg_dump_vlan_filter_config() issues
> hclge_get_vlan_filter_state() and
> hclge_get_port_vlan_filter_bypass_state() for every function and emits
> its section with no mutex and no state check, so reading vlan_config
> during a reset still runs that half against the resetting device and only
> then returns -EBUSY from the second half.  Since seq_read_iter() resets
> m->count to the record offset on a negative show() return, the output the
> first half produced is discarded and the read fails anyway.
>
> Also, unlike the enet readers, this loop holds dbg_mutex across
> 2 * (pci_num_vf() + 1) synchronous firmware commands with no
> per-iteration busy re-check.  Given that the reset path reaches
> guard(mutex)(&handle->dbg_mutex) via
>
>    hclge_reset_rebuild() -> rtnl_lock() -> hclge_reset_stack() ->
>    hclge_notify_client(HNAE3_UNINIT_CLIENT) ->
>    hns3_reset_notify_uninit_enet()
>
> can a PF debugfs reader stall the reset, and rtnl_lock with it, for the
> duration of the dump?  The changelog says the per-iteration check keeps
> reset latency bounded, which does not appear to apply to these two PF
> callbacks.
>
>>   	seq_puts(s, "FUNC_ID  PVID  ACCEPT_TAG1  ACCEPT_TAG2 ACCEPT_UNTAG1  ");
>>   	seq_puts(s, "ACCEPT_UNTAG2  INSERT_TAG1  INSERT_TAG2  SHIFT_TAG  ");
>>   	seq_puts(s, "STRIP_TAG1  STRIP_TAG2  DROP_TAG1  DROP_TAG2  ");