* Re: [PATCH net] net: sparx5: free the matchall entry on destroy
2026-10-07 15:03 [PATCH net] net: sparx5: free the matchall entry on destroy Daniel Machon
@ 2026-10-08 18:00 ` patchwork-bot+netdevbpf
2026-10-11 15:19 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2026-10-08 18:00 UTC (permalink / raw)
To: Daniel Machon
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, Steen.Hegelund,
UNGLinuxDriver, netdev, linux-arm-kernel, linux-kernel, kees
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 7 Oct 2026 17:03:15 +0200 you wrote:
> sparx5_tc_matchall_replace() allocates a struct sparx5_mall_entry for
> every offloaded matchall filter and adds it to sparx5->mall_entries.
> sparx5_tc_matchall_destroy() removes the entry from the list, but never
> frees it, so the entry of every deleted mirror and goto matchall filter
> is leaked.
>
> Free the entry after unlinking it.
>
> [...]
Here is the summary with links:
- [net] net: sparx5: free the matchall entry on destroy
https://git.kernel.org/netdev/net/c/2b82e16d6084
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] net: sparx5: free the matchall entry on destroy
2026-10-07 15:03 [PATCH net] net: sparx5: free the matchall entry on destroy Daniel Machon
2026-10-08 18:00 ` patchwork-bot+netdevbpf
@ 2026-10-11 15:19 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-11 15:19 UTC (permalink / raw)
To: daniel.machon
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, Steen.Hegelund,
UNGLinuxDriver, netdev, linux-arm-kernel, linux-kernel, kees
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 1 (1) · Medium: 0 · 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] The fix is incomplete: `struct sparx5_mall_entry` still leaks on
every error return in `sparx5_tc_matchall_replace()`.
Pre-existing issues:
- [High] `sparx5_tc_matchall_parse_mirror_action()` sets
`entry->mirror.port = netdev_priv(action->dev)` without checking that…
--
net: sparx5: free the matchall entry on destroy
This patch adds a kfree() of the struct sparx5_mall_entry in
sparx5_tc_matchall_destroy(), after the entry is removed from
sparx5->mall_entries. Without it, every deleted mirror and goto matchall
filter leaked its entry.
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c
> index 702257979462d..146d7f28e8f3d 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c
> @@ -160,6 +160,7 @@ static int sparx5_tc_matchall_destroy(struct net_device *ndev,
> }
>
> list_del(&entry->list);
> + kfree(entry);
>
> return err;
> }
[Severity: High]
Does sparx5_tc_matchall_replace() still leak mall_entry on its error
paths?
The entry is allocated before the action is checked or the hardware is
programmed. It is only added to the list at the very end:
sparx5_tc_matchall_replace() {
...
mall_entry = kzalloc_obj(*mall_entry);
if (!mall_entry)
return -ENOMEM;
...
list_add_tail(&mall_entry->list, &sparx5->mall_entries);
return 0;
}
Each return between those two points loses the only pointer to the entry
without freeing it:
- the sparx5_mirror_add() failure in the FLOW_ACTION_MIRRED case, which
returns err (-EEXIST, -EINVAL, -ENOENT)
- the four vcap_enable_lookups() failure returns in the FLOW_ACTION_GOTO
case (-EFAULT, -EADDRINUSE, -EADDRNOTAVAIL, and other errors)
- the default case:
default:
NL_SET_ERR_MSG_MOD(tmo->common.extack, "Unsupported action");
return -EOPNOTSUPP;
These entries never reach sparx5->mall_entries. That means
sparx5_tc_matchall_destroy() can never find them, and the new kfree()
does not cover them.
A failed offload is easy to repeat. Examples are a matchall filter with
an action like drop, which hits the default case, a second mirror on a
port that is already mirrored, or a goto to an unsupported chain.
Without skip_sw the software filter is still installed, so this can run
in a loop.
Since this is the same object the commit message says is leaked, could
the fix also free mall_entry on these paths? A shared error label in
sparx5_tc_matchall_replace() would work, or the action could be checked
before the allocation.
[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Is
the mirred target device checked before it is used as a sparx5 port?
sparx5_tc_matchall_parse_mirror_action() {
entry->mirror.port = netdev_priv(action->dev);
}
Nothing checks that action->dev is a sparx5 port, or even that it is
non-NULL. The driver already has a helper that checks both:
sparx5_netdevice_check() {
return dev && (dev->netdev_ops == &sparx5_port_netdev_ops);
}
The lan966x driver does a similar check in lan966x_mirror_port_add().
If the mirred target is a foreign netdev such as dummy, veth or a
bridge, sparx5_mirror_add() reads mport->portno from that driver's
private area. It then writes the value to hardware:
sparx5_mirror_add() {
...
if (sport->portno == mport->portno)
return -EINVAL;
...
sparx5_mirror_monitor_set(sparx5, mirror_idx, mport->portno);
...
}
As a result, mirrored traffic could be sent to an arbitrary switch port.
action->dev can also be NULL. In net/sched/act_mirred.c,
tcf_offload_mirred_get_dev() leaves entry->dev NULL when the action has
no device:
tcf_offload_mirred_get_dev() {
entry->dev = act->ops->get_dev(act, &entry->destructor);
if (!entry->dev)
return;
...
}
This happens with mirred blockid actions, where tcf_mirred_init() calls
tcf_mirred_replace_dev(m, NULL). It also happens when the target device
has been unregistered. In both cases entry->id is still set to
FLOW_ACTION_MIRRED.
netdev_priv(NULL) then returns a small non-NULL pointer, and the
mport->portno read in sparx5_mirror_add() would oops.
Would calling sparx5_netdevice_check() on action->dev before it is
used fix both cases?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-sparx5-matchall-kfree-net-v1-1-c8918685b337%40microchip.com
^ permalink raw reply [flat|nested] 3+ messages in thread