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 E636FA0531; Tue, 4 Feb 2020 10:51:34 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5A7EB1C000; Tue, 4 Feb 2020 10:51:34 +0100 (CET) Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.61]) by dpdk.org (Postfix) with ESMTP id 044BB1BFFD for <dev@dpdk.org>; Tue, 4 Feb 2020 10:51:31 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580809891; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=+EvoUtak53jBvpmXzZiCdxXAsz7wxlyWdue/cv3XVNw=; b=itk9IQ24ho76cqRpWZvrkSCsDQc6im7yjK2DDDgaKUWxNPvjH8Ov/7nbwm1/DTKNZcmWy+ u0QEvhL8POeEtRDi3wPHNRoBEjoysJ1y40ooj+R+Q9a/wzyCprFYoBjrg2gDhUNH+SxXqr TTx+obuBpdMNY63DjYLIuZW8yaVLkUQ= Received: from mail-vs1-f71.google.com (mail-vs1-f71.google.com [209.85.217.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-304-sZdd1WwZN6WGTMOU8wnBUw-1; Tue, 04 Feb 2020 04:51:29 -0500 Received: by mail-vs1-f71.google.com with SMTP id x15so1557282vsq.22 for <dev@dpdk.org>; Tue, 04 Feb 2020 01:51:29 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=zkej2NfYgWiJF7QQ3+jsY3XyTRI+bET4egxlx6Kd2QY=; b=mU5mwaX/bVs5dRF+z+cb4utSS5DxPIoYmR+ob1rG+loZ8ouvL58mXaNsHclSgbmTT7 0smPPlDxPcGfAS3I3dhHt6UwD05ByU/rytNGTygjcwz5ITxKdCQXeaVk3jaIb7kHsLof OYmWsKncvITkaWWQnRX0bwxgku4FaC2+I1C47DKzoGrxxQmb2TQNuHzU+TC2mtIRPKlS Oxd/El+lvkTlnVuWFZkVsjsTXZNV6snyD3rLo+I1HI4rsI3de9nDivQXFsj4ZADRxCXO 3i3IJF4gTupeeOlmiGtd+3mwHVIHCxvEP5wh35z2WclceXnhOkWXi8X8uNymQpe4vsCW 0wlw== X-Gm-Message-State: APjAAAXjMS+Ea97v9LIRbBxr/HtmuvldOEtdOzwouxB0baz95xy3/1xx GyXHFhxjErGKvV3g1ICXPGybwJ/aVZ7JtBJB/w4BlaiE6tpdI+swmLdfWjhh0EGKoP08Gsz8giC qIdg+tPNe2fOUMqaf3AM= X-Received: by 2002:ab0:618a:: with SMTP id h10mr17011268uan.53.1580809888802; Tue, 04 Feb 2020 01:51:28 -0800 (PST) X-Google-Smtp-Source: APXvYqwORO//ApM3K9i9mZRBuw15KN2dNdRRK9xS2miPOX8HkRuXwzQt3Ztk0y3sJ/a5xuNuOOIBHyEo+3pj9iX6GAo= X-Received: by 2002:ab0:618a:: with SMTP id h10mr17011244uan.53.1580809888298; Tue, 04 Feb 2020 01:51:28 -0800 (PST) MIME-Version: 1.0 References: <20191220152058.10739-1-david.marchand@redhat.com> <666f2cc7-0906-7a07-a582-87800f321a00@intel.com> <7566080.EvYhyI6sBW@xps> <2336620.usQuhbGJ8B@xps> <4ed777ce-8320-4636-2c9c-62bb96b66392@ashroe.eu> In-Reply-To: <4ed777ce-8320-4636-2c9c-62bb96b66392@ashroe.eu> From: David Marchand <david.marchand@redhat.com> Date: Tue, 4 Feb 2020 10:51:16 +0100 Message-ID: <CAJFAV8xg55tbhPSB4Xnkb7X862KChJSsgUZWmDJsUk71gvMptg@mail.gmail.com> To: Ray Kinsella <mdr@ashroe.eu> Cc: Thomas Monjalon <thomas@monjalon.net>, Neil Horman <nhorman@tuxdriver.com>, Luca Boccassi <bluca@debian.org>, Kevin Traynor <ktraynor@redhat.com>, "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, Akhil Goyal <akhil.goyal@nxp.com>, "Trahe, Fiona" <fiona.trahe@intel.com>, Ferruh Yigit <ferruh.yigit@intel.com>, dev <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>, Aaron Conole <aconole@redhat.com> X-MC-Unique: sZdd1WwZN6WGTMOU8wnBUw-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Mon, Feb 3, 2020 at 7:56 PM Ray Kinsella <mdr@ashroe.eu> wrote: > 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 e= xplanation 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 b= e still using the old value > >>>>>> which will be within bounds. If it's picking up the higher new val= ue 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-co= mpiled > >>>>> for the updated library. Indeed, compilation fixes compatibility is= sues. > >>>>> But this is not relevant for ABI compatibility. > >>>>> ABI compatibility means we can upgrade the library without recompil= ing > >>>>> 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 valu= e > >>>>> is returned by a function in the upgraded library. > >>>>> > >>>>>> There are also no structs on the API which contain arrays using th= is > >>>>>> 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 applicatio= n. > >>>>> > >>>>> 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 =3D 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 =3D 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 (=3D 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 >=3D MAX. > >> > >> Of course, blaming the API user is a lot easier than looking at the AP= I. > >> 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 breakag= e 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. Yes. > > > 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. It has a lot of friends, but it sounds like the right approach. Is someone looking into this? > So they only support/acknowledge the existence of Chacha-Poly for applica= tions build against > 20.02. > > It would be painful I know. > It would also mean that Chacha-Poly would only be available to those buil= ding against >=3D 20.02. Yes. -- David Marchand