GBalakrishna replied on github web page:
platform/linux-dpdk/odp_crypto.c
line 41
@@ -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;
break;
case ODP_AUTH_ALG_SHA1_HMAC:
auth_xform->auth.algo = RTE_CRYPTO_AUTH_SHA1_HMAC;
- auth_xform->auth.digest_length = 20;
break;
case ODP_AUTH_ALG_SHA512_HMAC:
auth_xform->auth.algo = RTE_CRYPTO_AUTH_SHA512_HMAC;
- auth_xform->auth.digest_length = 64;
break;
case ODP_AUTH_ALG_AES_GCM:
#if ODP_DEPRECATED_API
case ODP_AUTH_ALG_AES128_GCM:
#endif
auth_xform->auth.algo = RTE_CRYPTO_AUTH_AES_GCM;
- auth_xform->auth.digest_length = 16;
Comment:
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_r161782818
updated_at 2018-01-16 15:03:28
