From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 73CDAA0530; Mon, 3 Feb 2020 18:34:37 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 965631BFAE; Mon, 3 Feb 2020 18:34:36 +0100 (CET) Received: from new4-smtp.messagingengine.com (new4-smtp.messagingengine.com [66.111.4.230]) by dpdk.org (Postfix) with ESMTP id E7A931BFA1 for ; Mon, 3 Feb 2020 18:34:34 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailnew.nyi.internal (Postfix) with ESMTP id 5FBEF64AD; Mon, 3 Feb 2020 12:34:34 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Mon, 03 Feb 2020 12:34:34 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=s3nZ1wAOVY2W/zYml4abMuNuNKRExdaH4y+XNWAyAW0=; b=XaJfQaLose1v qEyJWJKgWihxnD1gTGxdvx3X/RaPhbzjFxoWkFU/7N3ZiDYWsOoZ+qd/LR5EjvFF rhWyyl+Jt0UTgcI1xDDW77uJ8piHP7FjaYnoyQujV5Xyq5sx8VmFWYcKNxbDhpNY V4OsqncNVwL6Cd5l6G9w23rnNOd3pJE= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=s3nZ1wAOVY2W/zYml4abMuNuNKRExdaH4y+XNWAyA W0=; b=WZ2mu7kFx2XrYmYUsefBAkdh3z5IwUxU5whGVs/XFi7pUD4lzjTxZNpkk R8bJz9VlPnGT+y9NxzgwFYvGtxieKWFunONd6do8vziaU1JuhI9UAvpZHT5xNbgZ AfCLZVBds0GdR+pciHBdLL3t0tY7uRLGaE8poZEugKZxO9IsWRhfEqsKPkMsk8Bj mc4UyZr2EIoE7i8XKnwAyoxj3PHW/jBwuMmEnXNjOdetu+0VhT824UU3MYm7vOkW cIB8X0SH+RAd1VPSYg1tNue91xcFG6aqFNGuwixh9NXZh6Ys6AfAYEaoEPvWZnHk ucw0gcBQynBebhXxKeiTc58LdTbrg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedugedrgeejgdellecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epjeejrddufeegrddvtdefrddukeegnecuvehluhhsthgvrhfuihiivgeptdenucfrrghr rghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id C705E328006A; Mon, 3 Feb 2020 12:34:31 -0500 (EST) From: Thomas Monjalon To: David Marchand , nhorman@tuxdriver.com, bluca@debian.org, ktraynor@redhat.com, Ray Kinsella Cc: "Ananyev, Konstantin" , Akhil Goyal , "Trahe, Fiona" , Ferruh Yigit , dev@dpdk.org, Anoob Joseph , "Kusztal, ArkadiuszX" , dev@dpdk.org, "Richardson, Bruce" , "Mcnamara, John" , dodji@seketeli.net, Andrew Rybchenko , aconole@redhat.com Date: Mon, 03 Feb 2020 18:34:29 +0100 Message-ID: <2336620.usQuhbGJ8B@xps> In-Reply-To: <7566080.EvYhyI6sBW@xps> References: <20191220152058.10739-1-david.marchand@redhat.com> <666f2cc7-0906-7a07-a582-87800f321a00@intel.com> <7566080.EvYhyI6sBW@xps> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v2 4/4] add ABI checks X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 03/02/2020 18:09, Thomas Monjalon: > 03/02/2020 10:30, Ferruh Yigit: > > On 2/2/2020 2:41 PM, Ananyev, Konstantin wrote: > > > 02/02/2020 14:05, Thomas Monjalon: > > >> 31/01/2020 15:16, Trahe, Fiona: > > >>> On 1/30/2020 8:18 PM, Thomas Monjalon wrote: > > >>>> If library give higher value than expected by the application, > > >>>> if the application uses this value as array index, > > >>>> there can be an access out of bounds. > > >>> > > >>> [Fiona] All asymmetric APIs are experimental so above shouldn't be a problem. > > >>> But for the same issue with sym crypto below, I believe Ferruh's explanation makes > > >>> sense and I don't see how there can be an API breakage. > > >>> So if an application hasn't compiled against the new lib it will be still using the old value > > >>> which will be within bounds. If it's picking up the higher new value from the lib it must > > >>> have been compiled against the lib so shouldn't have problems. > > >> > > >> You say there is no ABI issue because the application will be re-compiled > > >> for the updated library. Indeed, compilation fixes compatibility issues. > > >> But this is not relevant for ABI compatibility. > > >> ABI compatibility means we can upgrade the library without recompiling > > >> the application and it must work. > > >> You think it is a false positive because you assume the application > > >> "picks" the new value. I think you miss the case where the new value > > >> is returned by a function in the upgraded library. > > >> > > >>> There are also no structs on the API which contain arrays using this > > >>> for sizing, so I don't see an opportunity for an appl to have a > > >>> mismatch in memory addresses. > > >> > > >> Let me demonstrate where the API may "use" the new value > > >> RTE_CRYPTO_AEAD_CHACHA20_POLY1305 and how it impacts the application. > > >> > > >> Once upon a time a DPDK application counting the number of devices > > >> supporting each AEAD algo (in order to find the best supported algo). > > >> It is done in an array indexed by algo id: > > >> int aead_dev_count[RTE_CRYPTO_AEAD_LIST_END]; > > >> The application is compiled with DPDK 19.11, > > >> where RTE_CRYPTO_AEAD_LIST_END = 3. > > >> So the size of the application array aead_dev_count is 3. > > >> This binary is run with DPDK 20.02, > > >> where RTE_CRYPTO_AEAD_CHACHA20_POLY1305 = 3. > > >> When calling rte_cryptodev_info_get() on a device QAT_GEN3, > > >> rte_cryptodev_info.capabilities.sym.aead.algo is set to > > >> RTE_CRYPTO_AEAD_CHACHA20_POLY1305 (= 3). > > >> The application uses this value: > > >> ++ aead_dev_count[info.capabilities.sym.aead.algo]; > > >> The application is crashing because of out of bound access. > > > > > > I'd say this is an example of bad written app. > > > It probably should check that returned by library value doesn't > > > exceed its internal array size. > > > > +1 > > > > Application should ignore values >= MAX. > > Of course, blaming the API user is a lot easier than looking at the API. > Here the API has RTE_CRYPTO_AEAD_LIST_END which can be understood > as the max value for the application. > Value ranges are part of the ABI compatibility contract. > It seems you expect the application developer to be aware that > DPDK could return a higher value, so the application should > check every enum values after calling an API. CRAZY. > > When we decide to announce an ABI compatibility and do some marketing, > everyone is OK. But when we need to really make our ABI compatible, > I see little or no effort. DISAPPOINTING. > > > Do you suggest we don't extend any enum or define between ABI breakage releases > > to be sure bad written applications not affected? > > I suggest we must consider not breaking any assumption made on the API. > Here we are breaking the enum range because nothing mentions _LIST_END > is not really the absolute end of the enum. > The solution is to make the change below in 20.02 + backport in 19.11.1: Thinking twice, merging such change before 20.11 is breaking the ABI assumption based on the API 19.11.0. I ask the release maintainers (Luca, Kevin, David and me) and the ABI maintainers (Neil and Ray) to vote for a or b solution: a) add comment and LIST_MAX as below in 20.02 + 19.11.1 b) wait 20.11 and revert Chacha-Poly from 20.02 > - _LIST_END > + _LIST_END, /* an ABI-compatible version may increase this value */ > + _LIST_MAX = _LIST_END + 42 /* room for ABI-compatible additions */ > }; > > Then *_LIST_END values could be ignored by libabigail with such a change. > > If such a patch is not done by tomorrow, I will have to revert > Chacha-Poly commits before 20.02-rc2, because > > 1/ LIST_END, without any comment, means "size of range" > 2/ we do not blame users for undocumented ABI changes > 3/ we respect the ABI compatibility contract