The PPE teardown is incomplete after both probe failure and normal
driver removal.

mtk_ppe_init() initializes a per-PPE l2_flows rhashtable and creates
debugfs entries, while mtk_eth_offload_init() initializes the shared
eth->flow_table. Flow offload entries allocated by
mtk_flow_offload_replace() can remain in these tables for the lifetime
of the device.

mtk_ppe_deinit() currently only destroys the l2_flows tables. It does
not release entries in eth->flow_table or destroy the flow table, and
the normal remove path does not call mtk_ppe_deinit() at all. PPE
debugfs entries are also left behind.

Drain eth->flow_table with rhashtable_free_and_destroy(), clearing each
PPE flow entry, dropping the WED flow reference when necessary, and
freeing the flow entry. Remove the PPE debugfs directories and destroy
the per-PPE l2_flows tables afterwards.

Track initialization of the shared flow table so partial probe cleanup
is safe and the table is initialized only once when multiple PPE
instances are present. Also continue past missing PPE instances during
cleanup so later instances are not skipped.

Call mtk_ppe_deinit() from the remove path to perform the complete
PPE/offload teardown.

Fixes: 33fc42de3327 ("net: ethernet: mtk_eth_soc: support creating mac address based offload entries")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
v2:
  - Drain and destroy the shared offload flow table during PPE teardown.
  - Remove per-entry PPE and WED state before freeing flow entries.
  - Remove PPE debugfs entries during teardown.
  - Continue past missing PPE instances instead of returning early.
  - Track the shared flow table initialization state for safe cleanup.
  - Call the complete PPE teardown from the remove path.

 drivers/net/ethernet/mediatek/mtk_eth_soc.c   |  1 +
 drivers/net/ethernet/mediatek/mtk_eth_soc.h   |  2 +
 drivers/net/ethernet/mediatek/mtk_ppe.c       | 13 ++++++-
 .../net/ethernet/mediatek/mtk_ppe_offload.c   | 37 ++++++++++++++++++-
 4 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index be3bd025c41a..04d0a1eec4cf 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -5394,6 +5394,7 @@ static void mtk_remove(struct platform_device *pdev)
 
 	netif_napi_del(&eth->tx_napi);
 	netif_napi_del(&eth->rx_napi);
+	mtk_ppe_deinit(eth);
 	mtk_cleanup(eth);
 	free_netdev(eth->dummy_dev);
 	mtk_mdio_cleanup(eth);
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
index 0168e2fbc619..c1ca8af6f356 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
@@ -1344,6 +1344,7 @@ struct mtk_eth {
 
 	struct mtk_ppe			*ppe[3];
 	struct rhashtable		flow_table;
+	bool				flow_table_initialized;
 
 	struct bpf_prog			__rcu *prog;
 
@@ -1508,6 +1509,7 @@ int mtk_gmac_gephy_path_setup(struct mtk_eth *eth, int mac_id);
 int mtk_gmac_rgmii_path_setup(struct mtk_eth *eth, int mac_id);
 
 int mtk_eth_offload_init(struct mtk_eth *eth, u8 id);
+void mtk_eth_offload_deinit(struct mtk_eth *eth);
 int mtk_eth_setup_tc(struct net_device *dev, enum tc_setup_type type,
 		     void *type_data);
 int mtk_flow_offload_cmd(struct mtk_eth *eth, struct flow_cls_offload *cls,
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.c b/drivers/net/ethernet/mediatek/mtk_ppe.c
index 8451dc3fd00a..f41df3cf0e2b 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (C) 2020 Felix Fietkau <nbd@nbd.name> */
 
+#include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
@@ -946,7 +947,17 @@ void mtk_ppe_deinit(struct mtk_eth *eth)
 
 	for (i = 0; i < ARRAY_SIZE(eth->ppe); i++) {
 		if (!eth->ppe[i])
-			return;
+			continue;
+
+		debugfs_lookup_and_remove(eth->ppe[i]->dirname, NULL);
+	}
+
+	mtk_eth_offload_deinit(eth);
+
+	for (i = 0; i < ARRAY_SIZE(eth->ppe); i++) {
+		if (!eth->ppe[i])
+			continue;
+
 		rhashtable_destroy(&eth->ppe[i]->l2_flows);
 	}
 }
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
index 99b28aaa7cc4..5b14c7b3052f 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
@@ -678,9 +678,42 @@ int mtk_eth_setup_tc(struct net_device *dev, enum tc_setup_type type,
 	}
 }
 
+static void mtk_flow_offload_free(void *ptr, void *arg)
+{
+	struct mtk_flow_entry *entry = ptr;
+	struct mtk_eth *eth = arg;
+
+	mtk_foe_entry_clear(eth->ppe[entry->ppe_index], entry);
+
+	if (entry->wed_index >= 0)
+		mtk_wed_flow_remove(entry->wed_index);
+
+	kfree(entry);
+}
+
 int mtk_eth_offload_init(struct mtk_eth *eth, u8 id)
 {
-	if (!eth->ppe[id] || !eth->ppe[id]->foe_table)
+	int err;
+
+	if (!eth->ppe[id] || !eth->ppe[id]->foe_table ||
+	    eth->flow_table_initialized)
 		return 0;
-	return rhashtable_init(&eth->flow_table, &mtk_flow_ht_params);
+	err = rhashtable_init(&eth->flow_table, &mtk_flow_ht_params);
+	if (!err)
+		eth->flow_table_initialized = true;
+
+	return err;
+}
+
+void mtk_eth_offload_deinit(struct mtk_eth *eth)
+{
+	if (!eth->flow_table_initialized)
+		return;
+
+	mutex_lock(&mtk_flow_offload_mutex);
+	rhashtable_free_and_destroy(&eth->flow_table, mtk_flow_offload_free,
+				    eth);
+	mutex_unlock(&mtk_flow_offload_mutex);
+
+	eth->flow_table_initialized = false;
 }
-- 
2.43.0

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 1 (1) · Low: 0

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] Teardown ordering bug: mtk_remove() calls the newly added
  mtk_ppe_deinit(eth) *after* mtk_hw_deinit(eth), which has already…
- [High] Use-after-free: mtk_remove() destroys the shared offload
  rhashtable (mtk_ppe_deinit() -> mtk_eth_offload_deinit() ->…
- [Medium] The WED reference drop added in mtk_flow_offload_free()
  (mtk_wed_flow_remove(entry->wed_index)) is dead code on the remove…

Pre-existing issues:
- [Medium] Pre-existing WED flow-reference leak in
  mtk_flow_offload_replace(): after 'if (wed_index >= 0 && (err =…

--

net: mediatek: fix PPE resource leak on remove

This patch drains eth->flow_table with rhashtable_free_and_destroy()
during PPE teardown, removes the PPE debugfs directories, tracks whether
the shared flow table was initialized, and calls mtk_ppe_deinit() from
mtk_remove().

A few questions about the new teardown ordering below.

> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 2ea5dfe85539b..0625be1b112f7 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -5410,6 +5410,7 @@ static void mtk_remove(struct platform_device *pdev)
>  
>  	netif_napi_del(&eth->tx_napi);
>  	netif_napi_del(&eth->rx_napi);
> +	mtk_ppe_deinit(eth);
>  	mtk_cleanup(eth);
>  	free_netdev(eth->dummy_dev);
>  	mtk_mdio_cleanup(eth);

[Severity: High]
Can this new call site touch PPE registers after the clocks are gone?
In mtk_remove(), mtk_hw_deinit() has already run before this point:

drivers/net/ethernet/mediatek/mtk_eth_soc.c:mtk_hw_deinit() {
	if (!test_and_clear_bit(MTK_HW_INIT, &eth->state))
		return 0;

	mtk_clk_disable(eth);

	pm_runtime_put_sync(eth->dev);
	pm_runtime_disable(eth->dev);
	...
}

The new drain path reaches MMIO for every entry that still has a
hardware slot:

mtk_ppe_deinit() -> mtk_eth_offload_deinit() ->
rhashtable_free_and_destroy(&eth->flow_table, mtk_flow_offload_free) ->
mtk_foe_entry_clear() -> __mtk_foe_entry_clear()

drivers/net/ethernet/mediatek/mtk_ppe.c:__mtk_foe_entry_clear() {
	if (entry->hash != 0xffff) {
		struct mtk_foe_entry *hwe = mtk_foe_get_entry(ppe, entry->hash);

		hwe->ib1 &= ~MTK_FOE_IB1_STATE;
		hwe->ib1 |= FIELD_PREP(MTK_FOE_IB1_STATE, MTK_FOE_STATE_INVALID);
		dma_wmb();
		mtk_ppe_cache_clear(ppe);
	...
}

drivers/net/ethernet/mediatek/mtk_ppe.c:mtk_ppe_cache_clear() {
	ppe_set(ppe, MTK_PPE_CACHE_CTL, MTK_PPE_CACHE_CTL_CLEAR);
	ppe_clear(ppe, MTK_PPE_CACHE_CTL, MTK_PPE_CACHE_CTL_CLEAR);
}

mtk_ppe_stop() does not reset entry->hash, so the hash != 0xffff branch
is reachable during unbind when offloaded flows are present.

The probe unwind uses the opposite order, with mtk_ppe_deinit() running
before mtk_hw_deinit() and mtk_wed_exit():

err_deinit_ppe:
	mtk_ppe_deinit(eth);
	mtk_mdio_cleanup(eth);
err_free_dev:
	mtk_free_dev(eth);
err_deinit_hw:
	mtk_hw_deinit(eth);
err_wed_exit:
	mtk_wed_exit();

Should the remove path place mtk_ppe_deinit() before mtk_wed_exit() and
mtk_hw_deinit() to match?

> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index 88a9b3b23bea5..2b33067345c39 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -1346,6 +1346,7 @@ struct mtk_eth {
>  
>  	struct mtk_ppe			*ppe[3];
>  	struct rhashtable		flow_table;
> +	bool				flow_table_initialized;
>  
>  	struct bpf_prog			__rcu *prog;
>  

[ ... ]

> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.c b/drivers/net/ethernet/mediatek/mtk_ppe.c
> index 8451dc3fd00aa..f41df3cf0e2b8 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe.c

[ ... ]

> @@ -946,7 +947,17 @@ void mtk_ppe_deinit(struct mtk_eth *eth)
>  
>  	for (i = 0; i < ARRAY_SIZE(eth->ppe); i++) {
>  		if (!eth->ppe[i])
> -			return;
> +			continue;
> +
> +		debugfs_lookup_and_remove(eth->ppe[i]->dirname, NULL);
> +	}
> +
> +	mtk_eth_offload_deinit(eth);
> +
> +	for (i = 0; i < ARRAY_SIZE(eth->ppe); i++) {
> +		if (!eth->ppe[i])
> +			continue;
> +
>  		rhashtable_destroy(&eth->ppe[i]->l2_flows);
>  	}
>  }

> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> index 99b28aaa7cc47..5b14c7b3052f9 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe_offload.c
> @@ -678,9 +678,42 @@ int mtk_eth_setup_tc(struct net_device *dev, enum tc_setup_type type,
>  	}
>  }
>  
> +static void mtk_flow_offload_free(void *ptr, void *arg)
> +{
> +	struct mtk_flow_entry *entry = ptr;
> +	struct mtk_eth *eth = arg;
> +
> +	mtk_foe_entry_clear(eth->ppe[entry->ppe_index], entry);
> +
> +	if (entry->wed_index >= 0)
> +		mtk_wed_flow_remove(entry->wed_index);

[Severity: Medium]
Does this mtk_wed_flow_remove() actually drop anything on the remove
path?  mtk_remove() runs mtk_wed_exit() before the new
mtk_ppe_deinit(), and mtk_wed_exit() tears down the WED registry:

drivers/net/ethernet/mediatek/mtk_wed.c:mtk_wed_exit() {
	...
		hw_list[i] = NULL;
		debugfs_remove(hw->debugfs_dir);
		put_device(hw->dev);
		of_node_put(hw->node);
		kfree(hw);
	...
}

so by the time the drain runs, mtk_wed_flow_remove() takes the early
bail-out:

drivers/net/ethernet/mediatek/mtk_wed.c:mtk_wed_flow_remove() {
	struct mtk_wed_hw *hw = hw_list[index];

	mutex_lock(&hw_lock);

	if (!hw || !hw->wed_dev)
		goto out;
	...
}

hw->num_flows is never decremented, wlan.offload_disable() is never
called and mtk_wed_set_ext_int() is never re-run, so the enable done by
mtk_wed_flow_add() is left unpaired.  Is the commit message claim of
"dropping the WED flow reference when necessary" reachable with this
ordering?

[Severity: Medium]
This isn't a bug introduced by this patch, but while on the subject of
WED flow references: mtk_flow_offload_replace() acquires one before the
entry allocation and the allocation failure path returns without
releasing it:

drivers/net/ethernet/mediatek/mtk_ppe_offload.c:mtk_flow_offload_replace() {
	if (wed_index >= 0 && (err = mtk_wed_flow_add(wed_index)) < 0)
		return err;

	entry = kzalloc_obj(*entry);
	if (!entry)
		return -ENOMEM;
	...
}

mtk_wed_flow_add() has already incremented hw->num_flows and possibly
called wlan.offload_enable(), and since no entry is inserted into
eth->flow_table the new drain cannot balance it either.  Should this
return go through the goto-based unwind further down the function, which
does call mtk_wed_flow_remove()?

> +
> +	kfree(entry);
> +}
> +
>  int mtk_eth_offload_init(struct mtk_eth *eth, u8 id)
>  {
> -	if (!eth->ppe[id] || !eth->ppe[id]->foe_table)
> +	int err;
> +
> +	if (!eth->ppe[id] || !eth->ppe[id]->foe_table ||
> +	    eth->flow_table_initialized)
>  		return 0;
> -	return rhashtable_init(&eth->flow_table, &mtk_flow_ht_params);
> +	err = rhashtable_init(&eth->flow_table, &mtk_flow_ht_params);
> +	if (!err)
> +		eth->flow_table_initialized = true;
> +
> +	return err;
> +}
> +
> +void mtk_eth_offload_deinit(struct mtk_eth *eth)
> +{
> +	if (!eth->flow_table_initialized)
> +		return;
> +
> +	mutex_lock(&mtk_flow_offload_mutex);
> +	rhashtable_free_and_destroy(&eth->flow_table, mtk_flow_offload_free,
> +				    eth);
> +	mutex_unlock(&mtk_flow_offload_mutex);
> +
> +	eth->flow_table_initialized = false;
>  }

[Severity: High]
Can this destroy the flow table while the offload block callbacks are
still bound to live netdevs?

In mtk_remove() the new mtk_ppe_deinit() runs before mtk_cleanup():

	mtk_ppe_deinit(eth);
	mtk_cleanup(eth);

and the netdevs are only unregistered inside mtk_cleanup():

drivers/net/ethernet/mediatek/mtk_eth_soc.c:mtk_cleanup() {
	mtk_sgmii_destroy(eth);
	mtk_unreg_dev(eth);
	...
}

unregister_netdev() unbinds the ingress block and flushes the flowtable,
which replays FLOW_CLS_DESTROY back into the driver through
mtk_eth_setup_tc_block_cb() -> mtk_flow_offload_cmd() ->
mtk_flow_offload_destroy(), where the freed table is dereferenced:

	entry = rhashtable_lookup(&eth->flow_table, &f->cookie,
				  mtk_flow_ht_params);

rhashtable_free_and_destroy() kvfree()s the bucket tables and does not
clear ht->tbl, so eth->flow_table.tbl is left dangling.

mtk_flow_offload_cmd() only takes mtk_flow_offload_mutex and never
consults the new flag:

	mutex_lock(&mtk_flow_offload_mutex);
	switch (cls->command) {
	case FLOW_CLS_REPLACE:
		err = mtk_flow_offload_replace(eth, cls, ppe_index);

Since mtk_remove() holds no RTNL, a concurrent tc filter add ...
skip_sw or an nf_flow_offload work item can also reach
mtk_flow_offload_replace() -> rhashtable_insert_fast() on the freed
table.

Should the table be destroyed only after all netdevs are unregistered,
and should mtk_flow_offload_cmd() check eth->flow_table_initialized
under mtk_flow_offload_mutex?  Note the flag is also cleared outside
the mutex here.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921145421.443018-1-lgs201920130244%40gmail.com