From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id EB02158C3 for ; Thu, 12 May 2016 13:20:01 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP; 12 May 2016 04:20:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,609,1455004800"; d="scan'208";a="951717636" Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96]) by orsmga001.jf.intel.com with ESMTP; 12 May 2016 04:20:01 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.35]) by IRSMSX154.ger.corp.intel.com ([169.254.12.222]) with mapi id 14.03.0248.002; Thu, 12 May 2016 12:19:59 +0100 From: "Azarewicz, PiotrX T" To: "Mrozowicz, SlawomirX" , "Richardson, Bruce" CC: "dev@dpdk.org" Thread-Topic: [PATCH] lpm: unchecked return value Thread-Index: AQHRp5KSoPYdpWsoik2giNnKqiBje5+1BFRw Date: Thu, 12 May 2016 11:19:58 +0000 Message-ID: <4837007523CC9A4B9414D20C13DE6E64136B3E3D@IRSMSX102.ger.corp.intel.com> References: <1461761554-5900-1-git-send-email-slawomirx.mrozowicz@intel.com> <20160503143404.GA22728@bricha3-MOBL3> <158888A50F43E34AAE179517F56C97455A4043@IRSMSX103.ger.corp.intel.com> In-Reply-To: <158888A50F43E34AAE179517F56C97455A4043@IRSMSX103.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="iso-8859-2" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] lpm: unchecked return value X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 May 2016 11:20:02 -0000 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 > >> --- > >> 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 =3D 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 =3D 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 =3D 0; i < lpm->used_rules && status >=3D 0; i++) { > >> + status =3D 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 fir= st > place. I agree that -ENOSPC should never occur here. So rte_lpm6_add() instance sh= ould never fail here. Next I looked at rte_lpm6_add() and if rte_lpm6_delete() instances in it ma= y fail? The only suspicious place that I found is place when add every rule again b= ut 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 bet= ter > 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 t= est 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 de= l 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 >=20 >=20 > Hi Bruce, >=20 > 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. >=20 > 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 reconstructe= d > after delete operation. >=20 > 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. >=20 > Regards, > S=B3awomir I propose to classify this Coverity issues (ID 13205 and ID 13201) as Inten= tional. Regards, Piotr