[dpdk-dev] [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl
-Original Message- From: De Lara Guarch, Pablo [mailto:pablo.de.lara.gua...@intel.com] Sent: Monday, October 17, 2016 10:35 PM To: Gonzalez Monroy, Sergio ; Akhil Goyal ; dev at dpdk.org Subject: RE: [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl > -Original Message- > From: Gonzalez Monroy, Sergio > Sent: Monday, October 10, 2016 5:05 AM > To: De Lara Guarch, Pablo; Akhil Goyal; dev at dpdk.org > Subject: Re: [PATCH] examples/ipsec-secgw: Update checksum while > decrementing ttl > > On 07/10/2016 21:53, De Lara Guarch, Pablo wrote: > >> -Original Message- > >> From: Akhil Goyal [mailto:akhil.goyal at nxp.com] > >> Sent: Tuesday, October 04, 2016 11:33 PM > >> To: De Lara Guarch, Pablo; Gonzalez Monroy, Sergio; dev at dpdk.org > >> Subject: Re: [PATCH] examples/ipsec-secgw: Update checksum while > >> decrementing ttl > >> > >> On 10/5/2016 6:04 AM, De Lara Guarch, Pablo wrote: > >>> > >>>> -Original Message- > >>>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Sergio > Gonzalez > >>>> Monroy > >>>> Sent: Monday, September 26, 2016 6:28 AM > >>>> To: akhil.goyal at nxp.com; dev at dpdk.org > >>>> Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: Update > checksum > >>>> while decrementing ttl > >>>> > >>>> Hi Akhil, > >>>> > >>>> This application relies on checksum offload in both outbound and > >> inbound > >>>> paths (PKT_TX_IP_CKSUM flag). > >> [Akhil]Agreed that the application relies on checksum offload, but > >> here we are talking about the inner ip header. Inner IP checksum > >> will be updated on the next end point after decryption. This would > >> expect that the next end point must have checksum offload > >> capability. What if we are capturing the encrypted packets on > >> wireshark or say send it to some other machine which does not run > >> DPDK and do not know about > checksum > >> offload, then wireshark/other machine will not be able to get the > >> correct the checksum and will show error. > > Understood, we need to have a valid inner checksum. > RFC1624 states that the computation would be incorrect in > corner/boundary case. > I reckon you are basing your incremental update on RFC1141? > > Also I think you should take care of endianess and increment the > checksum with > host_to_be(0x0100) instead of +1. > > >>>> Because we assume that we always forward the packet in both > >>>> paths, > we > >>>> decrement the ttl in both inbound and outbound. > >>>> You seem to only increment (recalculate) the checksum of the > >>>> inner IP header in the outbound path but not the inbound path. > >> [Akhil]Correct I missed out the inbound path. > >>>> Also, in the inbound path you have to consider a possible ECN > >>>> value > >> update. > >> [Akhil]If I take care of the ECN then it would mean I need to > >> calculate the checksum completely, incremental checksum wont give correct > >> results. > >> This would surely impact performance. Any suggestion on how should > >> we take care of ECN update. Should I recalculate the checksum and > >> send the patch for ECN update? Or do we have a better solution. > > If I am understanding the RFCs mentioned above correctly, you should > be able to do incremental checksum update for any 16bit field/value of > the IP header. > I don't see no reason why you couldn't do something like that, except > that you would have to follow the full equation instead of just adding > 0x0100, which would be always the case when decrementing TTL. > > What do you think? Any comments, Akhil? Ok.. will send next version soon.
[dpdk-dev] [PATCH v2] test_cryptodev_perf: IV and digest should be stored at a DMAeble address
On 10/8/2016 3:06 AM, De Lara Guarch, Pablo wrote: > Hi Akhil, > >> -Original Message- >> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of >> akhil.goyal at nxp.com >> Sent: Friday, October 07, 2016 10:06 AM >> To: Kusztal, ArkadiuszX; Doherty, Declan >> Cc: Griffin, John; Trahe, Fiona; Jain, Deepak K; dev at dpdk.org; Akhil Goyal >> Subject: [dpdk-dev] [PATCH v2] test_cryptodev_perf: IV and digest should be >> stored at a DMAeble address >> >> From: Akhil Goyal >> >> For physical crypto devices, IV and digest are processed by the crypto >> device which need the contents to be written on some DMA able address. >> >> So in order to do that, IV and digest are accomodated in the packet. >> >> Signed-off-by: Akhil Goyal >> v2: patch rebased > > You need to rebase against the HEAD of the dpdk-next-crypto subtree: > (http://dpdk.org/browse/next/dpdk-next-crypto/). > > Thanks! > Pablo > > > > > Hi Pablo, I have already rebased this patch to dpdk-next-crypto subtree. Please let me know if there is any issue. Here is my git log 779f1301a382b6b9e2877fd0357bba33d1242d65 test_cryptodev_perf: IV and digest should be stored at a DMAeble address 8c9fdf4568768b7765fc2176e400a860dc758020 app/test: remove hard-coding of crypto num qps c1876c1cb90f0882ada0acd9e430be7cf63bc765 app/test: cleanup unnecessary ring size setup 136592c3a350ded56438b59cc4921a243f08e1d0 app/test: remove pointless for loop fca4f966b42adc0c8f3e1d43a94d93ea4fcb crypto/aesni_mb: free ring memory on qp release in PMD Regards
[dpdk-dev] [PATCH] test_cryptodev_perf: IV and digest should be stored at a DMAeble address
Hi Arek, Ok. I would rebase the patch. Regarding changes required to qat_snow3g, I do not have setup to test on qat and the hardware that I test, currently snow3g support is not added. I can send the patches for snow3g at some later stage. Regards, Akhil -Original Message- From: Kusztal, ArkadiuszX [mailto:arkadiuszx.kusz...@intel.com] Sent: Wednesday, October 05, 2016 2:57 PM To: Akhil Goyal ; dev at dpdk.org; Doherty, Declan Cc: Jain, Deepak K ; Trahe, Fiona ; Griffin, John Subject: RE: [dpdk-dev] [PATCH] test_cryptodev_perf: IV and digest should be stored at a DMAeble address Hi Akhil, Could you rebase it against newest next-crypto subtree, there were changes with function names in the meantime. And to make it really consistent across all hw tests could you add this change to qat_snow3g too, for snow3g I assume aad need to obtain correct physical address too. Regards, Arek > -Original Message- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Akhil Goyal > Sent: Wednesday, October 05, 2016 7:40 AM > To: dev at dpdk.org; Doherty, Declan > Subject: Re: [dpdk-dev] [PATCH] test_cryptodev_perf: IV and digest > should be stored at a DMAeble address > > On 9/26/2016 10:03 PM, akhil.goyal at nxp.com wrote: > > From: Akhil Goyal > > > > For physical crypto devices, IV and digest are processed by the > > crypto device which need the contents to be written on some DMA able > > address. > > > > So in order to do that, IV and digest are accomodated in the packet. > > > > Signed-off-by: Akhil Goyal > > --- > > app/test/test_cryptodev_perf.c | 10 -- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/app/test/test_cryptodev_perf.c > > b/app/test/test_cryptodev_perf.c index 0ea7ec1..930d5b8 100644 > > --- a/app/test/test_cryptodev_perf.c > > +++ b/app/test/test_cryptodev_perf.c > > @@ -2366,9 +2366,13 @@ test_perf_set_crypto_op(struct rte_crypto_op > *op, struct rte_mbuf *m, > > op->sym->auth.aad.length = AES_CBC_CIPHER_IV_LENGTH; > > > > /* Cipher Parameters */ > > - op->sym->cipher.iv.data = aes_cbc_iv; > > + op->sym->cipher.iv.data = (uint8_t *)m->buf_addr + m->data_off; > > + op->sym->cipher.iv.phys_addr = rte_pktmbuf_mtophys(m); > > op->sym->cipher.iv.length = AES_CBC_CIPHER_IV_LENGTH; > > > > + rte_memcpy(op->sym->cipher.iv.data, aes_cbc_iv, > > + AES_CBC_CIPHER_IV_LENGTH); > > + > > /* Data lengths/offsets Parameters */ > > op->sym->auth.data.offset = 0; > > op->sym->auth.data.length = data_len; @@ -2468,7 +2472,9 @@ > > test_perf_aes_sha(uint8_t dev_id, uint16_t queue_id, > > rte_pktmbuf_free(mbufs[k]); > > return -1; > > } > > - > > + /* Make room for Digest and IV in mbuf */ > > + rte_pktmbuf_append(mbufs[i], digest_length); > > + rte_pktmbuf_prepend(mbufs[i], > AES_CBC_CIPHER_IV_LENGTH); > > } > > > > > > > Hi Declan, > > Sorry I missed out copy your name in the TO list. Do we have some > comments on this patch. > > Regards, > Akhil
[dpdk-dev] [PATCH] test_cryptodev_perf: IV and digest should be stored at a DMAeble address
On 9/26/2016 10:03 PM, akhil.goyal at nxp.com wrote: > From: Akhil Goyal > > For physical crypto devices, IV and digest are processed by the crypto > device which need the contents to be written on some DMA able address. > > So in order to do that, IV and digest are accomodated in the packet. > > Signed-off-by: Akhil Goyal > --- > app/test/test_cryptodev_perf.c | 10 -- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/app/test/test_cryptodev_perf.c b/app/test/test_cryptodev_perf.c > index 0ea7ec1..930d5b8 100644 > --- a/app/test/test_cryptodev_perf.c > +++ b/app/test/test_cryptodev_perf.c > @@ -2366,9 +2366,13 @@ test_perf_set_crypto_op(struct rte_crypto_op *op, > struct rte_mbuf *m, > op->sym->auth.aad.length = AES_CBC_CIPHER_IV_LENGTH; > > /* Cipher Parameters */ > - op->sym->cipher.iv.data = aes_cbc_iv; > + op->sym->cipher.iv.data = (uint8_t *)m->buf_addr + m->data_off; > + op->sym->cipher.iv.phys_addr = rte_pktmbuf_mtophys(m); > op->sym->cipher.iv.length = AES_CBC_CIPHER_IV_LENGTH; > > + rte_memcpy(op->sym->cipher.iv.data, aes_cbc_iv, > + AES_CBC_CIPHER_IV_LENGTH); > + > /* Data lengths/offsets Parameters */ > op->sym->auth.data.offset = 0; > op->sym->auth.data.length = data_len; > @@ -2468,7 +2472,9 @@ test_perf_aes_sha(uint8_t dev_id, uint16_t queue_id, > rte_pktmbuf_free(mbufs[k]); > return -1; > } > - > + /* Make room for Digest and IV in mbuf */ > + rte_pktmbuf_append(mbufs[i], digest_length); > + rte_pktmbuf_prepend(mbufs[i], AES_CBC_CIPHER_IV_LENGTH); > } > > > Hi Declan, Sorry I missed out copy your name in the TO list. Do we have some comments on this patch. Regards, Akhil
[dpdk-dev] [PATCH] examples/ipsec-secgw: Update checksum while decrementing ttl
On 10/5/2016 6:04 AM, De Lara Guarch, Pablo wrote:
>
>
>> -Original Message-
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Sergio Gonzalez
>> Monroy
>> Sent: Monday, September 26, 2016 6:28 AM
>> To: akhil.goyal at nxp.com; dev at dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: Update checksum
>> while decrementing ttl
>>
>> Hi Akhil,
>>
>> This application relies on checksum offload in both outbound and inbound
>> paths (PKT_TX_IP_CKSUM flag).
[Akhil]Agreed that the application relies on checksum offload, but here
we are talking about the inner ip header. Inner IP checksum will be
updated on the next end point after decryption. This would expect that
the next end point must have checksum offload capability. What if we are
capturing the encrypted packets on wireshark or say send it to some
other machine which does not run DPDK and do not know about checksum
offload, then wireshark/other machine will not be able to get the
correct the checksum and will show error.
>>
>> Because we assume that we always forward the packet in both paths, we
>> decrement the ttl in both inbound and outbound.
>> You seem to only increment (recalculate) the checksum of the inner IP
>> header in the outbound path but not the inbound path.
[Akhil]Correct I missed out the inbound path.
>>
>> Also, in the inbound path you have to consider a possible ECN value update.
[Akhil]If I take care of the ECN then it would mean I need to calculate
the checksum completely, incremental checksum wont give correct results.
This would surely impact performance. Any suggestion on how should we
take care of ECN update. Should I recalculate the checksum and send the
patch for ECN update? Or do we have a better solution.
>
> Any further comments here, Akhil?
>
> Thanks,
> Pablo
>
[Akhil] Sorry I missed out the previous reply from Sergio.
Thanks,
Akhil
>>
>> Sergio
>>
>>
>> On 26/09/2016 17:32, akhil.goyal at nxp.com wrote:
>>> From: Akhil Goyal
>>>
>>> In IPsec-secgw application when TTL is decremented in IP header
>>> before forwarding the packet, checksum needs to be updated.
>>>
>>> In this patch an incremental checksum is added.
>>> Other applications(like l3fwd) are also doing so.
>>>
>>> Signed-off-by: Akhil Goyal
>>> ---
>>> examples/ipsec-secgw/ipip.h | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/examples/ipsec-secgw/ipip.h b/examples/ipsec-secgw/ipip.h
>>> index ff1dccd..ef059a9 100644
>>> --- a/examples/ipsec-secgw/ipip.h
>>> +++ b/examples/ipsec-secgw/ipip.h
>>> @@ -56,6 +56,7 @@ ipip_outbound(struct rte_mbuf *m, uint32_t offset,
>> uint32_t is_ipv6,
>>> if (inip4->ip_v == IPVERSION) {
>>> /* XXX This should be done by the forwarding engine instead
>> */
>>> inip4->ip_ttl -= 1;
>>> + inip4->ip_sum += 1;
>>> ds_ecn = inip4->ip_tos;
>>> } else {
>>> inip6 = (struct ip6_hdr *)inip4;
>>
>>
>
>
[dpdk-dev] [PATCH 1/4] libcrypto_pmd: initial implementation of SW crypto device
On 8/26/2016 12:51 PM, Piotr Azarewicz wrote:
> +/** Provide session for operation */
> +static struct libcrypto_session *
> +get_session(struct libcrypto_qp *qp, struct rte_crypto_op *op)
> +{
> + struct libcrypto_session *sess = NULL;
> +
> + if (op->sym->sess_type == RTE_CRYPTO_SYM_OP_WITH_SESSION) {
> + /* get existing session */
> + if (!unlikely(op->sym->session == NULL ||
> + op->sym->session->dev_type !=
> + RTE_CRYPTODEV_LIBCRYPTO_PMD))
> + sess = (struct libcrypto_session *)
> + op->sym->session->_private;
> + } else {
> + /* provide internal session */
> + void *_sess = NULL;
> +
> + if (!rte_mempool_get(qp->sess_mp, (void **)&_sess)) {
> + sess = (struct libcrypto_session *)
> + ((struct rte_cryptodev_sym_session *)_sess)
> + ->_private;
> +
> + if (unlikely(libcrypto_set_session_parameters(
> + sess, op->sym->xform) != 0)) {
> + rte_mempool_put(qp->sess_mp, _sess);
> + sess = NULL;
> + } else
> + op->sym->session = _sess;
> + }
> + }
> +
> + if (sess == NULL)
> + op->status = RTE_CRYPTO_OP_STATUS_INVALID_SESSION;
> +
> + return sess;
> +}
> +
> +/*
> +
> *--
> + * Process Operations
> +
> *--
> + */
> +
> +/** Process standard libcrypto cipher encryption */
> +static int
> +process_libcrypto_cipher_encrypt(uint8_t *src, uint8_t *dst,
> + uint8_t *iv, uint8_t *key, int srclen,
> + EVP_CIPHER_CTX *ctx, const EVP_CIPHER *algo)
> +{
> + int dstlen, totlen;
> +
> + if (EVP_EncryptInit_ex(ctx, algo, NULL, key, iv) <= 0)
> + goto process_cipher_encrypt_err;
[Akhil] this EVP_EncryptInit_ex() can be done for each session instead
of each packet. This will improve the performance. Also if there is some
change in the parameters later then it can be called again here with the
updated parameters only.
Same comment is for all cases (hmac, auth, etc)
> +
> + if (EVP_EncryptUpdate(ctx, dst, , src, srclen) <= 0)
> + goto process_cipher_encrypt_err;
> +
> + if (EVP_EncryptFinal_ex(ctx, dst + dstlen, ) <= 0)
> + goto process_cipher_encrypt_err;
> +
> + return 0;
> +
> +process_cipher_encrypt_err:
> + LIBCRYPTO_LOG_ERR("Process libcrypto cipher encrypt failed");
> + return -EINVAL;
> +}
> +
> +/** Process standard libcrypto cipher decryption */
> +static int
> +process_libcrypto_cipher_decrypt(uint8_t *src, uint8_t *dst,
> + uint8_t *iv, uint8_t *key, int srclen,
> + EVP_CIPHER_CTX *ctx, const EVP_CIPHER *algo)
> +{
> + int dstlen, totlen;
> +
> + if (EVP_DecryptInit_ex(ctx, algo, NULL, key, iv) <= 0)
> + goto process_cipher_decrypt_err;
> +
> + if (EVP_CIPHER_CTX_set_padding(ctx, 0) <= 0)
> + goto process_cipher_decrypt_err;
> +
> + if (EVP_DecryptUpdate(ctx, dst, , src, srclen) <= 0)
> + goto process_cipher_decrypt_err;
> +
> + if (EVP_DecryptFinal_ex(ctx, dst + dstlen, ) <= 0)
> + goto process_cipher_decrypt_err;
> +
> + return 0;
> +
> +process_cipher_decrypt_err:
> + LIBCRYPTO_LOG_ERR("Process libcrypto cipher decrypt failed");
> + return -EINVAL;
> +}
> +
> +/** Process cipher des 3 ctr encryption, decryption algorithm */
> +static int
> +process_libcrypto_cipher_des3ctr(uint8_t *src, uint8_t *dst,
> + uint8_t *iv, uint8_t *key, int srclen, EVP_CIPHER_CTX *ctx)
> +{
> + uint8_t ebuf[8], ctr[8];
> + int unused, n;
> +
> + /* We use 3DES encryption also for decryption.
> + * IV is not important for 3DES ecb
> + */
> + if (EVP_EncryptInit_ex(ctx, EVP_des_ede3_ecb(), NULL, key, NULL) <= 0)
> + goto process_cipher_des3ctr_err;
> +
> + memcpy(ctr, iv, 8);
> + n = 0;
> +
> + while (n < srclen) {
> + if (n % 8 == 0) {
> + if (EVP_EncryptUpdate(ctx, (unsigned char *),
> ,
> + (const unsigned char *), 8) <= 0)
> + goto process_cipher_des3ctr_err;
> + ctr_inc(ctr);
> + }
> + dst[n] = src[n] ^ ebuf[n % 8];
> + n++;
> + }
> +
> + return 0;
> +
> +process_cipher_des3ctr_err:
> + LIBCRYPTO_LOG_ERR("Process libcrypto cipher des 3 ede ctr failed");
> + return -EINVAL;
> +}
> +
> +/** Process auth gmac algorithm */
> +static int
> +process_libcrypto_auth_gmac(uint8_t *src, uint8_t *dst,
> + uint8_t *iv,
[dpdk-dev] ip_chksum not updated in ipsec-secgw application
On 7/18/2016 6:50 PM, Thomas Monjalon wrote: > 2016-07-18 13:57, Sergio Gonzalez Monroy: >> On 18/07/2016 13:41, Akhil Goyal wrote: >>> In Ipsec-secgw application, while adding the outer IP header, >>> it seems that the application does not update the checksum value >>> for outbound packets. This result in incorrect ip->checksum in >>> the encrypted packet. > [...] >> >> It is intentional. The application is using IP checksum offload > > The correct behaviour is to have a software fallback (using rte_ip.h) > for drivers which do not support checksum offload. > But given it is just an example, it is normal to have this kind of > constraint. However I think it should be explained in its doc. > And a list of tested NICs would be nice to have. > Agreed. The driver that I was using did not enable checksum offload. It is good to have a fallback option.
[dpdk-dev] ip_chksum not updated in ipsec-secgw application
On 7/18/2016 6:27 PM, Sergio Gonzalez Monroy wrote: > Hi, > > On 18/07/2016 13:41, Akhil Goyal wrote: >> Hi, >> >> In Ipsec-secgw application, while adding the outer IP header, it seems >> that the application does not update the checksum value for outbound >> packets. This result in incorrect ip->checksum in the encrypted packet. >> >> Please let me know if the checksum value is updated somewhere else or >> not. >> >> Also In case of inner ip header also the TTL value is decremented by >> one but the checksum value is not updated. Is it intentional or it is >> done somewhere else? > > It is intentional. The application is using IP checksum offload but just > looking now at the code there is a bug for IPv6 packets where the flag > does not get setup. > Is it only for IPv6 traffic that you are having this issue? > > For IPv4 traffic the PKT_TX_IP_CKSUM flag is setup in 'prepare_tx_pkt' > function in ipsec-secgw.c > > Sergio > Thanks Sergio, got your point. I missed the flag. I was using it for IPv4. Akhil
[dpdk-dev] ip_chksum not updated in ipsec-secgw application
Hi,
In Ipsec-secgw application, while adding the outer IP header, it seems that the
application does not update the checksum value for outbound packets. This
result in incorrect ip->checksum in the encrypted packet.
Please let me know if the checksum value is updated somewhere else or not.
Also In case of inner ip header also the TTL value is decremented by one but
the checksum value is not updated. Is it intentional or it is done somewhere
else?
After addition of following code, the checksum looks good and the encrypted
packets are good.
diff --git a/examples/ipsec-secgw/ipip.h b/examples/ipsec-secgw/ipip.h
index 322076c..0f7b60f 100644
--- a/examples/ipsec-secgw/ipip.h
+++ b/examples/ipsec-secgw/ipip.h
@@ -41,6 +41,24 @@
#include
#define IPV6_VERSION (6)
+static inline uint16_t
+ip_sum(const unaligned_uint16_t *hdr, int hdr_len)
+{
+ uint32_t sum = 0;
+
+ while (hdr_len > 1)
+ {
+ sum += *hdr++;
+ if (sum & 0x8000)
+ sum = (sum & 0x) + (sum >> 16);
+ hdr_len -= 2;
+ }
+
+ while (sum >> 16)
+ sum = (sum & 0x) + (sum >> 16);
+
+ return ~sum;
+}
static inline struct ip *
ip4ip_outbound(struct rte_mbuf *m, uint32_t offset, uint32_t src, uint32_t dst)
@@ -71,7 +89,8 @@ ip4ip_outbound(struct rte_mbuf *m, uint32_t offset, uint32_t
src, uint32_t dst)
outip->ip_src.s_addr = src;
outip->ip_dst.s_addr = dst;
-
+ outip->ip_sum = 0;
+ outip->ip_sum = ip_sum((const unaligned_uint16_t *)outip, sizeof(struct
ip));
return outip;
}
Regards,
Akhil
