GBalakrishna replied on github web page:
platform/linux-generic/odp_ipsec_sad.c
line 12
@@ -303,13 +303,15 @@ odp_ipsec_sa_t odp_ipsec_sa_create(const
odp_ipsec_sa_param_t *param)
case ODP_CIPHER_ALG_3DES_CBC:
ipsec_sa->esp_iv_len = 8;
ipsec_sa->esp_block_len = 8;
+ crypto_param.iv.length = 8;
break;
#if ODP_DEPRECATED_API
case ODP_CIPHER_ALG_AES128_CBC:
#endif
case ODP_CIPHER_ALG_AES_CBC:
ipsec_sa->esp_iv_len = 16;
ipsec_sa->esp_block_len = 16;
+ crypto_param.iv.length = 16;
Comment:
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_r161802113
updated_at 2018-01-16 15:59:54
