Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-dpdk/test/wrapper-script.sh
line 5
@@ -1,6 +1,7 @@
#!/bin/bash
-export ODP_PLATFORM_PARAMS=${ODP_PLATFORM_PARAMS:--n 4 --vdev "crypto_openssl"}
+export ODP_PLATFORM_PARAMS=${ODP_PLATFORM_PARAMS:--n 4 "--vdev \
+"crypto_openssl" --vdev "crypto_null""}
Comment:
Can you please paste the test log w/o crypto_null somewhere (pastebin or
e-mail)? Thanks!
> Dmitry Eremin-Solenikov(lumag) wrote:
> Yes.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Yes. 0 is also a valid iv length.
>>> GBalakrishna wrote
>>> you mean to say for MD5_96, we should ignore the digest_length from
>>> application even if it is set ?
>>>> GBalakrishna wrote
>>>> you mean to make this in separate commit ?
>>>>> GBalakrishna wrote
>>>>> you mean is_valid_size() ?
>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>> OK. You always use `param->auth_digest_len` if it is set. This logic is
>>>>>> incorrect. It should be used for all cases except MD5_96, SHA256_128,
>>>>>> AES128_GCM (where an override should be used).
>>>>>>> GBalakrishna wrote
>>>>>>> yes it works.
>>>>>>>> GBalakrishna wrote
>>>>>>>> it is set in get_crypto_dev()
>>>>>>>>> GBalakrishna wrote
>>>>>>>>> it is set in get_crypto_dev()
>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>> it is set in get_crypto_dev(): auth_xform->auth.digest_length =
>>>>>>>>>> cap->sym.auth.digest_size.min
>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>> All test updates should go in separate commit.
>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>> Name of the test suggests, that test is using truncated hmac, but
>>>>>>>>>>>> then you specify full auth_digest_len.
>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>> and this
>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>> Will it work w/o crypto_null driver?
>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>> `res` does not quite follow `crypto_chain_order`, does it?
>>>>>>>>>>>>>>> Please rename to 'order' or smth. like that.
>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>> Push this logic to `set_chain_order`
>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>> Make it return `crypto_chain_order`.
>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>> No need to keep this under `if (iv_length)`
>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>> Liked this typo. It shows that the code never worked as
>>>>>>>>>>>>>>>>>>> expected.
>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>> same issue here.
>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>> Same issue here.
>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>> `auth.digest_length` should be set for MD5_96 here, but
>>>>>>>>>>>>>>>>>>>>>> not for MD5_HMAC.
>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>> It's really not a problem at this moment, this can be
>>>>>>>>>>>>>>>>>>>>>>> fixed later.
>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>> oke I see. I will make the update in the next version.
>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>> both = Encrypt-then-MAC and MAC-cleartext
>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>> Could you please add tests cases rather than modify
>>>>>>>>>>>>>>>>>>>>>>>>>> existing ones?
>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>> Implementation should not set digest len for cases
>>>>>>>>>>>>>>>>>>>>>>>>>>> other than several deprecated auth algos.
>>>>>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>>>>>> setting digest_length is moved to
>>>>>>>>>>>>>>>>>>>>>>>>>>>> get_crypto_dev(), where it reads from PMD and set
>>>>>>>>>>>>>>>>>>>>>>>>>>>> the min digest length if application doesn't set
>>>>>>>>>>>>>>>>>>>>>>>>>>>> it. Reagrding the QAT, I need to look at it and
>>>>>>>>>>>>>>>>>>>>>>>>>>>> understand. may be as a separate PR.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I have updated the tests so that it works with
>>>>>>>>>>>>>>>>>>>>>>>>>>>>> both the platforms linux-generci & linux-dpdk.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> what do you mean by testing both combinations ?.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> We are testing both the combinations ENCODE &
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DECODE. It's the default value we are changing
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Checkpatch flags this line:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> WARNING: line over 80 characters
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> #363: FILE:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> platform/linux-dpdk/test/wrapper-script.sh:3:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> +export
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ODP_PLATFORM_PARAMS=${ODP_PLATFORM_PARAMS:--n 4
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> --vdev "crypto_openssl" --vdev "crypto_null"}
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> total: 0 errors, 1 warnings, 0 checks, 530
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> lines checked
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> NOTE: Ignored message types: AVOID_EXTERNS
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> BIT_MACRO COMPARISON_TO_NULL
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> DEPRECATED_VARIABLE NEW_TYPEDEFS PREFER_PRINTF
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PREFER_SCANF SPLIT_STRING SSCANF_TO_KSTRTO
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> VOLATILE
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 0001-linux-dpdk-crypto-support-for-cipher-auth-only-featu.patch
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> has style problems, please review.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> ```
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I suggest to factor out cipher and auth checks
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> as separate functions and apply them only if
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> corresponding algorithm is not NULL. Code
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> would be simpler.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> For deprecated MD5_96 (and
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> SHA256_128/AES128_GCM) application won't set
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `auth.digest_len`, so you have to enforce 12
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and 16 bytes digest len. For
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> MD5_HMAC/SHA256_HMAC (and others) application
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> will set `auth.digest_len`.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> IIRC QAT driver should support truncated
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> digests out-of-box. Also you can try
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> expanding pmd_openssl to support truncated
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> lengths, it should not be very complicated.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Also note, that full-length HMAC won't be
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> useful for IPsec.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> "Do not change tests". See my previous
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> comment.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Theoretically we'd better be testing both
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> combinations. The problem with test changes
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> in your commit is that you change tests
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> rather than expand them. Can you import
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> tests from #379?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> not sure what do u mean here.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Not sure if I have fallowed. I introduced
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Chain order because dpdk PMD's doesn't
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> have support for combination of NULL +
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Valid algo. So linux-dpdk implementation
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> converts the NULL also to chain order as
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> if it cipher only & auth only.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> my understanding of auth_cipher_text =
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> true means do encode then authenticate.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> and when it is applied together with if
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> (ODP_CRYPTO_OP_ENCODE == param->op) sets
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> the "entry->do_cipher_first" to true by
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> default and if the param->op == DECODE
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> it sets the !auth_cipher_text. I felt
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> this is more easier for understanding.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> GBalakrishna wrote
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> openssl PMD in 17.02 supports the
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> digest length to be min 16 bytes. But I
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> have now removed the setting
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> digest_lengths from here as it sets it
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> from the application.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> why?
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> `non tangere testos meos`
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> I'd suggest to use ALG_NULL checks
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> directly, rather than introduce
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> chain order.
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag)
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> This is wrong. MD5_96 should use
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 12-byte digest length. MD5_HMAC
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> should use digest length specified
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> by application.
https://github.com/Linaro/odp/pull/385#discussion_r161831026
updated_at 2018-01-16 17:34:40
