DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number
@ 2019-04-06  0:35 Yongseok Koh
  2019-04-06  0:35 ` Yongseok Koh
  2019-04-08 10:41 ` Ananyev, Konstantin
  0 siblings, 2 replies; 6+ messages in thread
From: Yongseok Koh @ 2019-04-06  0:35 UTC (permalink / raw)
  To: radu.nicolau, akhil.goyal; +Cc: dev, sergio.gonzalez.monroy, aviadye, stable

Outbound ESP sequence number should be incremented atomically and refeenced
indirectly. Otherwise, concurrent access by multiple threads will break
coherency.

Fixes: d299106e8e31 ("examples/ipsec-secgw: add IPsec sample application")
Fixes: 906257e965b7 ("examples/ipsec-secgw: support IPv6")
Fixes: cef50fc6f1e2 ("examples/ipsec-secgw: change CBC IV generation")
Fixes: 2a41fb7c6525 ("examples/ipsec-secgw: convert IV to big endian")
Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
Cc: sergio.gonzalez.monroy@intel.com
Cc: aviadye@mellanox.com
Cc: akhil.goyal@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---
 examples/ipsec-secgw/esp.c   | 13 +++++++------
 examples/ipsec-secgw/ipsec.h |  2 +-
 examples/ipsec-secgw/sa.c    |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
index faa84ddd13..6f616f3f69 100644
--- a/examples/ipsec-secgw/esp.c
+++ b/examples/ipsec-secgw/esp.c
@@ -222,6 +222,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 	struct rte_crypto_sym_op *sym_cop;
 	int32_t i;
 	uint16_t pad_payload_len, pad_len, ip_hdr_len;
+	uint64_t sa_seq;
 
 	RTE_ASSERT(m != NULL);
 	RTE_ASSERT(sa != NULL);
@@ -316,14 +317,14 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 		}
 	}
 
-	sa->seq++;
 	esp->spi = rte_cpu_to_be_32(sa->spi);
-	esp->seq = rte_cpu_to_be_32((uint32_t)sa->seq);
+	sa_seq = rte_atomic64_add_return(&sa->seq, 1);
+	esp->seq = rte_cpu_to_be_32((uint32_t)sa_seq);
 
 	/* set iv */
 	uint64_t *iv = (uint64_t *)(esp + 1);
 	if (sa->aead_algo == RTE_CRYPTO_AEAD_AES_GCM) {
-		*iv = rte_cpu_to_be_64(sa->seq);
+		*iv = rte_cpu_to_be_64(sa_seq);
 	} else {
 		switch (sa->cipher_algo) {
 		case RTE_CRYPTO_CIPHER_NULL:
@@ -332,7 +333,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 			memset(iv, 0, sa->iv_len);
 			break;
 		case RTE_CRYPTO_CIPHER_AES_CTR:
-			*iv = rte_cpu_to_be_64(sa->seq);
+			*iv = rte_cpu_to_be_64(sa_seq);
 			break;
 		default:
 			RTE_LOG(ERR, IPSEC_ESP,
@@ -373,7 +374,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 
 		struct cnt_blk *icb = get_cnt_blk(m);
 		icb->salt = sa->salt;
-		icb->iv = rte_cpu_to_be_64(sa->seq);
+		icb->iv = rte_cpu_to_be_64(sa_seq);
 		icb->cnt = rte_cpu_to_be_32(1);
 
 		aad = get_aad(m);
@@ -414,7 +415,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 
 		struct cnt_blk *icb = get_cnt_blk(m);
 		icb->salt = sa->salt;
-		icb->iv = rte_cpu_to_be_64(sa->seq);
+		icb->iv = rte_cpu_to_be_64(sa_seq);
 		icb->cnt = rte_cpu_to_be_32(1);
 
 		switch (sa->auth_algo) {
diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
index 99f49d65f8..742d09aa36 100644
--- a/examples/ipsec-secgw/ipsec.h
+++ b/examples/ipsec-secgw/ipsec.h
@@ -87,7 +87,7 @@ struct ipsec_sa {
 	struct rte_ipsec_session ips; /* one session per sa for now */
 	uint32_t spi;
 	uint32_t cdev_id_qp;
-	uint64_t seq;
+	rte_atomic64_t seq;
 	uint32_t salt;
 	union {
 		struct rte_cryptodev_sym_session *crypto_session;
diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
index a7298a30c2..adf6ac3f9a 100644
--- a/examples/ipsec-secgw/sa.c
+++ b/examples/ipsec-secgw/sa.c
@@ -795,7 +795,7 @@ sa_add_rules(struct sa_ctx *sa_ctx, const struct ipsec_sa entries[],
 			return -EINVAL;
 		}
 		*sa = entries[i];
-		sa->seq = 0;
+		rte_atomic64_set(&sa->seq, -1);
 
 		if (sa->type == RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL ||
 			sa->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO) {
-- 
2.11.0

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number
  2019-04-06  0:35 [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number Yongseok Koh
@ 2019-04-06  0:35 ` Yongseok Koh
  2019-04-08 10:41 ` Ananyev, Konstantin
  1 sibling, 0 replies; 6+ messages in thread
From: Yongseok Koh @ 2019-04-06  0:35 UTC (permalink / raw)
  To: radu.nicolau, akhil.goyal; +Cc: dev, sergio.gonzalez.monroy, aviadye, stable

Outbound ESP sequence number should be incremented atomically and refeenced
indirectly. Otherwise, concurrent access by multiple threads will break
coherency.

Fixes: d299106e8e31 ("examples/ipsec-secgw: add IPsec sample application")
Fixes: 906257e965b7 ("examples/ipsec-secgw: support IPv6")
Fixes: cef50fc6f1e2 ("examples/ipsec-secgw: change CBC IV generation")
Fixes: 2a41fb7c6525 ("examples/ipsec-secgw: convert IV to big endian")
Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
Cc: sergio.gonzalez.monroy@intel.com
Cc: aviadye@mellanox.com
Cc: akhil.goyal@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---
 examples/ipsec-secgw/esp.c   | 13 +++++++------
 examples/ipsec-secgw/ipsec.h |  2 +-
 examples/ipsec-secgw/sa.c    |  2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
index faa84ddd13..6f616f3f69 100644
--- a/examples/ipsec-secgw/esp.c
+++ b/examples/ipsec-secgw/esp.c
@@ -222,6 +222,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 	struct rte_crypto_sym_op *sym_cop;
 	int32_t i;
 	uint16_t pad_payload_len, pad_len, ip_hdr_len;
+	uint64_t sa_seq;
 
 	RTE_ASSERT(m != NULL);
 	RTE_ASSERT(sa != NULL);
@@ -316,14 +317,14 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 		}
 	}
 
-	sa->seq++;
 	esp->spi = rte_cpu_to_be_32(sa->spi);
-	esp->seq = rte_cpu_to_be_32((uint32_t)sa->seq);
+	sa_seq = rte_atomic64_add_return(&sa->seq, 1);
+	esp->seq = rte_cpu_to_be_32((uint32_t)sa_seq);
 
 	/* set iv */
 	uint64_t *iv = (uint64_t *)(esp + 1);
 	if (sa->aead_algo == RTE_CRYPTO_AEAD_AES_GCM) {
-		*iv = rte_cpu_to_be_64(sa->seq);
+		*iv = rte_cpu_to_be_64(sa_seq);
 	} else {
 		switch (sa->cipher_algo) {
 		case RTE_CRYPTO_CIPHER_NULL:
@@ -332,7 +333,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 			memset(iv, 0, sa->iv_len);
 			break;
 		case RTE_CRYPTO_CIPHER_AES_CTR:
-			*iv = rte_cpu_to_be_64(sa->seq);
+			*iv = rte_cpu_to_be_64(sa_seq);
 			break;
 		default:
 			RTE_LOG(ERR, IPSEC_ESP,
@@ -373,7 +374,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 
 		struct cnt_blk *icb = get_cnt_blk(m);
 		icb->salt = sa->salt;
-		icb->iv = rte_cpu_to_be_64(sa->seq);
+		icb->iv = rte_cpu_to_be_64(sa_seq);
 		icb->cnt = rte_cpu_to_be_32(1);
 
 		aad = get_aad(m);
@@ -414,7 +415,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 
 		struct cnt_blk *icb = get_cnt_blk(m);
 		icb->salt = sa->salt;
-		icb->iv = rte_cpu_to_be_64(sa->seq);
+		icb->iv = rte_cpu_to_be_64(sa_seq);
 		icb->cnt = rte_cpu_to_be_32(1);
 
 		switch (sa->auth_algo) {
diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
index 99f49d65f8..742d09aa36 100644
--- a/examples/ipsec-secgw/ipsec.h
+++ b/examples/ipsec-secgw/ipsec.h
@@ -87,7 +87,7 @@ struct ipsec_sa {
 	struct rte_ipsec_session ips; /* one session per sa for now */
 	uint32_t spi;
 	uint32_t cdev_id_qp;
-	uint64_t seq;
+	rte_atomic64_t seq;
 	uint32_t salt;
 	union {
 		struct rte_cryptodev_sym_session *crypto_session;
diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
index a7298a30c2..adf6ac3f9a 100644
--- a/examples/ipsec-secgw/sa.c
+++ b/examples/ipsec-secgw/sa.c
@@ -795,7 +795,7 @@ sa_add_rules(struct sa_ctx *sa_ctx, const struct ipsec_sa entries[],
 			return -EINVAL;
 		}
 		*sa = entries[i];
-		sa->seq = 0;
+		rte_atomic64_set(&sa->seq, -1);
 
 		if (sa->type == RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL ||
 			sa->type == RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO) {
-- 
2.11.0


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number
  2019-04-06  0:35 [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number Yongseok Koh
  2019-04-06  0:35 ` Yongseok Koh
@ 2019-04-08 10:41 ` Ananyev, Konstantin
  2019-04-08 10:41   ` Ananyev, Konstantin
  2019-04-08 19:36   ` Yongseok Koh
  1 sibling, 2 replies; 6+ messages in thread
From: Ananyev, Konstantin @ 2019-04-08 10:41 UTC (permalink / raw)
  To: Yongseok Koh, Nicolau, Radu, akhil.goyal; +Cc: dev, aviadye, stable


Hi Yongseok,

> Outbound ESP sequence number should be incremented atomically and refeenced
> indirectly. Otherwise, concurrent access by multiple threads will break
> coherency.

I think MT mode per SA is not supported right now by ipsec-secgw.
>From https://doc.dpdk.org/guides/sample_app_ug/ipsec_secgw.html:
49.2. Constraints
...
Each SA must be handle by a unique lcore (1 RX queue per port).

Also the changes you proposed wouldn't handle inbound case.
Anyway, if you still like to have atomic sqn updates for outbound,
then probably better to make it configurable
(otherwise it would introduce unnecessary slowdown).
There is '-a' (atomic) command-line option.
Right now it works for librte_ipsec mode only, but I suppose it wouldn't
be big deal to make legacy mode to take it into account too.

> 
> Fixes: d299106e8e31 ("examples/ipsec-secgw: add IPsec sample application")
> Fixes: 906257e965b7 ("examples/ipsec-secgw: support IPv6")
> Fixes: cef50fc6f1e2 ("examples/ipsec-secgw: change CBC IV generation")
> Fixes: 2a41fb7c6525 ("examples/ipsec-secgw: convert IV to big endian")
> Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
> Cc: sergio.gonzalez.monroy@intel.com
> Cc: aviadye@mellanox.com
> Cc: akhil.goyal@nxp.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>  examples/ipsec-secgw/esp.c   | 13 +++++++------
>  examples/ipsec-secgw/ipsec.h |  2 +-
>  examples/ipsec-secgw/sa.c    |  2 +-
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
> index faa84ddd13..6f616f3f69 100644
> --- a/examples/ipsec-secgw/esp.c
> +++ b/examples/ipsec-secgw/esp.c
> @@ -222,6 +222,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
>  	struct rte_crypto_sym_op *sym_cop;
>  	int32_t i;
>  	uint16_t pad_payload_len, pad_len, ip_hdr_len;
> +	uint64_t sa_seq;
> 
>  	RTE_ASSERT(m != NULL);
>  	RTE_ASSERT(sa != NULL);
> @@ -316,14 +317,14 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
>  		}
>  	}
> 
> -	sa->seq++;
>  	esp->spi = rte_cpu_to_be_32(sa->spi);
> -	esp->seq = rte_cpu_to_be_32((uint32_t)sa->seq);
> +	sa_seq = rte_atomic64_add_return(&sa->seq, 1);
> +	esp->seq = rte_cpu_to_be_32((uint32_t)sa_seq);
> 
>  	/* set iv */
>  	uint64_t *iv = (uint64_t *)(esp + 1);
>  	if (sa->aead_algo == RTE_CRYPTO_AEAD_AES_GCM) {
> -		*iv = rte_cpu_to_be_64(sa->seq);
> +		*iv = rte_cpu_to_be_64(sa_seq);
>  	} else {
>  		switch (sa->cipher_algo) {
>  		case RTE_CRYPTO_CIPHER_NULL:
> @@ -332,7 +333,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
>  			memset(iv, 0, sa->iv_len);
>  			break;
>  		case RTE_CRYPTO_CIPHER_AES_CTR:
> -			*iv = rte_cpu_to_be_64(sa->seq);
> +			*iv = rte_cpu_to_be_64(sa_seq);
>  			break;
>  		default:
>  			RTE_LOG(ERR, IPSEC_ESP,
> @@ -373,7 +374,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
> 
>  		struct cnt_blk *icb = get_cnt_blk(m);
>  		icb->salt = sa->salt;
> -		icb->iv = rte_cpu_to_be_64(sa->seq);
> +		icb->iv = rte_cpu_to_be_64(sa_seq);
>  		icb->cnt = rte_cpu_to_be_32(1);
> 
>  		aad = get_aad(m);
> @@ -414,7 +415,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
> 
>  		struct cnt_blk *icb = get_cnt_blk(m);
>  		icb->salt = sa->salt;
> -		icb->iv = rte_cpu_to_be_64(sa->seq);
> +		icb->iv = rte_cpu_to_be_64(sa_seq);
>  		icb->cnt = rte_cpu_to_be_32(1);
> 
>  		switch (sa->auth_algo) {
> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> index 99f49d65f8..742d09aa36 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -87,7 +87,7 @@ struct ipsec_sa {
>  	struct rte_ipsec_session ips; /* one session per sa for now */
>  	uint32_t spi;
>  	uint32_t cdev_id_qp;
> -	uint64_t seq;
> +	rte_atomic64_t seq;
>  	uint32_t salt;
>  	union {
>  		struct rte_cryptodev_sym_session *crypto_session;
> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> index a7298a30c2..adf6ac3f9a 100644
> --- a/examples/ipsec-secgw/sa.c
> +++ b/examples/ipsec-secgw/sa.c
> @@ -795,7 +795,7 @@ sa_add_rules(struct sa_ctx *sa_ctx, const struct ipsec_sa entries[],
>  			return -EINVAL;
>  		}
>  		*sa = entries[i];
> -		sa->seq = 0;
> +		rte_atomic64_set(&sa->seq, -1);

I think initial value should remain zero.
Konstantin

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number
  2019-04-08 10:41 ` Ananyev, Konstantin
@ 2019-04-08 10:41   ` Ananyev, Konstantin
  2019-04-08 19:36   ` Yongseok Koh
  1 sibling, 0 replies; 6+ messages in thread
From: Ananyev, Konstantin @ 2019-04-08 10:41 UTC (permalink / raw)
  To: Yongseok Koh, Nicolau, Radu, akhil.goyal; +Cc: dev, aviadye, stable


Hi Yongseok,

> Outbound ESP sequence number should be incremented atomically and refeenced
> indirectly. Otherwise, concurrent access by multiple threads will break
> coherency.

I think MT mode per SA is not supported right now by ipsec-secgw.
From https://doc.dpdk.org/guides/sample_app_ug/ipsec_secgw.html:
49.2. Constraints
...
Each SA must be handle by a unique lcore (1 RX queue per port).

Also the changes you proposed wouldn't handle inbound case.
Anyway, if you still like to have atomic sqn updates for outbound,
then probably better to make it configurable
(otherwise it would introduce unnecessary slowdown).
There is '-a' (atomic) command-line option.
Right now it works for librte_ipsec mode only, but I suppose it wouldn't
be big deal to make legacy mode to take it into account too.

> 
> Fixes: d299106e8e31 ("examples/ipsec-secgw: add IPsec sample application")
> Fixes: 906257e965b7 ("examples/ipsec-secgw: support IPv6")
> Fixes: cef50fc6f1e2 ("examples/ipsec-secgw: change CBC IV generation")
> Fixes: 2a41fb7c6525 ("examples/ipsec-secgw: convert IV to big endian")
> Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
> Cc: sergio.gonzalez.monroy@intel.com
> Cc: aviadye@mellanox.com
> Cc: akhil.goyal@nxp.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>  examples/ipsec-secgw/esp.c   | 13 +++++++------
>  examples/ipsec-secgw/ipsec.h |  2 +-
>  examples/ipsec-secgw/sa.c    |  2 +-
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
> index faa84ddd13..6f616f3f69 100644
> --- a/examples/ipsec-secgw/esp.c
> +++ b/examples/ipsec-secgw/esp.c
> @@ -222,6 +222,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
>  	struct rte_crypto_sym_op *sym_cop;
>  	int32_t i;
>  	uint16_t pad_payload_len, pad_len, ip_hdr_len;
> +	uint64_t sa_seq;
> 
>  	RTE_ASSERT(m != NULL);
>  	RTE_ASSERT(sa != NULL);
> @@ -316,14 +317,14 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
>  		}
>  	}
> 
> -	sa->seq++;
>  	esp->spi = rte_cpu_to_be_32(sa->spi);
> -	esp->seq = rte_cpu_to_be_32((uint32_t)sa->seq);
> +	sa_seq = rte_atomic64_add_return(&sa->seq, 1);
> +	esp->seq = rte_cpu_to_be_32((uint32_t)sa_seq);
> 
>  	/* set iv */
>  	uint64_t *iv = (uint64_t *)(esp + 1);
>  	if (sa->aead_algo == RTE_CRYPTO_AEAD_AES_GCM) {
> -		*iv = rte_cpu_to_be_64(sa->seq);
> +		*iv = rte_cpu_to_be_64(sa_seq);
>  	} else {
>  		switch (sa->cipher_algo) {
>  		case RTE_CRYPTO_CIPHER_NULL:
> @@ -332,7 +333,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
>  			memset(iv, 0, sa->iv_len);
>  			break;
>  		case RTE_CRYPTO_CIPHER_AES_CTR:
> -			*iv = rte_cpu_to_be_64(sa->seq);
> +			*iv = rte_cpu_to_be_64(sa_seq);
>  			break;
>  		default:
>  			RTE_LOG(ERR, IPSEC_ESP,
> @@ -373,7 +374,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
> 
>  		struct cnt_blk *icb = get_cnt_blk(m);
>  		icb->salt = sa->salt;
> -		icb->iv = rte_cpu_to_be_64(sa->seq);
> +		icb->iv = rte_cpu_to_be_64(sa_seq);
>  		icb->cnt = rte_cpu_to_be_32(1);
> 
>  		aad = get_aad(m);
> @@ -414,7 +415,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
> 
>  		struct cnt_blk *icb = get_cnt_blk(m);
>  		icb->salt = sa->salt;
> -		icb->iv = rte_cpu_to_be_64(sa->seq);
> +		icb->iv = rte_cpu_to_be_64(sa_seq);
>  		icb->cnt = rte_cpu_to_be_32(1);
> 
>  		switch (sa->auth_algo) {
> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> index 99f49d65f8..742d09aa36 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -87,7 +87,7 @@ struct ipsec_sa {
>  	struct rte_ipsec_session ips; /* one session per sa for now */
>  	uint32_t spi;
>  	uint32_t cdev_id_qp;
> -	uint64_t seq;
> +	rte_atomic64_t seq;
>  	uint32_t salt;
>  	union {
>  		struct rte_cryptodev_sym_session *crypto_session;
> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> index a7298a30c2..adf6ac3f9a 100644
> --- a/examples/ipsec-secgw/sa.c
> +++ b/examples/ipsec-secgw/sa.c
> @@ -795,7 +795,7 @@ sa_add_rules(struct sa_ctx *sa_ctx, const struct ipsec_sa entries[],
>  			return -EINVAL;
>  		}
>  		*sa = entries[i];
> -		sa->seq = 0;
> +		rte_atomic64_set(&sa->seq, -1);

I think initial value should remain zero.
Konstantin

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number
  2019-04-08 10:41 ` Ananyev, Konstantin
  2019-04-08 10:41   ` Ananyev, Konstantin
@ 2019-04-08 19:36   ` Yongseok Koh
  2019-04-08 19:36     ` Yongseok Koh
  1 sibling, 1 reply; 6+ messages in thread
From: Yongseok Koh @ 2019-04-08 19:36 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: Nicolau, Radu, akhil.goyal, dev, Aviad Yehezkel, stable

On Mon, Apr 08, 2019 at 10:41:29AM +0000, Ananyev, Konstantin wrote:
> 
> Hi Yongseok,
> 
> > Outbound ESP sequence number should be incremented atomically and refeenced
> > indirectly. Otherwise, concurrent access by multiple threads will break
> > coherency.
> 
> I think MT mode per SA is not supported right now by ipsec-secgw.
> From https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fsample_app_ug%2Fipsec_secgw.html&amp;data=02%7C01%7Cyskoh%40mellanox.com%7C92fcdccccff446f9f93708d6bc0ec532%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636903168953772804&amp;sdata=8l1b79cFf7Jodlk%2Bup3b8uRUUSU2IyglmFxhmXqsbCI%3D&amp;reserved=0:
> 49.2. Constraints
> ...
> Each SA must be handle by a unique lcore (1 RX queue per port).

I wasn't aware of this limitation. If it is already documented, I'll take back
my patch. Actually, I have done some performance testing with this example and
it worked fine with single SA in each direction. That's why I thought it was
supported.

> Also the changes you proposed wouldn't handle inbound case.

No seq number check for inbound direction in this example code..

> Anyway, if you still like to have atomic sqn updates for outbound,
> then probably better to make it configurable
> (otherwise it would introduce unnecessary slowdown).
> There is '-a' (atomic) command-line option.
> Right now it works for librte_ipsec mode only, but I suppose it wouldn't
> be big deal to make legacy mode to take it into account too.
> 
[...]
> > diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> > index a7298a30c2..adf6ac3f9a 100644
> > --- a/examples/ipsec-secgw/sa.c
> > +++ b/examples/ipsec-secgw/sa.c
> > @@ -795,7 +795,7 @@ sa_add_rules(struct sa_ctx *sa_ctx, const struct ipsec_sa entries[],
> >  			return -EINVAL;
> >  		}
> >  		*sa = entries[i];
> > -		sa->seq = 0;
> > +		rte_atomic64_set(&sa->seq, -1);
> 
> I think initial value should remain zero.

The reason was because there's no rte_atomic64_return_add() but
rte_atomic64_add_return(). Would be better to have it with
__sync_fetch_and_add() later. Anyway, I'm taking back this patch like I
mentioned.

Thanks,
Yongseok

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number
  2019-04-08 19:36   ` Yongseok Koh
@ 2019-04-08 19:36     ` Yongseok Koh
  0 siblings, 0 replies; 6+ messages in thread
From: Yongseok Koh @ 2019-04-08 19:36 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: Nicolau, Radu, akhil.goyal, dev, Aviad Yehezkel, stable

On Mon, Apr 08, 2019 at 10:41:29AM +0000, Ananyev, Konstantin wrote:
> 
> Hi Yongseok,
> 
> > Outbound ESP sequence number should be incremented atomically and refeenced
> > indirectly. Otherwise, concurrent access by multiple threads will break
> > coherency.
> 
> I think MT mode per SA is not supported right now by ipsec-secgw.
> From https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fsample_app_ug%2Fipsec_secgw.html&amp;data=02%7C01%7Cyskoh%40mellanox.com%7C92fcdccccff446f9f93708d6bc0ec532%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636903168953772804&amp;sdata=8l1b79cFf7Jodlk%2Bup3b8uRUUSU2IyglmFxhmXqsbCI%3D&amp;reserved=0:
> 49.2. Constraints
> ...
> Each SA must be handle by a unique lcore (1 RX queue per port).

I wasn't aware of this limitation. If it is already documented, I'll take back
my patch. Actually, I have done some performance testing with this example and
it worked fine with single SA in each direction. That's why I thought it was
supported.

> Also the changes you proposed wouldn't handle inbound case.

No seq number check for inbound direction in this example code..

> Anyway, if you still like to have atomic sqn updates for outbound,
> then probably better to make it configurable
> (otherwise it would introduce unnecessary slowdown).
> There is '-a' (atomic) command-line option.
> Right now it works for librte_ipsec mode only, but I suppose it wouldn't
> be big deal to make legacy mode to take it into account too.
> 
[...]
> > diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> > index a7298a30c2..adf6ac3f9a 100644
> > --- a/examples/ipsec-secgw/sa.c
> > +++ b/examples/ipsec-secgw/sa.c
> > @@ -795,7 +795,7 @@ sa_add_rules(struct sa_ctx *sa_ctx, const struct ipsec_sa entries[],
> >  			return -EINVAL;
> >  		}
> >  		*sa = entries[i];
> > -		sa->seq = 0;
> > +		rte_atomic64_set(&sa->seq, -1);
> 
> I think initial value should remain zero.

The reason was because there's no rte_atomic64_return_add() but
rte_atomic64_add_return(). Would be better to have it with
__sync_fetch_and_add() later. Anyway, I'm taking back this patch like I
mentioned.

Thanks,
Yongseok

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-08 19:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-06  0:35 [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number Yongseok Koh
2019-04-06  0:35 ` Yongseok Koh
2019-04-08 10:41 ` Ananyev, Konstantin
2019-04-08 10:41   ` Ananyev, Konstantin
2019-04-08 19:36   ` Yongseok Koh
2019-04-08 19:36     ` Yongseok Koh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).