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

[Jakub Kicinski <kuba@kernel.org>]

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