[dpdk-stable] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number

[dpdk-stable] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number

[Yongseok Koh]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number

[Ananyev, Konstantin]

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

Re: [dpdk-stable] [dpdk-dev] [PATCH] examples/ipsec-secgw: fix coherency of ESP sequence number

[Yongseok Koh]

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&data=02%7C01%7Cyskoh%40mellanox.com%7C92fcdccccff446f9f93708d6bc0ec532%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636903168953772804&sdata=8l1b79cFf7Jodlk%2Bup3b8uRUUSU2IyglmFxhmXqsbCI%3D&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