From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <michalx.k.jastrzebski@intel.com>
Received: from mga02.intel.com (mga02.intel.com [134.134.136.20])
 by dpdk.org (Postfix) with ESMTP id 7DCB0C484
 for <dev@dpdk.org>; Thu, 23 Jun 2016 10:13:25 +0200 (CEST)
Received: from fmsmga003.fm.intel.com ([10.253.24.29])
 by orsmga101.jf.intel.com with ESMTP; 23 Jun 2016 01:13:24 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.26,509,1459839600"; d="scan'208";a="723816983"
Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31])
 by FMSMGA003.fm.intel.com with ESMTP; 23 Jun 2016 01:13:23 -0700
Received: from irsmsx109.ger.corp.intel.com ([169.254.13.193]) by
 IRSMSX106.ger.corp.intel.com ([169.254.8.145]) with mapi id 14.03.0248.002;
 Thu, 23 Jun 2016 09:13:22 +0100
From: "Jastrzebski, MichalX K" <michalx.k.jastrzebski@intel.com>
To: "Azarewicz, PiotrX T" <piotrx.t.azarewicz@intel.com>, "Mrozowicz,
 SlawomirX" <slawomirx.mrozowicz@intel.com>, "Richardson, Bruce"
 <bruce.richardson@intel.com>
CC: "dev@dpdk.org" <dev@dpdk.org>
Thread-Topic: [PATCH] lpm: unchecked return value
Thread-Index: AQHRp5KDSB8yq08AuUCxztS3iqNGgp+1IIYAgEHb12A=
Date: Thu, 23 Jun 2016 08:13:22 +0000
Message-ID: <60ABE07DBB3A454EB7FAD707B4BB158213AADDBC@IRSMSX109.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>
 <4837007523CC9A4B9414D20C13DE6E64136B3E3D@IRSMSX102.ger.corp.intel.com>
In-Reply-To: <4837007523CC9A4B9414D20C13DE6E64136B3E3D@IRSMSX102.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.180]
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 <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Thu, 23 Jun 2016 08:13:26 -0000

> -----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
>=20
> Hi,
>=20
> I handle Coverity defect ID 13201. It is about unchecked return value fro=
m
> rte_lpm6_delete() instances in rte_lpm6_add() function.
> Next I found this thread and I see that both defects (ID 13205 and ID 132=
01)
> may be resolved all together.
>=20
> > >> 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 =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 f=
irst
> > place.
>=20
> I agree that -ENOSPC should never occur here. So rte_lpm6_add() instance
> should never fail here.
>=20
> 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.
>=20
> > >
> > >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 ther=
e
> > >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.
>=20
> 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 saf=
e.
>=20
> > >
> > >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 reconstruc=
ted
> > 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=B3awomir
>=20
> I propose to classify this Coverity issues (ID 13205 and ID 13201) as
> Intentional.
>=20
> 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