From: "Mrozowicz, SlawomirX" <slawomirx.mrozowicz@intel.com>
To: "Richardson, Bruce" <bruce.richardson@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] lpm: unchecked return value
Date: Fri, 6 May 2016 12:25:49 +0000 [thread overview]
Message-ID: <158888A50F43E34AAE179517F56C97455A4043@IRSMSX103.ger.corp.intel.com> (raw)
In-Reply-To: <20160503143404.GA22728@bricha3-MOBL3>
>-----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
next prev parent reply other threads:[~2016-05-06 12:25 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-27 12:52 Slawomir Mrozowicz
2016-05-03 14:34 ` Bruce Richardson
2016-05-06 12:25 ` Mrozowicz, SlawomirX [this message]
2016-05-12 11:19 ` Azarewicz, PiotrX T
2016-06-23 8:13 ` Jastrzebski, MichalX K
2016-06-23 8:48 ` Bruce Richardson
2016-06-23 9:03 ` Jastrzebski, MichalX K
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=158888A50F43E34AAE179517F56C97455A4043@IRSMSX103.ger.corp.intel.com \
--to=slawomirx.mrozowicz@intel.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).