Re: [dpdk-dev] [PATCH] lpm: unchecked return value

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

On Wed, Apr 27, 2016 at 02:52:34PM +0200, Slawomir Mrozowicz wrote:
> Fix issue reported by Coverity.
> 
> Coverity ID 13205: Unchecked return value
> Unchecked return value
> check_return: Calling rte_lpm6_add without checking return value
> Fixes: 5c510e13a9cb ("lpm: add IPv6 support")
> 
> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz@intel.com>
> ---
>  lib/librte_lpm/rte_lpm6.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
> index ba4353c..f4db3fa 100644
> --- a/lib/librte_lpm/rte_lpm6.c
> +++ b/lib/librte_lpm/rte_lpm6.c
> @@ -749,6 +749,7 @@ rte_lpm6_delete(struct rte_lpm6 *lpm, uint8_t *ip, uint8_t depth)
>  	int32_t rule_to_delete_index;
>  	uint8_t ip_masked[RTE_LPM6_IPV6_ADDR_SIZE];
>  	unsigned i;
> +	int status = 0;
>  
>  	/*
>  	 * Check input arguments.
> @@ -790,12 +791,13 @@ rte_lpm6_delete(struct rte_lpm6 *lpm, uint8_t *ip, uint8_t depth)
>  	 * Add every rule again (except for the one that was removed from
>  	 * the rules table).
>  	 */
> -	for (i = 0; i < lpm->used_rules; i++) {
> -		rte_lpm6_add(lpm, lpm->rules_tbl[i].ip, lpm->rules_tbl[i].depth,
> -				lpm->rules_tbl[i].next_hop);
> +	for (i = 0; i < lpm->used_rules && status >= 0; i++) {
> +		status = rte_lpm6_add(
> +			lpm, lpm->rules_tbl[i].ip, lpm->rules_tbl[i].depth,
> +			lpm->rules_tbl[i].next_hop);
>  	}
>  
> -	return 0;
> +	return status;
>  }

Hi,

I'm not sure that this patch is actually necessary, as I'm not sure that the
lpm6_add calls can fail in this instance. Looking through the code, this function
deletes the rule and then clears the actual lpm lookup tables before re-adding
all other routes to it again. The only error condition that could be returned,
that I can see, is -ENOSPC, which should never occur here since the original
rules fitted in the first place.

If it was possible to fail, then I think we would have a worse problem, in that
deleting a single rule has wiped out our lpm table and left it in an inconsistent
state, so the error handling probably needs to be better than just quitting.

Finally, one other thing I spot looking through the code, is that there seems
to be a worrying set of calls between add and delete. If the add function fails,
then it calls delete which in turn will call add again, etc. etc. This may all
work correctly, but it seems fragile and error prone to me - especially if we allow
calls from one to another to fail.

This looks like it might need some further examination to verify what the possible
failure cases are and what happens in each scenario.

Regards,
/Bruce