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

[Mrozowicz, SlawomirX]

>-----Original Message-----
>From: Richardson, Bruce
>Sent: Tuesday, May 3, 2016 4:34 PM
>To: Mrozowicz, SlawomirX <slawomirx.mrozowicz at intel.com>
>Cc: dev at dpdk.org
>Subject: Re: [PATCH] lpm: unchecked return value
>
>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 at 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


Hi Bruce,

In my opinion the worst-case scenario should be take into account. If function like rte_lpm6_add() returns false then it should be handled.

Anyway I agree with you that if the function fail then we have serious problem.
I see two problems:
1. Code construction: calls between function rte_lpm6_add() and rte_lpm6_delete(). As you said it should be examined.
2. How we should handle situation if the rules table are not reconstructed after delete operation.

I propose to add new issue in ClearQuest to proceed solve the problems because there are extend the original issue (CID 13205 Unchecked return value) from Coverity.

Regards,
Sławomir