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