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

[netdev-bot+sashiko@kernel.org]

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?

>     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?

> 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?

[ ... ]

> 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?

> @@ -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  ");

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