* [PATCH] mlxsw: spectrum_flower: Fix port range register leak
@ 2026-09-17 11:32 Wentao Liang
2026-09-18 16:39 ` Petr Machata
2026-09-21 12:52 ` netdev-bot+sashiko
0 siblings, 2 replies; 5+ messages in thread
From: Wentao Liang @ 2026-09-17 11:32 UTC (permalink / raw)
To: andrew+netdev
Cc: davem, edumazet, idosch, kuba, linux-kernel, netdev, pabeni,
petrm, Wentao Liang, stable
mlxsw_sp_flower_parse_ports_range() acquires the source port range
register before the destination one. If the destination lookup then
fails, the source register reference is left behind in a partially
filled rule info, and callers that pass a stack allocated rule info
never release it. Release the source register before returning the
error so that the reference is not leaked.
Fixes: fe22f7410527 ("mlxsw: spectrum_flower: Add ability to match on port ranges")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
index 353fd9ca89a6..4cad6c46b07e 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c
@@ -486,8 +486,14 @@ mlxsw_sp_flower_parse_ports_range(struct mlxsw_sp *mlxsw_sp,
err = mlxsw_sp_port_range_reg_get(mlxsw_sp, &range,
f->common.extack, &prr_index);
- if (err)
+ if (err) {
+ if (rulei->src_port_range_reg_valid) {
+ mlxsw_sp_port_range_reg_put(mlxsw_sp,
+ rulei->src_port_range_reg_index);
+ rulei->src_port_range_reg_valid = false;
+ }
return err;
+ }
rulei->dst_port_range_reg_index = prr_index;
rulei->dst_port_range_reg_valid = true;
--
2.34.1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] mlxsw: spectrum_flower: Fix port range register leak 2026-09-17 11:32 [PATCH] mlxsw: spectrum_flower: Fix port range register leak Wentao Liang @ 2026-09-18 16:39 ` Petr Machata 2026-09-20 6:19 ` Ido Schimmel 2026-09-21 12:52 ` netdev-bot+sashiko 1 sibling, 1 reply; 5+ messages in thread From: Petr Machata @ 2026-09-18 16:39 UTC (permalink / raw) To: Wentao Liang Cc: andrew+netdev, davem, edumazet, idosch, kuba, linux-kernel, netdev, pabeni, petrm, stable Wentao Liang <vulab@iscas.ac.cn> writes: > mlxsw_sp_flower_parse_ports_range() acquires the source port range > register before the destination one. If the destination lookup then > fails, the source register reference is left behind in a partially > filled rule info, and callers that pass a stack allocated rule info > never release it. Release the source register before returning the > error so that the reference is not leaked. I think this is fixing the wrong issue in fact. This does fix something, namely the issue of trying to add a new tc chain filter with both src_port and dst_port ranges in a situation where only one resource is left. Before the fix, we end up with full resource allocation even as the offload fails, because the register for the src_port range is not released. After the fix, the src_port register is correctly released. But look: # tc qdisc add dev swp1 ingress # tc chain add dev swp1 ingress chain 90 protocol ip flower ip_proto udp src_port 100-9000 dst_port 200-9000 # devlink -j resource show pci/0000:06:00.0 | jq '.resources.[][] | select(.name == "port_range_registers") | .occ' 2 # tc chain del dev swp1 ingress chain 90 # devlink -j resource show pci/0000:06:00.0 | jq '.resources.[][] | select(.name == "port_range_registers") | .occ' 2 So it's much more broken than just this cleanup path edge case. (Notably, 'tc filter' cleans up properly, it is really just 'tc template' that triggers it.) I.e. let's not have this. I think this is the fix that we need, and it fixes the cleanup path issue as well. modified drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c @@ -862,14 +862,24 @@ int mlxsw_sp_flower_tmplt_create(struct mlxsw_sp *mlxsw_sp, memset(&rulei, 0, sizeof(rulei)); err = mlxsw_sp_flower_parse(mlxsw_sp, block, &rulei, f); if (err) - return err; + goto out; + ruleset = mlxsw_sp_acl_ruleset_get(mlxsw_sp, block, f->common.chain_index, MLXSW_SP_ACL_PROFILE_FLOWER, &rulei.values.elusage); /* keep the reference to the ruleset */ - return PTR_ERR_OR_ZERO(ruleset); + err = PTR_ERR_OR_ZERO(ruleset); + +out: + if (rulei.src_port_range_reg_valid) + mlxsw_sp_port_range_reg_put(mlxsw_sp, + rulei.src_port_range_reg_index); + if (rulei.dst_port_range_reg_valid) + mlxsw_sp_port_range_reg_put(mlxsw_sp, + rulei.dst_port_range_reg_index); + return err; } void mlxsw_sp_flower_tmplt_destroy(struct mlxsw_sp *mlxsw_sp, The template parser just needs to figure out which keys are used (the rulei.values.elusage), it doesn't need to allocate any registers, but it neglects to make the appropriate cleanups. I'll test the above fix some more and send it sometime next week. > Fixes: fe22f7410527 ("mlxsw: spectrum_flower: Add ability to match on port ranges") > Cc: stable@vger.kernel.org > Signed-off-by: Wentao Liang <vulab@iscas.ac.cn> > --- > drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > index 353fd9ca89a6..4cad6c46b07e 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > @@ -486,8 +486,14 @@ mlxsw_sp_flower_parse_ports_range(struct mlxsw_sp *mlxsw_sp, > > err = mlxsw_sp_port_range_reg_get(mlxsw_sp, &range, > f->common.extack, &prr_index); > - if (err) > + if (err) { > + if (rulei->src_port_range_reg_valid) { > + mlxsw_sp_port_range_reg_put(mlxsw_sp, > + rulei->src_port_range_reg_index); > + rulei->src_port_range_reg_valid = false; > + } > return err; > + } > > rulei->dst_port_range_reg_index = prr_index; > rulei->dst_port_range_reg_valid = true; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mlxsw: spectrum_flower: Fix port range register leak 2026-09-18 16:39 ` Petr Machata @ 2026-09-20 6:19 ` Ido Schimmel 2026-09-21 7:49 ` Petr Machata 0 siblings, 1 reply; 5+ messages in thread From: Ido Schimmel @ 2026-09-20 6:19 UTC (permalink / raw) To: Petr Machata Cc: Wentao Liang, andrew+netdev, davem, edumazet, kuba, linux-kernel, netdev, pabeni, stable On Fri, Sep 18, 2026 at 06:39:32PM +0200, Petr Machata wrote: > Wentao Liang <vulab@iscas.ac.cn> writes: > > > mlxsw_sp_flower_parse_ports_range() acquires the source port range > > register before the destination one. If the destination lookup then > > fails, the source register reference is left behind in a partially > > filled rule info, and callers that pass a stack allocated rule info > > never release it. Release the source register before returning the > > error so that the reference is not leaked. > > I think this is fixing the wrong issue in fact. > > This does fix something, namely the issue of trying to add a new tc > chain filter with both src_port and dst_port ranges in a situation where > only one resource is left. Before the fix, we end up with full resource > allocation even as the offload fails, because the register for the > src_port range is not released. After the fix, the src_port register is > correctly released. > > But look: > > # tc qdisc add dev swp1 ingress > # tc chain add dev swp1 ingress chain 90 protocol ip flower ip_proto udp src_port 100-9000 dst_port 200-9000 > # devlink -j resource show pci/0000:06:00.0 | jq '.resources.[][] | select(.name == "port_range_registers") | .occ' > 2 > # tc chain del dev swp1 ingress chain 90 > # devlink -j resource show pci/0000:06:00.0 | jq '.resources.[][] | select(.name == "port_range_registers") | .occ' > 2 > > So it's much more broken than just this cleanup path edge case. > (Notably, 'tc filter' cleans up properly, it is really just 'tc > template' that triggers it.) > > I.e. let's not have this. > > > I think this is the fix that we need, and it fixes the cleanup path > issue as well. > > modified drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > @@ -862,14 +862,24 @@ int mlxsw_sp_flower_tmplt_create(struct mlxsw_sp *mlxsw_sp, > memset(&rulei, 0, sizeof(rulei)); > err = mlxsw_sp_flower_parse(mlxsw_sp, block, &rulei, f); > if (err) > - return err; > + goto out; > + > ruleset = mlxsw_sp_acl_ruleset_get(mlxsw_sp, block, > f->common.chain_index, > MLXSW_SP_ACL_PROFILE_FLOWER, > &rulei.values.elusage); > > /* keep the reference to the ruleset */ > - return PTR_ERR_OR_ZERO(ruleset); > + err = PTR_ERR_OR_ZERO(ruleset); > + > +out: > + if (rulei.src_port_range_reg_valid) > + mlxsw_sp_port_range_reg_put(mlxsw_sp, > + rulei.src_port_range_reg_index); > + if (rulei.dst_port_range_reg_valid) > + mlxsw_sp_port_range_reg_put(mlxsw_sp, > + rulei.dst_port_range_reg_index); > + return err; > } > > void mlxsw_sp_flower_tmplt_destroy(struct mlxsw_sp *mlxsw_sp, > > The template parser just needs to figure out which keys are used (the > rulei.values.elusage), it doesn't need to allocate any registers, but it > neglects to make the appropriate cleanups. > > I'll test the above fix some more and send it sometime next week. The above diff still makes it likely that we will miss similar cleanup in the future. It's better if both cleanup paths call the same function. Something like: diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h index b03ff9e044f9..48c199bb9e25 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h @@ -989,6 +989,8 @@ void mlxsw_sp_acl_ruleset_prio_get(struct mlxsw_sp_acl_ruleset *ruleset, struct mlxsw_sp_acl_rule_info * mlxsw_sp_acl_rulei_create(struct mlxsw_sp_acl *acl, struct mlxsw_afa_block *afa_block); +void mlxsw_sp_acl_rulei_fini(struct mlxsw_sp *mlxsw_sp, + struct mlxsw_sp_acl_rule_info *rulei); void mlxsw_sp_acl_rulei_destroy(struct mlxsw_sp *mlxsw_sp, struct mlxsw_sp_acl_rule_info *rulei); int mlxsw_sp_acl_rulei_commit(struct mlxsw_sp_acl_rule_info *rulei); diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c index cb232accb296..af287b18deee 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c @@ -340,8 +340,8 @@ mlxsw_sp_acl_rulei_create(struct mlxsw_sp_acl *acl, return ERR_PTR(err); } -void mlxsw_sp_acl_rulei_destroy(struct mlxsw_sp *mlxsw_sp, - struct mlxsw_sp_acl_rule_info *rulei) +void mlxsw_sp_acl_rulei_fini(struct mlxsw_sp *mlxsw_sp, + struct mlxsw_sp_acl_rule_info *rulei) { if (rulei->action_created) mlxsw_afa_block_destroy(rulei->act_block); @@ -351,6 +351,12 @@ void mlxsw_sp_acl_rulei_destroy(struct mlxsw_sp *mlxsw_sp, if (rulei->dst_port_range_reg_valid) mlxsw_sp_port_range_reg_put(mlxsw_sp, rulei->dst_port_range_reg_index); +} + +void mlxsw_sp_acl_rulei_destroy(struct mlxsw_sp *mlxsw_sp, + struct mlxsw_sp_acl_rule_info *rulei) +{ + mlxsw_sp_acl_rulei_fini(mlxsw_sp, rulei); kfree(rulei); } diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c index 353fd9ca89a6..3532c4bc7e56 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c @@ -862,14 +862,17 @@ int mlxsw_sp_flower_tmplt_create(struct mlxsw_sp *mlxsw_sp, memset(&rulei, 0, sizeof(rulei)); err = mlxsw_sp_flower_parse(mlxsw_sp, block, &rulei, f); if (err) - return err; + goto out; ruleset = mlxsw_sp_acl_ruleset_get(mlxsw_sp, block, f->common.chain_index, MLXSW_SP_ACL_PROFILE_FLOWER, &rulei.values.elusage); + err = PTR_ERR_OR_ZERO(ruleset); /* keep the reference to the ruleset */ - return PTR_ERR_OR_ZERO(ruleset); +out: + mlxsw_sp_acl_rulei_fini(mlxsw_sp, &rulei); + return err; } void mlxsw_sp_flower_tmplt_destroy(struct mlxsw_sp *mlxsw_sp, ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mlxsw: spectrum_flower: Fix port range register leak 2026-09-20 6:19 ` Ido Schimmel @ 2026-09-21 7:49 ` Petr Machata 0 siblings, 0 replies; 5+ messages in thread From: Petr Machata @ 2026-09-21 7:49 UTC (permalink / raw) To: Ido Schimmel Cc: Petr Machata, Wentao Liang, andrew+netdev, davem, edumazet, kuba, linux-kernel, netdev, pabeni, stable Ido Schimmel <idosch@nvidia.com> writes: > On Fri, Sep 18, 2026 at 06:39:32PM +0200, Petr Machata wrote: >> Wentao Liang <vulab@iscas.ac.cn> writes: >> >> > mlxsw_sp_flower_parse_ports_range() acquires the source port range >> > register before the destination one. If the destination lookup then >> > fails, the source register reference is left behind in a partially >> > filled rule info, and callers that pass a stack allocated rule info >> > never release it. Release the source register before returning the >> > error so that the reference is not leaked. >> >> I think this is fixing the wrong issue in fact. >> >> This does fix something, namely the issue of trying to add a new tc >> chain filter with both src_port and dst_port ranges in a situation where >> only one resource is left. Before the fix, we end up with full resource >> allocation even as the offload fails, because the register for the >> src_port range is not released. After the fix, the src_port register is >> correctly released. >> >> But look: >> >> # tc qdisc add dev swp1 ingress >> # tc chain add dev swp1 ingress chain 90 protocol ip flower ip_proto udp src_port 100-9000 dst_port 200-9000 >> # devlink -j resource show pci/0000:06:00.0 | jq '.resources.[][] | select(.name == "port_range_registers") | .occ' >> 2 >> # tc chain del dev swp1 ingress chain 90 >> # devlink -j resource show pci/0000:06:00.0 | jq '.resources.[][] | select(.name == "port_range_registers") | .occ' >> 2 >> >> So it's much more broken than just this cleanup path edge case. >> (Notably, 'tc filter' cleans up properly, it is really just 'tc >> template' that triggers it.) >> >> I.e. let's not have this. >> >> >> I think this is the fix that we need, and it fixes the cleanup path >> issue as well. >> >> modified drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c >> @@ -862,14 +862,24 @@ int mlxsw_sp_flower_tmplt_create(struct mlxsw_sp *mlxsw_sp, >> memset(&rulei, 0, sizeof(rulei)); >> err = mlxsw_sp_flower_parse(mlxsw_sp, block, &rulei, f); >> if (err) >> - return err; >> + goto out; >> + >> ruleset = mlxsw_sp_acl_ruleset_get(mlxsw_sp, block, >> f->common.chain_index, >> MLXSW_SP_ACL_PROFILE_FLOWER, >> &rulei.values.elusage); >> >> /* keep the reference to the ruleset */ >> - return PTR_ERR_OR_ZERO(ruleset); >> + err = PTR_ERR_OR_ZERO(ruleset); >> + >> +out: >> + if (rulei.src_port_range_reg_valid) >> + mlxsw_sp_port_range_reg_put(mlxsw_sp, >> + rulei.src_port_range_reg_index); >> + if (rulei.dst_port_range_reg_valid) >> + mlxsw_sp_port_range_reg_put(mlxsw_sp, >> + rulei.dst_port_range_reg_index); >> + return err; >> } >> >> void mlxsw_sp_flower_tmplt_destroy(struct mlxsw_sp *mlxsw_sp, >> >> The template parser just needs to figure out which keys are used (the >> rulei.values.elusage), it doesn't need to allocate any registers, but it >> neglects to make the appropriate cleanups. >> >> I'll test the above fix some more and send it sometime next week. > > The above diff still makes it likely that we will miss similar cleanup in the > future. It's better if both cleanup paths call the same function. Something > like: The fix I posted follows the current `tc filter` cleanup approach in that the cleanup is outsourced to the caller. What you posted is obviously cleaner and not as messy of a patch as I was afraid, what with the new function, so I'll go with it. > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > index b03ff9e044f9..48c199bb9e25 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h > @@ -989,6 +989,8 @@ void mlxsw_sp_acl_ruleset_prio_get(struct mlxsw_sp_acl_ruleset *ruleset, > struct mlxsw_sp_acl_rule_info * > mlxsw_sp_acl_rulei_create(struct mlxsw_sp_acl *acl, > struct mlxsw_afa_block *afa_block); > +void mlxsw_sp_acl_rulei_fini(struct mlxsw_sp *mlxsw_sp, > + struct mlxsw_sp_acl_rule_info *rulei); > void mlxsw_sp_acl_rulei_destroy(struct mlxsw_sp *mlxsw_sp, > struct mlxsw_sp_acl_rule_info *rulei); > int mlxsw_sp_acl_rulei_commit(struct mlxsw_sp_acl_rule_info *rulei); > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c > index cb232accb296..af287b18deee 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c > @@ -340,8 +340,8 @@ mlxsw_sp_acl_rulei_create(struct mlxsw_sp_acl *acl, > return ERR_PTR(err); > } > > -void mlxsw_sp_acl_rulei_destroy(struct mlxsw_sp *mlxsw_sp, > - struct mlxsw_sp_acl_rule_info *rulei) > +void mlxsw_sp_acl_rulei_fini(struct mlxsw_sp *mlxsw_sp, > + struct mlxsw_sp_acl_rule_info *rulei) > { > if (rulei->action_created) > mlxsw_afa_block_destroy(rulei->act_block); > @@ -351,6 +351,12 @@ void mlxsw_sp_acl_rulei_destroy(struct mlxsw_sp *mlxsw_sp, > if (rulei->dst_port_range_reg_valid) > mlxsw_sp_port_range_reg_put(mlxsw_sp, > rulei->dst_port_range_reg_index); > +} > + > +void mlxsw_sp_acl_rulei_destroy(struct mlxsw_sp *mlxsw_sp, > + struct mlxsw_sp_acl_rule_info *rulei) > +{ > + mlxsw_sp_acl_rulei_fini(mlxsw_sp, rulei); > kfree(rulei); > } > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > index 353fd9ca89a6..3532c4bc7e56 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > @@ -862,14 +862,17 @@ int mlxsw_sp_flower_tmplt_create(struct mlxsw_sp *mlxsw_sp, > memset(&rulei, 0, sizeof(rulei)); > err = mlxsw_sp_flower_parse(mlxsw_sp, block, &rulei, f); > if (err) > - return err; > + goto out; > ruleset = mlxsw_sp_acl_ruleset_get(mlxsw_sp, block, > f->common.chain_index, > MLXSW_SP_ACL_PROFILE_FLOWER, > &rulei.values.elusage); > + err = PTR_ERR_OR_ZERO(ruleset); > > /* keep the reference to the ruleset */ > - return PTR_ERR_OR_ZERO(ruleset); > +out: > + mlxsw_sp_acl_rulei_fini(mlxsw_sp, &rulei); > + return err; > } > > void mlxsw_sp_flower_tmplt_destroy(struct mlxsw_sp *mlxsw_sp, ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mlxsw: spectrum_flower: Fix port range register leak 2026-09-17 11:32 [PATCH] mlxsw: spectrum_flower: Fix port range register leak Wentao Liang 2026-09-18 16:39 ` Petr Machata @ 2026-09-21 12:52 ` netdev-bot+sashiko 1 sibling, 0 replies; 5+ messages in thread From: netdev-bot+sashiko @ 2026-09-21 12:52 UTC (permalink / raw) To: vulab Cc: andrew+netdev, davem, edumazet, idosch, kuba, linux-kernel, netdev, pabeni, petrm, stable Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider. Critical: 0 · High: 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] Incomplete fix: the port-range register leak the commit message claims to close remains fully reachable. -- mlxsw: spectrum_flower: Fix port range register leak Adds an unwind to mlxsw_sp_flower_parse_ports_range() so that the source port range register is released when acquiring the destination port range register fails. The commit message motivates this by callers that pass a stack allocated rule info and never release it. > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > index 353fd9ca89a68..4cad6c46b07e9 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > @@ -486,8 +486,14 @@ mlxsw_sp_flower_parse_ports_range(struct mlxsw_sp *mlxsw_sp, > > err = mlxsw_sp_port_range_reg_get(mlxsw_sp, &range, > f->common.extack, &prr_index); > - if (err) > + if (err) { > + if (rulei->src_port_range_reg_valid) { > + mlxsw_sp_port_range_reg_put(mlxsw_sp, > + rulei->src_port_range_reg_index); > + rulei->src_port_range_reg_valid = false; > + } > return err; > + } > > rulei->dst_port_range_reg_index = prr_index; > rulei->dst_port_range_reg_valid = true; [Severity: High] Does this hunk actually close the leak the changelog describes? mlxsw_sp_flower_parse() has two callers. The first is mlxsw_sp_flower_replace(), which uses the heap rulei owned by the rule and routes every error through mlxsw_sp_acl_rule_destroy() -> mlxsw_sp_acl_rulei_destroy(), which already drops both registers: drivers/net/ethernet/mellanox/mlxsw/spectrum_acl.c:mlxsw_sp_acl_rulei_destroy() { if (rulei->src_port_range_reg_valid) mlxsw_sp_port_range_reg_put(mlxsw_sp, rulei->src_port_range_reg_index); if (rulei->dst_port_range_reg_valid) mlxsw_sp_port_range_reg_put(mlxsw_sp, rulei->dst_port_range_reg_index); kfree(rulei); } So on the replace path the new put looks redundant. The second caller is the stack rulei one named in the changelog, mlxsw_sp_flower_tmplt_create(): struct mlxsw_sp_acl_rule_info rulei; ... memset(&rulei, 0, sizeof(rulei)); err = mlxsw_sp_flower_parse(mlxsw_sp, block, &rulei, f); if (err) return err; ruleset = mlxsw_sp_acl_ruleset_get(mlxsw_sp, block, f->common.chain_index, MLXSW_SP_ACL_PROFILE_FLOWER, &rulei.values.elusage); /* keep the reference to the ruleset */ return PTR_ERR_OR_ZERO(ruleset); This never calls mlxsw_sp_acl_rulei_destroy() (it cannot, that function ends in kfree(rulei)), and mlxsw_sp_flower_tmplt_destroy() only drops ruleset references. Isn't every port range register recorded in that stack rulei still leaked? The new unwind only covers the narrow window "source acquired, destination acquisition failed". For the template caller the reference is still dropped on the floor in these cases: - the success path, where one or two registers stay pinned per template (this is the common case) - later failures inside mlxsw_sp_flower_parse() after mlxsw_sp_flower_parse_ports_range() already succeeded, namely mlxsw_sp_flower_parse_tcp() rejecting reserved flags, mlxsw_sp_flower_parse_ip() rejecting a non-IP n_proto, and mlxsw_sp_flower_parse_actions() failing: err = mlxsw_sp_flower_parse_ports_range(mlxsw_sp, rulei, f, ip_proto); if (err) return err; err = mlxsw_sp_flower_parse_tcp(mlxsw_sp, rulei, f, ip_proto); if (err) return err; err = mlxsw_sp_flower_parse_ip(mlxsw_sp, rulei, f, n_proto_key & n_proto_mask); if (err) return err; return mlxsw_sp_flower_parse_actions(mlxsw_sp, block, rulei, &f->rule->action, f->common.extack); - mlxsw_sp_acl_ruleset_get() returning an error in mlxsw_sp_flower_tmplt_create() Is the template key really non-zero here? fl_tmplt_create() in net/sched/cls_flower.c fills tmplt->dummy_key and tmplt->mask from the user attributes via fl_set_key(), fl_set_key_port_range() sets mask->tp_range.tp_min.src/dst to all ones for TCP/UDP/SCTP, and fl_hw_create_tmplt() hands that dissector, key and mask to the driver. FLOW_DISSECTOR_KEY_PORTS_RANGE is in the accepted key mask in mlxsw_sp_flower_parse(), so mlxsw_sp_port_range_reg_get() takes a real reference for a template such as: tc chain add dev swp1 ingress protocol ip \ flower ip_proto tcp src_port 100-200 The pool is small and fixed. mlxsw_sp_port_range_init() sets pr_core->prr_ids.max from MLXSW_CORE_RES_GET(core, ACL_MAX_L4_PORT_RANGE) and warns above 16 entries: max = MLXSW_CORE_RES_GET(core, ACL_MAX_L4_PORT_RANGE); /* Each port range register is represented using a single bit in the * two bytes "l4_port_range" ACL key element. */ WARN_ON(max > BITS_PER_BYTE * sizeof(u16)); Once it is exhausted, mlxsw_sp_port_range_reg_create() fails: err = xa_alloc(&pr_core->prr_xa, &prr->index, prr, pr_core->prr_ids, GFP_KERNEL); if (err) { if (err == -EBUSY) NL_SET_ERR_MSG_MOD(extack, "Exceeded number of port range registers"); Can repeated template add/delete with distinct port ranges therefore pin all registers for the lifetime of the device, keep devlink prr_count inflated, and make mlxsw_sp_port_range_fini()'s WARN_ON(!xa_empty(&pr_core->prr_xa)) fire on teardown? Would it be better to drop this hunk and instead release the rulei's port range registers in mlxsw_sp_flower_tmplt_create(), for example through a non-freeing rulei cleanup helper that mirrors mlxsw_sp_acl_rulei_destroy(), or to skip register acquisition altogether when parsing a template? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917113236.2149095-1-vulab%40iscas.ac.cn ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-09-21 12:52 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-09-17 11:32 [PATCH] mlxsw: spectrum_flower: Fix port range register leak Wentao Liang 2026-09-18 16:39 ` Petr Machata 2026-09-20 6:19 ` Ido Schimmel 2026-09-21 7:49 ` Petr Machata 2026-09-21 12:52 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®