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

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

[Wentao Liang]

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

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

[Petr Machata]

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;