Re: [PATCH v2 1/3] net/ice: fix possible memory leak

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Jul 15, 2024 at 06:04:39PM +0000, Vladimir Medvedkin wrote:
> This patch fixes possible memory leak inside the
> ice_hash_parse_raw_pattern() due to the lack of a call to rte_free()
> for previously allocated pkt_buf and msk_buf.
> 
> Fixes: 1b9c68120a1c ("net/ice: enable protocol agnostic flow offloading in RSS")
> Cc: stable@dpdk.org
> 
> Reported-by: Michael Theodore Stolarchuk <mike.stolarchuk@arista.com>
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>

Hi,

comments line below.

/Bruce

> ---
>  drivers/net/ice/ice_hash.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ice/ice_hash.c b/drivers/net/ice/ice_hash.c
> index f923641533..913f54fca4 100644
> --- a/drivers/net/ice/ice_hash.c
> +++ b/drivers/net/ice/ice_hash.c
> @@ -650,7 +650,7 @@ ice_hash_parse_raw_pattern(struct ice_adapter *ad,
>  	uint8_t *pkt_buf, *msk_buf;
>  	uint8_t tmp_val = 0;
>  	uint8_t tmp_c = 0;
> -	int i, j;
> +	int i, j, ret = 0;
>  
>  	if (ad->psr == NULL)
>  		return -rte_errno;
> @@ -670,8 +670,10 @@ ice_hash_parse_raw_pattern(struct ice_adapter *ad,
>  		return -ENOMEM;
>  
>  	msk_buf = rte_zmalloc(NULL, pkt_len, 0);
> -	if (!msk_buf)
> +	if (!msk_buf) {
> +		rte_free(pkt_buf);
>  		return -ENOMEM;
> +	}
>  
>  	/* convert string to int array */
>  	for (i = 0, j = 0; i < spec_len; i += 2, j++) {
> @@ -708,17 +710,20 @@ ice_hash_parse_raw_pattern(struct ice_adapter *ad,
>  			msk_buf[j] = tmp_val * 16 + tmp_c - '0';
>  	}
>  
> -	if (ice_parser_run(ad->psr, pkt_buf, pkt_len, &rslt))
> -		return -rte_errno;
> +	ret = ice_parser_run(ad->psr, pkt_buf, pkt_len, &rslt);
> +	if (ret)
> +		goto free_mem;

We are losing the error code retrn value here. I think the final return in
this function should be "return ret" rather than "return 0" so we can
propagate up the error.

>  
> -	if (ice_parser_profile_init(&rslt, pkt_buf, msk_buf,
> -		pkt_len, ICE_BLK_RSS, true, &prof))
> -		return -rte_errno;
> +	ret = ice_parser_profile_init(&rslt, pkt_buf, msk_buf,
> +			pkt_len, ICE_BLK_RSS, true, &prof);
> +		goto free_mem;

Same here. Also the "if" statement before the "goto" is missing.

>  
>  	rte_memcpy(&meta->raw.prof, &prof, sizeof(prof));
>  
> +free_mem:
>  	rte_free(pkt_buf);
>  	rte_free(msk_buf);
> +
>  	return 0;
>  }
>  
> -- 
> 2.34.1
>