[PATCH net-next v2] net: microchip: sparx5: clean up PSFP resources on flower setup failure

[PATCH net-next v2] net: microchip: sparx5: clean up PSFP resources on flower setup failure

[Haoxiang Li]

Add error handling paths to unwind the PSFP resources that have already
been allocated before returning the error.

Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
---
Changes in v2:
 - Rebase on netdev/net-next.
 - Add a Reviewed-by tag.
---
 .../microchip/sparx5/sparx5_tc_flower.c        | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
index f779a5c00803..a17143953502 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
@@ -807,7 +807,7 @@ static int sparx5_tc_flower_psfp_setup(struct sparx5 *sparx5,
 		/* Add new flow-meter */
 		ret = sparx5_psfp_fm_add(sparx5, pol_idx, fm, &psfp_fmid);
 		if (ret < 0)
-			return ret;
+			goto err_sg_del;
 	}
 
 	/* Map stream filter to stream gate */
@@ -816,7 +816,7 @@ static int sparx5_tc_flower_psfp_setup(struct sparx5 *sparx5,
 	/* Add new stream-filter and map it to a steam gate */
 	ret = sparx5_psfp_sf_add(sparx5, sf, &psfp_sfid);
 	if (ret < 0)
-		return ret;
+		goto err_fm_del;
 
 	/* Streams are classified by ISDX - map ISDX 1:1 to sfid for now. */
 	sparx5_isdx_conf_set(sparx5, psfp_sfid, psfp_sfid, psfp_fmid);
@@ -824,13 +824,23 @@ static int sparx5_tc_flower_psfp_setup(struct sparx5 *sparx5,
 	ret = vcap_rule_add_action_bit(vrule, VCAP_AF_ISDX_ADD_REPLACE_SEL,
 				       VCAP_BIT_1);
 	if (ret)
-		return ret;
+		goto err_sf_del;
 
 	ret = vcap_rule_add_action_u32(vrule, VCAP_AF_ISDX_VAL, psfp_sfid);
 	if (ret)
-		return ret;
+		goto err_sf_del;
 
 	return 0;
+
+err_sf_del:
+	sparx5_isdx_conf_set(sparx5, psfp_sfid, 0, 0);
+	sparx5_psfp_sf_del(sparx5, psfp_sfid);
+err_fm_del:
+	if (pol_idx >= 0)
+		sparx5_psfp_fm_del(sparx5, psfp_fmid);
+err_sg_del:
+	sparx5_psfp_sg_del(sparx5, psfp_sgid);
+	return ret;
 }
 
 /* Handle the action trap for a VCAP rule */
-- 
2.25.1

Re: [PATCH net-next v2] net: microchip: sparx5: clean up PSFP resources on flower setup failure

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: microchip: sparx5: clean up PSFP resources on flower setup failure

This patch adds error handling paths to sparx5_tc_flower_psfp_setup() so
that PSFP resources allocated earlier in the function are unwound when a
later step fails.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> index f779a5c00803..a17143953502 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c
> @@ -807,7 +807,7 @@ static int sparx5_tc_flower_psfp_setup(struct sparx5 *sparx5,
>  		/* Add new flow-meter */
>  		ret = sparx5_psfp_fm_add(sparx5, pol_idx, fm, &psfp_fmid);
>  		if (ret < 0)
> -			return ret;
> +			goto err_sg_del;
>  	}
>  
>  	/* Map stream filter to stream gate */
> @@ -816,7 +816,7 @@ static int sparx5_tc_flower_psfp_setup(struct sparx5 *sparx5,
>  	/* Add new stream-filter and map it to a steam gate */
>  	ret = sparx5_psfp_sf_add(sparx5, sf, &psfp_sfid);
>  	if (ret < 0)
> -		return ret;
> +		goto err_fm_del;
>  
>  	/* Streams are classified by ISDX - map ISDX 1:1 to sfid for now. */
>  	sparx5_isdx_conf_set(sparx5, psfp_sfid, psfp_sfid, psfp_fmid);
> @@ -824,13 +824,23 @@ static int sparx5_tc_flower_psfp_setup(struct sparx5 *sparx5,
>  	ret = vcap_rule_add_action_bit(vrule, VCAP_AF_ISDX_ADD_REPLACE_SEL,
>  				       VCAP_BIT_1);
>  	if (ret)
> -		return ret;
> +		goto err_sf_del;
>  
>  	ret = vcap_rule_add_action_u32(vrule, VCAP_AF_ISDX_VAL, psfp_sfid);
>  	if (ret)
> -		return ret;
> +		goto err_sf_del;
>  
>  	return 0;
> +
> +err_sf_del:
> +	sparx5_isdx_conf_set(sparx5, psfp_sfid, 0, 0);
> +	sparx5_psfp_sf_del(sparx5, psfp_sfid);
> +err_fm_del:
> +	if (pol_idx >= 0)
> +		sparx5_psfp_fm_del(sparx5, psfp_fmid);

[High]
Can the err_fm_del path leak a flow-meter refcount when the flow meter
was already in use?

Looking at sparx5_psfp_fm_add():

	ret = sparx5_psfp_fm_get(sparx5, uidx, &fm->pol.idx);
	if (ret < 0)
		return ret;
	/* Was already in use, no need to reconfigure */
	if (ret > 1)
		return 0;
	...
	*id = fm->pol.idx;
	return 0;

When the flow meter is already in use, sparx5_psfp_fm_add() returns 0
without writing back to *id, so the caller's psfp_fmid is left at its
initialized value of 0.

If sparx5_psfp_sf_add() or one of the vcap_rule_add_action_* calls then
fails, err_fm_del runs sparx5_psfp_fm_del(sparx5, 0). Pool ids appear to
be 1-based:

	u32 sparx5_pool_idx_to_id(u32 idx)
	{
		return ++idx;
	}

so id 0 has no valid LBSET, sparx5_sdlb_group_get_by_index() returns
-EINVAL, and sparx5_psfp_fm_del() aborts before reaching its
sparx5_psfp_fm_put() call:

	ret = sparx5_sdlb_group_get_by_index(sparx5, id, &fm.pol.group);
	if (ret < 0)
		return ret;

	ret = sparx5_psfp_fm_put(sparx5, id);

The refcount that sparx5_psfp_fm_get() incremented on the actually-reused
flow meter would not be decremented.

Would it work to either move the *id = fm->pol.idx; assignment before the
ret > 1 early return inside sparx5_psfp_fm_add(), or to call
sparx5_psfp_fm_del(sparx5, fm->pol.idx) at this cleanup site instead?

> +err_sg_del:
> +	sparx5_psfp_sg_del(sparx5, psfp_sgid);
> +	return ret;
>  }
>  
>  /* Handle the action trap for a VCAP rule */
-- 
pw-bot: cr