[PATCH] net: mediatek: fix PPE resource leak on remove

[PATCH] net: mediatek: fix PPE resource leak on remove

[Guangshuo Li]

mtk_ppe_init() initializes the l2_flows rhashtable for each PPE
instance.

The probe failure path releases these resources with
mtk_ppe_deinit(). However, after a successful probe, mtk_remove()
tears down the network devices without calling mtk_ppe_deinit(),
leaving the PPE rhashtable resources allocated.

Call mtk_ppe_deinit() during device removal before freeing the network
devices.

This issue was found by manual code inspection.

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>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 1 +
 1 file changed, 1 insertion(+)

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);
-- 
2.43.0

Re: [PATCH] net: mediatek: fix PPE resource leak on remove

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

> mtk_ppe_init() initializes the l2_flows rhashtable for each PPE
> instance.
> 
> The probe failure path releases these resources with
> mtk_ppe_deinit(). However, after a successful probe, mtk_remove()
> tears down the network devices without calling mtk_ppe_deinit(),
> leaving the PPE rhashtable resources allocated.
> 
> Call mtk_ppe_deinit() during device removal before freeing the network
> devices.
> 
> This issue was found by manual code inspection.
> 
> 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>

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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);
> -- 
> 2.43.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] net: mediatek: fix PPE resource leak on remove

[Wayen Yan]

Hi Guangshuo,

Thanks for the patch. Calling mtk_ppe_deinit() from the remove path is
the right direction, since the probe error path already tears PPE down
but the successful remove path currently does not.

However, I think the cleanup is still incomplete.

mtk_ppe_deinit() currently only destroys the per-PPE l2_flows
rhashtable:

    rhashtable_destroy(&eth->ppe[i]->l2_flows);

This only destroys the hash table itself. It does not free any entries
that may still be stored in it. In this driver, L2 offload entries are
struct mtk_flow_entry objects allocated by mtk_flow_offload_replace()
and inserted into eth->flow_table, and bridge/L2 entries are also
indexed from ppe->l2_flows via mtk_foe_entry_commit_l2().

There is also a lifetime mismatch for eth->flow_table:
mtk_eth_offload_init() initializes it, but I do not see a matching
destroy/free on the remove path.

So I think remove should first drain the offload flow entries, using the
same per-entry teardown as mtk_flow_offload_destroy():

    mtk_foe_entry_clear(...)
    mtk_wed_flow_remove(...) when needed
    kfree(entry)

and then destroy eth->flow_table and the per-PPE l2_flows tables.
rhashtable_free_and_destroy() with a small callback would probably fit
this better than plain rhashtable_destroy().

While touching mtk_ppe_deinit(), it would also be better to use continue
instead of return for missing PPE instances, so later PPE instances are
not skipped.

One more minor cleanup issue: mtk_ppe_debugfs_init() creates the ppe%d
debugfs directory, but the PPE teardown path does not remove it.

So I agree with the fix direction, but I think this should be respun as
a complete PPE/offload cleanup rather than only adding mtk_ppe_deinit()
to mtk_remove().

Thanks,
Wayen

Re: [PATCH] net: mediatek: fix PPE resource leak on remove

[netdev-bot+sashiko]

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

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

- [High] mtk_remove() now calls mtk_ppe_deinit(eth)
  (drivers/net/ethernet/mediatek/mtk_eth_soc.c:5409) *before*…
- [High] The patch closes only half of the leak it claims to fix.

Pre-existing issues:
- [High] mtk_ppe_init() registers per-PPE debugfs entries via
  mtk_ppe_debugfs_init() (mtk_ppe.c:934), which creates a top-level…

--

net: mediatek: fix PPE resource leak on remove

This adds a mtk_ppe_deinit(eth) call to mtk_remove() so that the l2_flows
rhashtables allocated by mtk_ppe_init() are released on device removal, the
way the probe error unwind already does.  A few questions about the placement
and the completeness of the teardown below.

> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index fd7a49ae88d01..903dac36ccf10 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -5406,6 +5406,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);

[Severity: High]
Can this destroy ppe[i]->l2_flows while the netdevs are still registered and
their flow offload callbacks are still live?

mtk_ppe_deinit() only calls rhashtable_destroy(), which frees the bucket
table and leaves ht->tbl dangling, while eth->ppe[i] stays non-NULL:

drivers/net/ethernet/mediatek/mtk_ppe.c:mtk_ppe_deinit() {
	for (i = 0; i < ARRAY_SIZE(eth->ppe); i++) {
		if (!eth->ppe[i])
			return;
		rhashtable_destroy(&eth->ppe[i]->l2_flows);
	}
}

The unregister_netdev() calls only happen afterwards, in mtk_cleanup() ->
mtk_unreg_dev(), and unregistering a device with offloaded flower rules
replays the removals back into the driver:

unregister_netdev() -> dev_shutdown() -> tcf_block_offload_unbind() ->
tcf_block_unbind() -> tcf_block_playback_offloads(..., add=false) ->
fl_reoffload()

net/sched/cls_flower.c:fl_reoffload() {
		cls_flower.command = add ?
			FLOW_CLS_REPLACE : FLOW_CLS_DESTROY;

For an MTK_FLOW_TYPE_L2 (bridge) entry that lands back in the already
destroyed table:

mtk_eth_setup_tc_block_cb() -> mtk_flow_offload_cmd() ->
mtk_flow_offload_destroy() -> mtk_foe_entry_clear() ->

drivers/net/ethernet/mediatek/mtk_ppe.c:__mtk_foe_entry_clear() {
	if (entry->type == MTK_FLOW_TYPE_L2) {
		rhashtable_remove_fast(&ppe->l2_flows, &entry->l2_node,
				       mtk_flow_l2_ht_params);

Is there anything preventing a concurrent FLOW_CLS_REPLACE from reaching
mtk_foe_entry_commit_l2() -> rhashtable_lookup_get_insert_fast() on the same
freed table?  mtk_remove() holds neither rtnl nor mtk_flow_offload_mutex.

The same applies to eth->reset.monitor_work and eth->pending_work, which are
only cancelled later, inside mtk_cleanup():

drivers/net/ethernet/mediatek/mtk_eth_soc.c:mtk_cleanup() {
	mtk_sgmii_destroy(eth);
	mtk_unreg_dev(eth);
	mtk_free_dev(eth);
	cancel_work_sync(&eth->pending_work);
	cancel_delayed_work_sync(&eth->reset.monitor_work);

If those re-run mtk_open()/napi_enable(), the receive path reaches
mtk_ppe_check_skb() -> rhashtable_lookup_fast(&ppe->l2_flows) on the freed
table.

The probe unwind orders this the other way around:

err_unreg_netdev:
	mtk_unreg_dev(eth);
err_deinit_ppe:
	mtk_ppe_deinit(eth);

Would placing mtk_ppe_deinit(eth) after mtk_cleanup(eth) match that order?

[Severity: High]
Does this leave eth->flow_table leaked?

mtk_probe() initializes two rhashtables per PPE index: l2_flows inside
mtk_ppe_init(), and eth->flow_table inside mtk_eth_offload_init():

drivers/net/ethernet/mediatek/mtk_ppe_offload.c:mtk_eth_offload_init() {
	if (!eth->ppe[id] || !eth->ppe[id]->foe_table)
		return 0;
	return rhashtable_init(&eth->flow_table, &mtk_flow_ht_params);
}

The only two rhashtable_destroy() call sites in
drivers/net/ethernet/mediatek/ are both on ppe->l2_flows, in mtk_ppe_init()'s
error path and in mtk_ppe_deinit(), so eth->flow_table's bucket table is
never freed on unbind either before or after this patch.  Should
mtk_ppe_deinit() (or the remove path) also destroy eth->flow_table?

Two related observations on eth->flow_table:

Only rhashtable_free_and_destroy() does cancel_work_sync(&ht->run_work), so a
pending deferred rehash work item stays pointing into struct mtk_eth, which
is devm_kzalloc'ed in mtk_probe() and freed on detach.

eth->flow_table is a single per-device field, but mtk_eth_offload_init() is
called once per PPE index in the mtk_probe() loop, and ppe_num is 2 on
mt7986/mt7981 and 3 on mt7988.  Does re-running rhashtable_init() on the same
object overwrite and leak the previously allocated bucket table already at
probe time?

[Severity: High]
This isn't a bug introduced by this patch, but since mtk_ppe_deinit() is now
the PPE teardown for the remove path: it has no counterpart for the debugfs
entries created during init.

mtk_ppe_init() ends with mtk_ppe_debugfs_init(ppe, index), and that creates a
top level directory plus two files whose private data is the devm_kzalloc'ed
struct mtk_ppe:

drivers/net/ethernet/mediatek/mtk_ppe_debugfs.c:mtk_ppe_debugfs_init() {
	root = debugfs_create_dir(ppe->dirname, NULL);
	debugfs_create_file("entries", S_IRUGO, root, ppe, &mtk_ppe_debugfs_foe_all_fops);
	debugfs_create_file("bind", S_IRUGO, root, ppe, &mtk_ppe_debugfs_foe_bind_fops);

The root dentry is not stored anywhere and there is no debugfs_remove() for
the PPE in the driver (the only debugfs_remove() in
drivers/net/ethernet/mediatek/ is in mtk_wed.c, which does keep
hw->debugfs_dir).  With CONFIG_DEBUG_FS=y, does reading
/sys/kernel/debug/ppe0/entries after unbind dereference the freed struct
mtk_ppe and its freed dmam-allocated foe_table, and does a re-probe collide
with the stale ppe0 directory?  Would adding the debugfs teardown to
mtk_ppe_deinit() be appropriate here?

>  	mtk_cleanup(eth);
>  	free_netdev(eth->dummy_dev);
>  	mtk_mdio_cleanup(eth);

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