From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <dev-bounces@dpdk.org> Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id D23AAA0531; Tue, 4 Feb 2020 11:26:09 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 93A521C001; Tue, 4 Feb 2020 11:26:09 +0100 (CET) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 523061C000 for <dev@dpdk.org>; Tue, 4 Feb 2020 11:26:07 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Feb 2020 01:46:35 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,401,1574150400"; d="scan'208";a="249295329" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.252.22.38]) ([10.252.22.38]) by orsmga002.jf.intel.com with ESMTP; 04 Feb 2020 01:46:31 -0800 To: Thomas Monjalon <thomas@monjalon.net>, David Marchand <david.marchand@redhat.com>, nhorman@tuxdriver.com, bluca@debian.org, ktraynor@redhat.com, Ray Kinsella <mdr@ashroe.eu> Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, Akhil Goyal <akhil.goyal@nxp.com>, "Trahe, Fiona" <fiona.trahe@intel.com>, dev@dpdk.org, Anoob Joseph <anoobj@marvell.com>, "Kusztal, ArkadiuszX" <arkadiuszx.kusztal@intel.com>, "Richardson, Bruce" <bruce.richardson@intel.com>, "Mcnamara, John" <john.mcnamara@intel.com>, dodji@seketeli.net, Andrew Rybchenko <arybchenko@solarflare.com>, aconole@redhat.com References: <20191220152058.10739-1-david.marchand@redhat.com> <2336620.usQuhbGJ8B@xps> <4ed777ce-8320-4636-2c9c-62bb96b66392@ashroe.eu> <2546229.NgBsaNRSFp@xps> From: Ferruh Yigit <ferruh.yigit@intel.com> Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJUBBMBCgA+AhsDAh4BAheABQsJCAcDBRUK CQgLBRYCAwEAFiEE0jZTh0IuwoTjmYHH+TPrQ98TYR8FAl1meboFCQlupOoACgkQ+TPrQ98T YR9ACBAAv2tomhyxY0Tp9Up7mNGLfEdBu/7joB/vIdqMRv63ojkwr9orQq5V16V/25+JEAD0 60cKodBDM6HdUvqLHatS8fooWRueSXHKYwJ3vxyB2tWDyZrLzLI1jxEvunGodoIzUOtum0Ce gPynnfQCelXBja0BwLXJMplM6TY1wXX22ap0ZViC0m714U5U4LQpzjabtFtjT8qOUR6L7hfy YQ72PBuktGb00UR/N5UrR6GqB0x4W41aZBHXfUQnvWIMmmCrRUJX36hOTYBzh+x86ULgg7H2 1499tA4o6rvE13FiGccplBNWCAIroAe/G11rdoN5NBgYVXu++38gTa/MBmIt6zRi6ch15oLA Ln2vHOdqhrgDuxjhMpG2bpNE36DG/V9WWyWdIRlz3NYPCDM/S3anbHlhjStXHOz1uHOnerXM 1jEjcsvmj1vSyYoQMyRcRJmBZLrekvgZeh7nJzbPHxtth8M7AoqiZ/o/BpYU+0xZ+J5/szWZ aYxxmIRu5ejFf+Wn9s5eXNHmyqxBidpCWvcbKYDBnkw2+Y9E5YTpL0mS0dCCOlrO7gca27ux ybtbj84aaW1g0CfIlUnOtHgMCmz6zPXThb+A8H8j3O6qmPoVqT3qnq3Uhy6GOoH8Fdu2Vchh TWiF5yo+pvUagQP6LpslffufSnu+RKAagkj7/RSuZV25Ag0EV9ZMvgEQAKc0Db17xNqtSwEv mfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ESYpV8QWj0xK4YM0dLxnDU2IYxjEshSB1T qAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4AibPtrHuIXWQOBECcVZTTOdZYGAzaYzxiA ONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxDUQljeNvKYt1lZE/gAUUxNLWsYyTT+22/ vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35p iVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVjsM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQ I3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdcq9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYH fVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH71PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZ qw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFBVOQOxCvwRG2QCgcJ/UTn5vlivul+cThi 6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJl Rr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYCGwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNh HwUCXWZ5wAUJB3FgggAKCRD5M+tD3xNhH2O+D/9OEz62YuJQLuIuOfL67eFTIB5/1+0j8Tsu o2psca1PUQ61SZJZOMl6VwNxpdvEaolVdrpnSxUF31kPEvR0Igy8HysQ11pj8AcgH0a9FrvU /8k2Roccd2ZIdpNLkirGFZR7LtRw41Kt1Jg+lafI0efkiHKMT/6D/P1EUp1RxOBNtWGV2hrd 0Yg9ds+VMphHHU69fDH02SwgpvXwG8Qm14Zi5WQ66R4CtTkHuYtA63sS17vMl8fDuTCtvfPF HzvdJLIhDYN3Mm1oMjKLlq4PUdYh68Fiwm+boJoBUFGuregJFlO3hM7uHBDhSEnXQr5mqpPM 6R/7Q5BjAxrwVBisH0yQGjsWlnysRWNfExAE2sRePSl0or9q19ddkRYltl6X4FDUXy2DTXa9 a+Fw4e1EvmcF3PjmTYs9IE3Vc64CRQXkhujcN4ZZh5lvOpU8WgyDxFq7bavFnSS6kx7Tk29/ wNJBp+cf9qsQxLbqhW5kfORuZGecus0TLcmpZEFKKjTJBK9gELRBB/zoN3j41hlEl7uTUXTI JQFLhpsFlEdKLujyvT/aCwP3XWT+B2uZDKrMAElF6ltpTxI53JYi22WO7NH7MR16Fhi4R6vh FHNBOkiAhUpoXRZXaCR6+X4qwA8CwHGqHRBfYFSU/Ulq1ZLR+S3hNj2mbnSx0lBs1eEqe2vh cA== Message-ID: <4dbd87be-9f37-1c75-7087-52c19b85e717@intel.com> Date: Tue, 4 Feb 2020 09:46:30 +0000 MIME-Version: 1.0 In-Reply-To: <2546229.NgBsaNRSFp@xps> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> On 2/3/2020 9:07 PM, Thomas Monjalon wrote: > 03/02/2020 19:55, Ray Kinsella: >> On 03/02/2020 17:34, Thomas Monjalon wrote: >>> 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 >> >> That would still be an ABI breakage though right. >> >>> b) wait 20.11 and revert Chacha-Poly from 20.02 >> >> Thanks for analysis above Fiona, Ferruh and all. >> >> That is a nasty one alright - there is no "good" answer here. >> I agree with Ferruh's sentiments overall, we should rethink this API for 20.11. >> Could do without an enumeration? >> >> There a c) though right. >> We could work around the issue by api versioning rte_cryptodev_info_get() and friends. >> So they only support/acknowledge the existence of Chacha-Poly for applications build against > 20.02. > > I agree there is a c) as I proposed in another email: > http://mails.dpdk.org/archives/dev/2020-February/156919.html > " > In this case, the proper solution is to implement > rte_cryptodev_info_get_v1911() so it filters out > RTE_CRYPTO_AEAD_CHACHA20_POLY1305 capability. > With this solution, an application compiled with DPDK 19.11 will keep > seeing the same range as before, while a 20.02 application could > see and use ChachaPoly. > " > >> It would be painful I know. > > Not so painful in my opinion. > Just need to call rte_cryptodev_info_get() from > rte_cryptodev_info_get_v1911() and filter the value > in the 19.11 range: [0..AES_GCM]. > >> It would also mean that Chacha-Poly would only be available to >> those building against >= 20.02. > > Yes exactly. > > The addition of comments and LIST_MAX like below are still valid > to avoid versioning after 20.11. > >>>> - _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. > > In order to avoid ABI check complaining, the best is to completely > remove LIST_END in DPDK 20.11. We can remove LIST_END only if we go with option (c). Two different approach, - Provide the LIST_END and expect application protect itself against new values can be coming in newer version of the library - Do ABI versioning to prevent application receive new values at all, (c). We can select one, but I believe the selection shouldn't be based on just silencing the ABI check tool. > > >>>> 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 > > >