Re: [PATCH] mlxsw: spectrum_flower: Fix port range register leak

[Petr Machata]

Ido Schimmel <idosch@xxxxxxxxxx> writes:

> On Fri, Sep 18, 2026 at 06:39:32PM +0200, Petr Machata wrote:
>> Wentao Liang <vulab@xxxxxxxxxxx> 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,