mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [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®