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 1DE58A0535; Tue, 4 Feb 2020 15:45:33 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1E57B1C1E0; Tue, 4 Feb 2020 15:45:22 +0100 (CET) Received: from us-smtp-delivery-1.mimecast.com (us-smtp-2.mimecast.com [205.139.110.61]) by dpdk.org (Postfix) with ESMTP id 503901C1D0 for ; Tue, 4 Feb 2020 15:45:20 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580827519; 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=tgk3L3ddy1GMZmwLLp2g5gvk6GcDd3Gmn4oq2vc42sg=; b=Fm6JNhG4MwH7sY9rosV0gwTzyfcGt4GLDZM3mtwZsnDmwe1spF2d/PeIfmFx2Q9LYtZ1Ux sgs+MBKoMAicGdSH8PdxtRdEYMvQf5y8uTTx+2PZsIRkkhdSre2yKo6PSvDkXXRItO1IRv +G+WqQwTXWY6Qppbh6a/Soqvjp8aI0k= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-296--qm40VAXMY6dVv0ychYtLg-1; Tue, 04 Feb 2020 09:45:03 -0500 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1D459107B286; Tue, 4 Feb 2020 14:45:00 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.126]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AD71B60BF7; Tue, 4 Feb 2020 14:44:54 +0000 (UTC) From: Aaron Conole To: Thomas Monjalon Cc: David Marchand , nhorman@tuxdriver.com, bluca@debian.org, ktraynor@redhat.com, Ray Kinsella , dev@dpdk.org, Akhil Goyal , "Trahe\, Fiona" , Ferruh Yigit , "Ananyev\, Konstantin" , Anoob Joseph , "Kusztal\, ArkadiuszX" , "Richardson\, Bruce" , "Mcnamara\, John" , dodji@seketeli.net, Andrew Rybchenko References: <20191220152058.10739-1-david.marchand@redhat.com> <4ed777ce-8320-4636-2c9c-62bb96b66392@ashroe.eu> <2546229.NgBsaNRSFp@xps> <6660180.4vTCxPXJkl@xps> Date: Tue, 04 Feb 2020 09:44:53 -0500 In-Reply-To: <6660180.4vTCxPXJkl@xps> (Thomas Monjalon's message of "Tue, 04 Feb 2020 11:24:12 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-MC-Unique: -qm40VAXMY6dVv0ychYtLg-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Thomas Monjalon writes: > RED FLAG > > I don't see a lot of reactions, so I summarize the issue. > We need action TODAY! > > API makes think that rte_cryptodev_info_get() cannot return > a value >=3D 3 (RTE_CRYPTO_AEAD_LIST_END in 19.11). > Current 20.02 returns 3 (RTE_CRYPTO_AEAD_CHACHA20_POLY1305). > The ABI compatibility contract is broken currently. > > There are 3 possible outcomes: > > a) Change the API comments and backport to 19.11.1 > The details are discussed between Ferruh and me. > Either put responsibility on API user (with explicit comment), > or expose ABI extension allowance with a new API max value. > In both cases, this is breaking the implicit contract of 19.11.0. > This option can be chosen only if release and ABI maintainers > vote for it. > > b) Revert Chacha-Poly from 20.02-rc2. > > c) Add versioned function rte_cryptodev_info_get_v1911() > which calls rte_cryptodev_info_get() and filters out > RTE_CRYPTO_AEAD_CHACHA20_POLY1305 capability. > So Chacha-Poly capability would be seen and usable only > if compiling with DPDK 20.02. > > I hope it is clear what are the actions for everybody: > - ABI and release maintainers must say yes or no to the proposal (a) > - In the meantime, crypto team must send a patch for the proposal (c) > - If (a) and (c) are not possible at the end of today, I will take (b) > > Note: do not say it is too short for (c), as it was possible to work > on such solution since the issue was exposed on last Wednesday. While I'm not a maintainer, if I my opinion counts for anything, I'd choose option c or b. Absolutely NACK to a. > > 03/02/2020 22:07, Thomas Monjalon: >> 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 recom= piling >> > >>>>> the application and it must work. >> > >>>>> You think it is a false positive because you assume the applicat= ion >> > >>>>> "picks" the new value. I think you miss the case where the new v= alue >> > >>>>> 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 applica= tion. >> > >>>>> >> > >>>>> Once upon a time a DPDK application counting the number of devic= es >> > >>>>> supporting each AEAD algo (in order to find the best supported a= lgo). >> > >>>>> 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= 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 marketi= ng, >> > >> 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 brea= kage 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: >> > >=20 >> > > 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: >> > > =09a) add comment and LIST_MAX as below in 20.02 + 19.11.1 >> >=20 >> > That would still be an ABI breakage though right. >> >=20 >> > > =09b) wait 20.11 and revert Chacha-Poly from 20.02 >> >=20 >> > Thanks for analysis above Fiona, Ferruh and all.=20 >> >=20 >> > That is a nasty one alright - there is no "good" answer here. >> > I agree with Ferruh's sentiments overall, we should rethink this API f= or 20.11.=20 >> > Could do without an enumeration? >> >=20 >> > There a c) though right. >> > We could work around the issue by api versioning rte_cryptodev_info_ge= t() and friends. >> > So they only support/acknowledge the existence of Chacha-Poly for >> > applications build against > 20.02. >>=20 >> 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. >> " >>=20 >> > It would be painful I know. >>=20 >> 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]. >>=20 >> > It would also mean that Chacha-Poly would only be available to >> > those building against >=3D 20.02. >>=20 >> Yes exactly. >>=20 >> The addition of comments and LIST_MAX like below are still valid >> to avoid versioning after 20.11. >>=20 >> > >> - _LIST_END >> > >> + _LIST_END, /* an ABI-compatible version may increase this value *= / >> > >> + _LIST_MAX =3D _LIST_END + 42 /* room for ABI-compatible additions= */ >> > >> }; >> > >> >> > >> Then *_LIST_END values could be ignored by libabigail with such a c= hange. >>=20 >> In order to avoid ABI check complaining, the best is to completely >> remove LIST_END in DPDK 20.11. >>=20 >>=20 >> > >> 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