[dpdk-dev] lpm: unchecked return value

  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(-)
  

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
  

>-----Original Message-----
>From: Richardson, Bruce
>Sent: Tuesday, May 3, 2016 4:34 PM
>To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>
>Cc: dev@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@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
  

Hi,

I handle Coverity defect ID 13201. It is about unchecked return value from rte_lpm6_delete() instances in rte_lpm6_add() function.
Next I found this thread and I see that both defects (ID 13205 and ID 13201) may be resolved all together.

> >> 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.

I agree that -ENOSPC should never occur here. So rte_lpm6_add() instance should never fail here.

Next I looked at rte_lpm6_add() and if rte_lpm6_delete() instances in it may fail?
The only suspicious place that I found is place when add every rule again but that should work as discussed above.

> >
> >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.

I see no failure scenarios in here. I mean I see no possibility to create test that show that add function fail in del and opposite.
The only scenario what I have in my mind is that someone call add or/and del functions on different threads with the same lpm table instance, but this is not allowed, cause we know that this functions are not thread safe.

> >
> >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

I propose to classify this Coverity issues (ID 13205 and ID 13201) as Intentional.

Regards,
Piotr
  

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Azarewicz, PiotrX T
> Sent: Thursday, May 12, 2016 1:20 PM
> To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] lpm: unchecked return value
> 
> Hi,
> 
> I handle Coverity defect ID 13201. It is about unchecked return value from
> rte_lpm6_delete() instances in rte_lpm6_add() function.
> Next I found this thread and I see that both defects (ID 13205 and ID 13201)
> may be resolved all together.
> 
> > >> 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.
> 
> I agree that -ENOSPC should never occur here. So rte_lpm6_add() instance
> should never fail here.
> 
> Next I looked at rte_lpm6_add() and if rte_lpm6_delete() instances in it
> may fail?
> The only suspicious place that I found is place when add every rule again
> but that should work as discussed above.
> 
> > >
> > >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.
> 
> I see no failure scenarios in here. I mean I see no possibility to create test
> that show that add function fail in del and opposite.
> The only scenario what I have in my mind is that someone call add or/and
> del functions on different threads with the same lpm table instance, but
> this is not allowed, cause we know that this functions are not thread safe.
> 
> > >
> > >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
> 
> I propose to classify this Coverity issues (ID 13205 and ID 13201) as
> Intentional.
> 
> Regards,
> Piotr

Hi Bruce,
We would like to move forward with theses Coverity defects thus
Please share your opinion about classifying these defects as Intentional?

Michal
  

On Thu, Jun 23, 2016 at 09:13:22AM +0100, Jastrzebski, MichalX K wrote:
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Azarewicz, PiotrX T
> > Sent: Thursday, May 12, 2016 1:20 PM
> > To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>; Richardson,
> > Bruce <bruce.richardson@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] lpm: unchecked return value
> > 
> > Hi,
> > 
> > I handle Coverity defect ID 13201. It is about unchecked return value from
> > rte_lpm6_delete() instances in rte_lpm6_add() function.
> > Next I found this thread and I see that both defects (ID 13205 and ID 13201)
> > may be resolved all together.
> > 
> > > >> 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.
> > 
> > I agree that -ENOSPC should never occur here. So rte_lpm6_add() instance
> > should never fail here.
> > 
> > Next I looked at rte_lpm6_add() and if rte_lpm6_delete() instances in it
> > may fail?
> > The only suspicious place that I found is place when add every rule again
> > but that should work as discussed above.
> > 
> > > >
> > > >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.
> > 
> > I see no failure scenarios in here. I mean I see no possibility to create test
> > that show that add function fail in del and opposite.
> > The only scenario what I have in my mind is that someone call add or/and
> > del functions on different threads with the same lpm table instance, but
> > this is not allowed, cause we know that this functions are not thread safe.
> > 
> > > >
> > > >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
> > 
> > I propose to classify this Coverity issues (ID 13205 and ID 13201) as
> > Intentional.
> > 
> > Regards,
> > Piotr
> 
> Hi Bruce,
> We would like to move forward with theses Coverity defects thus
> Please share your opinion about classifying these defects as Intentional?
> 
> Michal

From previous analysis detailed above, it looks like there is no issue with
failing to check the return values here, so I'm ok with this classification.

/Bruce
  

> -----Original Message-----

> From: Richardson, Bruce

> Sent: Thursday, June 23, 2016 10:48 AM

> To: Jastrzebski, MichalX K <michalx.k.jastrzebski@intel.com>

> Cc: Azarewicz, PiotrX T <piotrx.t.azarewicz@intel.com>; Mrozowicz,

> SlawomirX <slawomirx.mrozowicz@intel.com>; dev@dpdk.org

> Subject: Re: [PATCH] lpm: unchecked return value

> 

> On Thu, Jun 23, 2016 at 09:13:22AM +0100, Jastrzebski, MichalX K wrote:

> > > -----Original Message-----

> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Azarewicz,

> PiotrX T

> > > Sent: Thursday, May 12, 2016 1:20 PM

> > > To: Mrozowicz, SlawomirX <slawomirx.mrozowicz@intel.com>;

> Richardson,

> > > Bruce <bruce.richardson@intel.com>

> > > Cc: dev@dpdk.org

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

> > >

> > > Hi,

> > >

> > > I handle Coverity defect ID 13201. It is about unchecked return value

> from

> > > rte_lpm6_delete() instances in rte_lpm6_add() function.

> > > Next I found this thread and I see that both defects (ID 13205 and ID

> 13201)

> > > may be resolved all together.

> > >

> > > > >> 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.

> > >

> > > I agree that -ENOSPC should never occur here. So rte_lpm6_add()

> instance

> > > should never fail here.

> > >

> > > Next I looked at rte_lpm6_add() and if rte_lpm6_delete() instances in it

> > > may fail?

> > > The only suspicious place that I found is place when add every rule

> again

> > > but that should work as discussed above.

> > >

> > > > >

> > > > >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.

> > >

> > > I see no failure scenarios in here. I mean I see no possibility to create

> test

> > > that show that add function fail in del and opposite.

> > > The only scenario what I have in my mind is that someone call add

> or/and

> > > del functions on different threads with the same lpm table instance, but

> > > this is not allowed, cause we know that this functions are not thread

> safe.

> > >

> > > > >

> > > > >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

> > >

> > > I propose to classify this Coverity issues (ID 13205 and ID 13201) as

> > > Intentional.

> > >

> > > Regards,

> > > Piotr

> >

> > Hi Bruce,

> > We would like to move forward with theses Coverity defects thus

> > Please share your opinion about classifying these defects as Intentional?

> >

> > Michal

> 

> From previous analysis detailed above, it looks like there is no issue with

> failing to check the return values here, so I'm ok with this classification.

> 

> /Bruce


Thanks Bruce,
There is a third issue in Coverity tool, not listed here but related with the same problem
(CID13203) in rte_lpm6_delete_bulk_func function. We will also classify it as Intentional

Michal.