Dmitry Eremin-Solenikov(lumag) replied on github web page:
test/performance/odp_crypto.c
line 17
@@ -45,6 +45,21 @@ static uint8_t test_key24[24] = { 0x01, 0x02, 0x03, 0x04,
0x05,
0x15, 0x16, 0x17, 0x18
};
+static uint8_t test_key64[64] = { 0x01, 0x02, 0x03, 0x04, 0x05,
+ 0x06, 0x07, 0x08, 0x09, 0x0a,
+ 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
+ 0x10, 0x11, 0x12, 0x13, 0x14,
+ 0x15, 0x16, 0x17, 0x18, 0x19,
+ 0x1a, 0x1b, 0x1c, 0x1d, 0x1e,
+ 0x1f, 0x20, 0x21, 0x22, 0x23,
+ 0x24, 0x25, 0x26, 0x27, 0x28,
+ 0x29, 0x2a, 0x2b, 0x2c, 0x2d,
+ 0x2e, 0x2f, 0x30, 0x31, 0x32,
+ 0x33, 0x34, 0x35, 0x36, 0x37,
+ 0x38, 0x39, 0x3a, 0x3b, 0x3c,
+ 0x3d, 0x3e, 0x3f, 0x40
+};
Comment:
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_r161769748
updated_at 2018-01-16 14:21:10
