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 A037BA0531; Tue, 4 Feb 2020 11:17:49 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7973A1C00E; Tue, 4 Feb 2020 11:17:49 +0100 (CET) Received: from us-smtp-delivery-1.mimecast.com (us-smtp-2.mimecast.com [207.211.31.81]) by dpdk.org (Postfix) with ESMTP id DE4911BFAD for ; Tue, 4 Feb 2020 11:17:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580811467; 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:autocrypt:autocrypt; bh=qLJr6RUFU0H3bPfPSyKc2N6glIquHEkIJtRAqTTmm2E=; b=IzUREd1CyAoHnn+M3qzBIfs0Jg9Hs5CkFLxBAXKN+aKlrdsdQJ6sEDxT63N0xTCSy8lxDo ZcZK40/a1Su0uufafmwm4drTptDjIoEfSb2n6fvjTrV0zf17f24Iua/5RGOmpTbu5tRW35 EGhpV7ri0BzC6s6ZsEXUKN7R4cNE2UA= 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-97-BQ_MYzbqOL2yxlz0hwCQZg-1; Tue, 04 Feb 2020 05:17:42 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AB8BB107B78F; Tue, 4 Feb 2020 10:17:40 +0000 (UTC) Received: from [10.33.36.105] (unknown [10.33.36.105]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0C229167A3; Tue, 4 Feb 2020 10:17:33 +0000 (UTC) To: Ferruh Yigit , Thomas Monjalon Cc: dev@dpdk.org, "Ananyev, Konstantin" , Akhil Goyal , "Trahe, Fiona" , David Marchand , Anoob Joseph , "Kusztal, ArkadiuszX" , "Richardson, Bruce" , nhorman@tuxdriver.com, "Mcnamara, John" , dodji@seketeli.net, Andrew Rybchenko , aconole@redhat.com, bluca@debian.org References: <20191220152058.10739-1-david.marchand@redhat.com> <2219936.atdPhlSkOF@xps> <0f85d878-c238-8531-e629-e41d49f5f05b@intel.com> <13361272.RDIVbhacDa@xps> <6c630641-d229-daff-92d5-fb80eae4bd16@intel.com> From: Kevin Traynor Autocrypt: addr=ktraynor@redhat.com; keydata= mQINBF2J2awBEADUEPNhgNI+nJNgiTAUcw4YIgVXEoHlsNPyyzG1BEXkWXALy0Y3fNTiw6+r ltWDkF9jzL9kfkecgQ67itGfk1OaBXgSGKuw1PUpxAwX2Bi76LAR6M5OsyGM9TSVVQwARalz hMwRBIZPzPc7or6Pw7jAOJ8SQGJ1Zlp1YJCjrvpe87V1tH/LY8Wnxn/EuoseFmWILAQZAtYS tGjcrAgYn3SPMLR1B0BP5bTBY06vWQjiufH8drenfDnMJAzuBdG1mqjnTqCjULZ3Hunv4xqZ aMnkvL/K5Tj1c12Oe4930EE53LrXIBUltRg5mBudSWHnC7twjH0082HH9f963Z/2UI63SFIT iUvRvAzJYytgy7XnWLQ0+goZBADKYfolOuC0H8VgCaux8u8KFF28Dy+N6TV2KI58jTlyg1Zu l7QwykZpnOkJFiy37Gfbu3YEOzO72cP/S7/A+zvuqkxi63jyEkd+FY99vLt/HN2MUZwRmKDw UPbLkmrs8WU01/POVsqDcfvz7vu2St8hqqTiSIdQGS2zyTKB2/DvPSM3jws3udkIYSuhn+X4 QBiV6lkVZ7DSE6a065gnAauAql+b32Eymy+xnG5jCt1tR+0Cp2VZYCR9OU2gmomUKBDoX/He pSgED01CqYPNjN+TddirwmQX7ep4DtXc8FWvv2g/pq9WZFQk2QARAQABtCNLZXZpbiBUcmF5 bm9yIDxrdHJheW5vckByZWRoYXQuY29tPokCTgQTAQgAOBYhBAoiOaH51tHF7VYtEI9CINER a+yJBQJdidmsAhsDBQsJCAcCBhUKCQgLAgQWAgMBAh4BAheAAAoJEI9CINERa+yJoxIP/3VF 2TIgW4ckxhRFCvFu/606bnvCPie88ake4uWVWMAWwcMc4fKEltRWRCpkSVOwgqoMHnyHxK5r kOKzx2CLJMX5TgTMfKzPuaBDHngHLUzl2DStpBzrod0cVg5TShdmmfjY61uxRJKz+DlSkwgJ riADdVF5PPosQXTkKSGf2ombpTGpx/pue9ocjnr3x4SDpRLlnooM6Jf/3Y3Ib4jX6HPEyWuY b+owIIk9y2nRRGPQ6jbqAhsrXd9V+77UL0QuGWloMuKMZFbNg8hbu7X5aFijAbfxj4YUgojS ba7gfGZQan8h32A9KGQWrmsCBc3j2GqEPsX0r05X7cn7WL6IOPgQJ5EiQ7PlazQYVLrvZg9B n0GKK0k6895mLG0ZZ5v/qajOPF52etSmvFD1WUPb4OqaHqGA9ZtMpaKFRt7Y6rpXqKNU1xzW F5KjbTPtTb9WF3An8dciVv+AYUI7totkZYkWvQtgss8lfaX3NKUvXLVxqK0z3dQyr7rF/tYz PneTKypSksjCgaEBLSrsRmM5zKfe7tSNF/fDntfIq/029Jtcw29TcWEP57peNu6TtejewQD9 sTI+oqiXvW2D5l7LNUDYG8eMJp2oT7I0ZSBRvwcbmjH0DtN/bXCCFfCvk8Yic68F3tV1ctix wQARVKDBhT30uCxycRWojCYqTgNJJS71uQINBF2J2awBEADP57PR2IpSYBeNSrsAjeIcsahE N4SQP2C4s50S8QEWAUhqMRI7WNv5cfeef0nDvcl1IUA6oz5SokbcsbMa+mRgaNF4N5KikWTO LPYxq2YVJoXwJ+tKmNzyOLFUIfFJ4NBJZple5dTfWzD00Dbb19Mri1hy1mWMqNTPGBee1+hw Qcp6n3mmGECvajs8G5A7NyXbwL8ihN7HX9D01ucD62b4G03yKe2g/hvKgcdUVmhCldJlF27I 2fSR9tDxH9pZqRODY4rjbFZEey/vWKXqjE+DQ8AtMSEaDfFe5D+i4Aw6erWQ3Wr+DwZt1/7G dIAElGA/q90T1ENVwJX9y7fsQssawKYYdDqURHCl5JuDXI+VXUypExipUUT5SPycMmbLsx0D iKEqPPDQWKxkIDVKqj2+EhamSuJznZUwBLJKn0h4zrIWiXWUy07lRwtVuhaDXhF3GfW+5W/x wAg7Qg3w00ASsb/XTHBIhMnenKDfS7ihtQA8SacwX8ySdxb+15XPyiplM979qBQ0mhnilulm MIJzEf/JxoYR5huuj4f1PFqqrsP06Dl+YGB7dQZp3IKggS5c3/TAynARRg9N89UsDXNtp7X0 tgIPFF5k6fnHE0J5O64GYHeTqN/1aE6dAEOV9WrGzQAJxU9ipikb8jKAWXzLewRIKGmoPcRZ WdB0NmIjmQARAQABiQI2BBgBCAAgFiEECiI5ofnW0cXtVi0Qj0Ig0RFr7IkFAl2J2awCGwwA CgkQj0Ig0RFr7IkkORAAl/NbX93WK5MEoRw7/DaPTo/Lo6Pj1XMeSqGyACigHK/452UDvlEH NjNJMzYYrNIjMtEmN9VVCfjT38CSca7mpGQVwchc0mC7QSPAETLCS+UacVf/Kwxz5FfkEUUw UT7A+uyVOIgW3d9ldlRzkHA2czonSSgTQU+i2g6DM4ha+BuQb4byAXH6HQHt/Zh1J64z0ohH v6iGsCzCY/sMWF8+LEGSnzMGRCLiiwSF0vJBHbzWK68fANaF4gBV0Z/+6tQRFN7YMhj/INmk qgvHj1ZzHFNtirjMGPRxoZs51YoLQM/aBPxKrnmXThx1ufH+0L6sGmFTugiDt0XSEkC5reH7 a+VhQ1VTFFQrClA8NmDSPzFeuhru4ryaaDHO+uEB16cNHxHrQtlP/2hts2JM5lwkZRWJ5A57 h8eDEIK5be47T85NVHfuTaboNRmgg1HygVejhGUtt69u/0MVRg/roUTa0FyEbNsvz4qAecyW yWzMcVrcGJDQLC9JLKEpoyUF6gdTKaiDL2Vao4+XRIA3Y57b6MO35a3HuzAv7+i5Z0mnDEJO XxXqTOmKYpMIGexzM/PtuA0712sT1abG9tAJ17ao/B7cqMW5IkKkalemFbWfI2unns4Papvo tk9igVqyp6EJDU98z5TJioCVojwK2laDaoIjTJk9YYv3iwCsqPd5feU= Message-ID: Date: Tue, 4 Feb 2020 10:17:33 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <6c630641-d229-daff-92d5-fb80eae4bd16@intel.com> Content-Language: en-US X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: BQ_MYzbqOL2yxlz0hwCQZg-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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 04/02/2020 09:56, Ferruh Yigit wrote: > On 2/4/2020 9:45 AM, Thomas Monjalon wrote: >> 04/02/2020 10:19, Ferruh Yigit: >>> On 2/3/2020 6:40 PM, Thomas Monjalon wrote: >>>> 03/02/2020 18:40, Ferruh Yigit: >>>>> On 2/3/2020 5:09 PM, Thomas Monjalon wrote: >>>>>> 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: >>>>>>>>>>> 30/01/2020 17:09, Ferruh Yigit: >>>>>>>>>>>> On 1/29/2020 8:13 PM, Akhil Goyal wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> I believe these enums will be used only in case of ASYM case = which is experimental. >>>>>>>>>>>> >>>>>>>>>>>> Independent from being experiment and not, this shouldn't be a= problem, I think >>>>>>>>>>>> this is a false positive. >>>>>>>>>>>> >>>>>>>>>>>> The ABI break can happen when a struct has been shared between= the application >>>>>>>>>>>> and the library (DPDK) and the layout of that memory know diff= erently by >>>>>>>>>>>> application and the library. >>>>>>>>>>>> >>>>>>>>>>>> Here in all cases, there is no layout/size change. >>>>>>>>>>>> >>>>>>>>>>>> As to the value changes of the enums, since application compil= ed with old DPDK, >>>>>>>>>>>> it will know only up to '6', 7 and more means invalid to the a= pplication. So it >>>>>>>>>>>> won't send these values also it should ignore these values fro= m library. Only >>>>>>>>>>>> consequence is old application won't able to use new features = those new enums >>>>>>>>>>>> provide but that is expected/normal. >>>>>>>>>>> >>>>>>>>>>> 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 v= alue 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 recomp= iling >>>>>>>>> the application and it must work. >>>>>>>>> You think it is a false positive because you assume the applicati= on >>>>>>>>> "picks" the new value. I think you miss the case where the new va= lue >>>>>>>>> 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 applicat= ion. >>>>>>>>> >>>>>>>>> Once upon a time a DPDK application counting the number of device= s >>>>>>>>> supporting each AEAD algo (in order to find the best supported al= go). >>>>>>>>> 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 marketin= g, >>>>>> everyone is OK. But when we need to really make our ABI compatible, >>>>>> I see little or no effort. DISAPPOINTING. >>>>> >>>>> This is not to blame the user or to do less work, this is more sane a= pproach >>>>> that library provides the _END/_MAX value and application uses it as = valid range >>>>> check. >>>>> >>>>>>> Do you suggest we don't extend any enum or define between ABI break= age releases >>>>>>> to be sure bad written applications not affected? >>>>>> >>>>>> I suggest we must consider not breaking any assumption made on the A= PI. >>>>>> Here we are breaking the enum range because nothing mentions _LIST_E= ND >>>>>> is not really the absolute end of the enum. >>>>>> The solution is to make the change below in 20.02 + backport in 19.1= 1.1: >>>>>> >>>>>> - _LIST_END >>>>>> + _LIST_END, /* an ABI-compatible version may increase this value */ >>>>>> + _LIST_MAX =3D _LIST_END + 42 /* room for ABI-compatible additions = */ >>>>>> }; >>>>>> >>>>> >>>>> What is the point of "_LIST_MAX" here? >>>> >>>> _LIST_MAX is range of value that DPDK can return in the ABI contract. >>>> So the appplication can rely on the range 0.._LIST_MAX. >>>> >>>>> Application should know the "_LIST_END" of when it has been compiled = for the >>>>> valid range check. Next time it is compiled "_LIST_END" may be differ= ent value >>>>> but same logic applies. >>>> >>>> No, ABI compatibility contract means you can compile your application >>>> with DPDK 19.11.0 and run it with DPDK 20.02. >>>> So _LIST_END comes from 19.11 and does not include ChachaPoly. >>> >>> That is what I mean, let me try to give a sample. >>> >>> DPDK19.11 returns, A=3D1, B=3D2, END=3D3 >>> >>> Application compiled with DPDK19.11, it will process A, B and ignore an= ything ">=3D 3" >> >> No, the application will not ignore anything ">=3D3" as I explained abov= e, >> and you blamed the application for it. >> Nothing in the API says the application must filter value higher than 3, >> because as of now, values higher than 3 are PMD bug. >=20 > When application compiled, that is the END value, anything bigger than th= is > value is not valid, if any application use the return value directly I th= ink it > is doing something wrong. I don't think we can make an assumption that the application will/should range check *and* silently ignore what it considers out of bounds values. An application may not range check, but if it does, it may ignore these new values (we got lucky), or it could print errors or even decide to abort because it considers that DPDK is now returning values higher than the (compiled) max so something must be corrupt. Versioning sounds the best solution to me too, but I'm not sure how difficult the mechanics are in this case. > But yes there may be applications relying on library will always send in = the > range. We never communicated this. But we can add comments to clarify thi= s. >=20 >> >> >>> DPDK20.02 returns A=3D1, B=3D2, C=3D3, D=3D4, END=3D5 >>> >>> Old application will still only will know/use A, B and can ignore when = library >>> sends C=3D3, D=3D4 etc... >>> >>> >>> In above, if you add another limit as you suggested, like MAX=3D10 and = ask >>> application to use it, >>> >>> Application compiled with DPDK19.11 will be OK since library only sends= A,B and >>> application uses them. >>> >>> But with DPDK20.02 application may have problem, since library will be = sending >>> C=3D3, which is valid according to the check " <=3D MAX (10)", how appl= ication will >>> know to ignore it. >> >> Why application should ignore value C=3D3 with DPDK 20.02? >=20 > This is the application compiled with DPDK19.11, and running with DPDK20.= 02. >=20 > So for the application this is the value >=3D MAX and something it doesn'= t know > what to do. >=20 >> >> >>> So application should use _END to know the valid ones according it, if = so what >>> is the point of having _MAX. >>> >>> >>>>> When "_LIST_END" is missing, application can't protect itself, in tha= t case >>>>> library should send only the values application knows when it is comp= iled, this >>>>> means either we can't extend our enum/defines until next ABI breakage= , or we >>>>> need to do ABI versioning to the functions that returns an enum each = time enum >>>>> value extended. >>>> >>>> If we define _LIST_MAX as a bigger value than current _LIST_END, >>>> we have some room to add values in between. >>>> >>>> If (as of now) we don't have _LIST_MAX room, then yes we must version >>>> the functions returning the enum. >>>> 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. >>>> This is another proposal that I was expecting from the crypto team, >>>> instead of claiming there is no issue (and wasting precious time). >>>> >>>> >>>>> I believe it is saner to provide _END/_MAX values to the application = to use. And >>>>> if required comment them to clarify the expected usage. >>>>> >>>>> But in above suggestion application can't use or rely on "_LIST_MAX",= it doesn't >>>>> mean anything to application. >>>> >>>> I don't understand what you mean. I think you misunderstood what is AB= I compat. >>>> >>>> >>>>>> Then *_LIST_END values could be ignored by libabigail with such a ch= ange. >>>>>> >>>>>> 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 >> >> >> >=20