GBalakrishna replied on github web page:
platform/linux-dpdk/odp_crypto.c
line 26
@@ -122,29 +130,24 @@ static int auth_alg_odp_to_rte(odp_auth_alg_t auth_alg,
case ODP_AUTH_ALG_MD5_96:
#endif
auth_xform->auth.algo = RTE_CRYPTO_AUTH_MD5_HMAC;
- auth_xform->auth.digest_length = 12;
break;
case ODP_AUTH_ALG_SHA256_HMAC:
#if ODP_DEPRECATED_API
case ODP_AUTH_ALG_SHA256_128:
#endif
auth_xform->auth.algo = RTE_CRYPTO_AUTH_SHA256_HMAC;
- auth_xform->auth.digest_length = 16;
Comment:
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_r161782774
updated_at 2018-01-16 15:03:20
