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

["Jastrzebski, MichalX K" <michalx.k.jastrzebski@intel.com>]

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