[PATCH 0/2] fix IPSec endianness problem

[PATCH 0/2] fix IPSec endianness problem

[Chaoyong He]

This patch series aims to fix the IPSec problem related with SA
salt endianness.

Shihong Wang (2):
  examples/ipsec-secgw: fix SA salt endianness problem
  net/nfp: fix data endianness problem

 drivers/net/nfp/nfp_ipsec.c | 72 +++++++++++++++++++++++--------------
 drivers/net/nfp/nfp_ipsec.h |  9 ++---
 examples/ipsec-secgw/sa.c   | 10 ++++--
 3 files changed, 54 insertions(+), 37 deletions(-)

-- 
2.39.1

[PATCH 1/2] examples/ipsec-secgw: fix SA salt endianness problem

[Chaoyong He]

From: Shihong Wang <shihong.wang@corigine.com>

The SA salt of struct ipsec_sa is a CPU-endian u32 variable, but it’s
value is stored in an array of encryption or authentication keys
according to big-endian. So it maybe need to convert the endianness
order to ensure that the value assigned to the SA salt is CPU-endian.

Fixes: 50d75cae2a2c ("examples/ipsec-secgw: initialize SA salt")
Fixes: 9413c3901f31 ("examples/ipsec-secgw: support additional algorithms")
Fixes: 501e9c226adf ("examples/ipsec-secgw: add AEAD parameters")
Cc: stable@dpdk.org

Signed-off-by: Shihong Wang <shihong.wang@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 examples/ipsec-secgw/sa.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
index c4bac17cd7..4018b0558a 100644
--- a/examples/ipsec-secgw/sa.c
+++ b/examples/ipsec-secgw/sa.c
@@ -374,6 +374,7 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
 	uint32_t ti; /*token index*/
 	uint32_t *ri /*rule index*/;
 	struct ipsec_sa_cnt *sa_cnt;
+	rte_be32_t salt; /*big-endian salt*/
 	uint32_t cipher_algo_p = 0;
 	uint32_t auth_algo_p = 0;
 	uint32_t aead_algo_p = 0;
@@ -508,8 +509,9 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
 			if (algo->algo == RTE_CRYPTO_CIPHER_AES_CTR) {
 				key_len -= 4;
 				rule->cipher_key_len = key_len;
-				memcpy(&rule->salt,
+				memcpy(&salt,
 					&rule->cipher_key[key_len], 4);
+				rule->salt = rte_be_to_cpu_32(salt);
 			}
 
 			cipher_algo_p = 1;
@@ -573,8 +575,9 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
 				key_len -= 4;
 				rule->auth_key_len = key_len;
 				rule->iv_len = algo->iv_len;
-				memcpy(&rule->salt,
+				memcpy(&salt,
 					&rule->auth_key[key_len], 4);
+				rule->salt = rte_be_to_cpu_32(salt);
 			}
 
 			auth_algo_p = 1;
@@ -632,8 +635,9 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
 
 			key_len -= 4;
 			rule->cipher_key_len = key_len;
-			memcpy(&rule->salt,
+			memcpy(&salt,
 				&rule->cipher_key[key_len], 4);
+			rule->salt = rte_be_to_cpu_32(salt);
 
 			aead_algo_p = 1;
 			continue;
-- 
2.39.1

[PATCH 2/2] net/nfp: fix data endianness problem

[Chaoyong He]

From: Shihong Wang <shihong.wang@corigine.com>

The algorithm key of the security framework is stored in the u8
array according to big-endian, and the driver algorithm key is
CPU-endian of u32, so it maybe need to convert the endianness order
to ensure that the value assigned to the driver is CPU-endian.

This patch removes the operation of converting IPsec Tx metadata
to big-endian to ensure that IPsec Tx metadata is CPU-endian.

Fixes: 547137405be7 ("net/nfp: initialize IPsec related content")
Fixes: 3d21da66c06b ("net/nfp: create security session")
Fixes: 310a1780581e ("net/nfp: support IPsec Rx and Tx offload")
Cc: stable@dpdk.org

Signed-off-by: Shihong Wang <shihong.wang@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/nfp/nfp_ipsec.c | 72 +++++++++++++++++++++++--------------
 drivers/net/nfp/nfp_ipsec.h |  9 ++---
 2 files changed, 47 insertions(+), 34 deletions(-)

diff --git a/drivers/net/nfp/nfp_ipsec.c b/drivers/net/nfp/nfp_ipsec.c
index 0b815fa983..8824533656 100644
--- a/drivers/net/nfp/nfp_ipsec.c
+++ b/drivers/net/nfp/nfp_ipsec.c
@@ -20,6 +20,7 @@
 #include "nfp_rxtx.h"
 
 #define NFP_UDP_ESP_PORT            4500
+#define NFP_ESP_IV_LENGTH           8
 
 static const struct rte_cryptodev_capabilities nfp_crypto_caps[] = {
 	{
@@ -523,7 +524,8 @@ nfp_aesgcm_iv_update(struct ipsec_add_sa *cfg,
 	char *save;
 	char *iv_b;
 	char *iv_str;
-	uint8_t *cfg_iv;
+	const rte_be32_t *iv_value;
+	uint8_t cfg_iv[NFP_ESP_IV_LENGTH];
 
 	iv_str = strdup(iv_string);
 	if (iv_str == NULL) {
@@ -531,8 +533,6 @@ nfp_aesgcm_iv_update(struct ipsec_add_sa *cfg,
 		return;
 	}
 
-	cfg_iv = (uint8_t *)cfg->aesgcm_fields.iv;
-
 	for (i = 0; i < iv_len; i++) {
 		iv_b = strtok_r(i ? NULL : iv_str, ",", &save);
 		if (iv_b == NULL)
@@ -541,8 +541,9 @@ nfp_aesgcm_iv_update(struct ipsec_add_sa *cfg,
 		cfg_iv[i] = strtoul(iv_b, NULL, 0);
 	}
 
-	*(uint32_t *)cfg_iv = rte_be_to_cpu_32(*(uint32_t *)cfg_iv);
-	*(uint32_t *)&cfg_iv[4] = rte_be_to_cpu_32(*(uint32_t *)&cfg_iv[4]);
+	iv_value = (const rte_be32_t *)(cfg_iv);
+	cfg->aesgcm_fields.iv[0] = rte_be_to_cpu_32(iv_value[0]);
+	cfg->aesgcm_fields.iv[1] = rte_be_to_cpu_32(iv_value[1]);
 
 	free(iv_str);
 }
@@ -583,7 +584,7 @@ nfp_aead_map(struct rte_eth_dev *eth_dev,
 	uint32_t offset;
 	uint32_t device_id;
 	const char *iv_str;
-	const uint32_t *key;
+	const rte_be32_t *key;
 	struct nfp_net_hw *net_hw;
 
 	net_hw = eth_dev->data->dev_private;
@@ -633,7 +634,7 @@ nfp_aead_map(struct rte_eth_dev *eth_dev,
 		return -EINVAL;
 	}
 
-	key = (const uint32_t *)(aead->key.data);
+	key = (const rte_be32_t *)(aead->key.data);
 
 	/*
 	 * The CHACHA20's key order needs to be adjusted based on hardware design.
@@ -645,16 +646,22 @@ nfp_aead_map(struct rte_eth_dev *eth_dev,
 
 	for (i = 0; i < key_length / sizeof(cfg->cipher_key[0]); i++) {
 		index = (i + offset) % (key_length / sizeof(cfg->cipher_key[0]));
-		cfg->cipher_key[index] = rte_cpu_to_be_32(*key++);
+		cfg->cipher_key[index] = rte_be_to_cpu_32(key[i]);
 	}
 
 	/*
-	 * The iv of the FW is equal to ESN by default. Reading the
-	 * iv of the configuration information is not supported.
+	 * The iv of the FW is equal to ESN by default. Only the
+	 * aead algorithm can offload the iv of configuration and
+	 * the length of iv cannot be greater than NFP_ESP_IV_LENGTH.
 	 */
 	iv_str = getenv("ETH_SEC_IV_OVR");
 	if (iv_str != NULL) {
 		iv_len = aead->iv.length;
+		if (iv_len > NFP_ESP_IV_LENGTH) {
+			PMD_DRV_LOG(ERR, "Unsupported length of iv data");
+			return -EINVAL;
+		}
+
 		nfp_aesgcm_iv_update(cfg, iv_len, iv_str);
 	}
 
@@ -671,7 +678,7 @@ nfp_cipher_map(struct rte_eth_dev *eth_dev,
 	int ret;
 	uint32_t i;
 	uint32_t device_id;
-	const uint32_t *key;
+	const rte_be32_t *key;
 	struct nfp_net_hw *net_hw;
 
 	net_hw = eth_dev->data->dev_private;
@@ -705,14 +712,14 @@ nfp_cipher_map(struct rte_eth_dev *eth_dev,
 		return -EINVAL;
 	}
 
-	key = (const uint32_t  *)(cipher->key.data);
+	key = (const rte_be32_t *)(cipher->key.data);
 	if (key_length > sizeof(cfg->cipher_key)) {
 		PMD_DRV_LOG(ERR, "Insufficient space for offloaded key");
 		return -EINVAL;
 	}
 
 	for (i = 0; i < key_length / sizeof(cfg->cipher_key[0]); i++)
-		cfg->cipher_key[i] = rte_cpu_to_be_32(*key++);
+		cfg->cipher_key[i] = rte_be_to_cpu_32(key[i]);
 
 	return 0;
 }
@@ -807,7 +814,7 @@ nfp_auth_map(struct rte_eth_dev *eth_dev,
 	uint32_t i;
 	uint8_t key_length;
 	uint32_t device_id;
-	const uint32_t *key;
+	const rte_be32_t *key;
 	struct nfp_net_hw *net_hw;
 
 	if (digest_length == 0) {
@@ -854,7 +861,7 @@ nfp_auth_map(struct rte_eth_dev *eth_dev,
 		return -EINVAL;
 	}
 
-	key = (const uint32_t *)(auth->key.data);
+	key = (const rte_be32_t *)(auth->key.data);
 	key_length = auth->key.length;
 	if (key_length > sizeof(cfg->auth_key)) {
 		PMD_DRV_LOG(ERR, "Insufficient space for offloaded auth key!");
@@ -862,7 +869,7 @@ nfp_auth_map(struct rte_eth_dev *eth_dev,
 	}
 
 	for (i = 0; i < key_length / sizeof(cfg->auth_key[0]); i++)
-		cfg->auth_key[i] = rte_cpu_to_be_32(*key++);
+		cfg->auth_key[i] = rte_be_to_cpu_32(key[i]);
 
 	return 0;
 }
@@ -902,7 +909,7 @@ nfp_crypto_msg_build(struct rte_eth_dev *eth_dev,
 			return ret;
 		}
 
-		cfg->aesgcm_fields.salt = rte_cpu_to_be_32(conf->ipsec.salt);
+		cfg->aesgcm_fields.salt = conf->ipsec.salt;
 		break;
 	case RTE_CRYPTO_SYM_XFORM_AUTH:
 		/* Only support Auth + Cipher for inbound */
@@ -967,7 +974,10 @@ nfp_ipsec_msg_build(struct rte_eth_dev *eth_dev,
 		struct rte_security_session_conf *conf,
 		struct nfp_ipsec_msg *msg)
 {
+	int i;
 	int ret;
+	rte_be32_t *src_ip;
+	rte_be32_t *dst_ip;
 	struct ipsec_add_sa *cfg;
 	enum rte_security_ipsec_tunnel_type type;
 
@@ -1025,12 +1035,18 @@ nfp_ipsec_msg_build(struct rte_eth_dev *eth_dev,
 		type = conf->ipsec.tunnel.type;
 		cfg->ctrl_word.mode = NFP_IPSEC_MODE_TUNNEL;
 		if (type == RTE_SECURITY_IPSEC_TUNNEL_IPV4) {
-			cfg->src_ip.v4 = conf->ipsec.tunnel.ipv4.src_ip;
-			cfg->dst_ip.v4 = conf->ipsec.tunnel.ipv4.dst_ip;
+			src_ip = (rte_be32_t *)&conf->ipsec.tunnel.ipv4.src_ip.s_addr;
+			dst_ip = (rte_be32_t *)&conf->ipsec.tunnel.ipv4.dst_ip.s_addr;
+			cfg->src_ip[0] = rte_be_to_cpu_32(src_ip[0]);
+			cfg->dst_ip[0] = rte_be_to_cpu_32(dst_ip[0]);
 			cfg->ipv6 = 0;
 		} else if (type == RTE_SECURITY_IPSEC_TUNNEL_IPV6) {
-			cfg->src_ip.v6 = conf->ipsec.tunnel.ipv6.src_addr;
-			cfg->dst_ip.v6 = conf->ipsec.tunnel.ipv6.dst_addr;
+			src_ip = (rte_be32_t *)conf->ipsec.tunnel.ipv6.src_addr.s6_addr;
+			dst_ip = (rte_be32_t *)conf->ipsec.tunnel.ipv6.dst_addr.s6_addr;
+			for (i = 0; i < 4; i++) {
+				cfg->src_ip[i] = rte_be_to_cpu_32(src_ip[i]);
+				cfg->dst_ip[i] = rte_be_to_cpu_32(dst_ip[i]);
+			}
 			cfg->ipv6 = 1;
 		} else {
 			PMD_DRV_LOG(ERR, "Unsupported address family!");
@@ -1043,9 +1059,11 @@ nfp_ipsec_msg_build(struct rte_eth_dev *eth_dev,
 		cfg->ctrl_word.mode = NFP_IPSEC_MODE_TRANSPORT;
 		if (type == RTE_SECURITY_IPSEC_TUNNEL_IPV4) {
 			memset(&cfg->src_ip, 0, sizeof(cfg->src_ip));
+			memset(&cfg->dst_ip, 0, sizeof(cfg->dst_ip));
 			cfg->ipv6 = 0;
 		} else if (type == RTE_SECURITY_IPSEC_TUNNEL_IPV6) {
 			memset(&cfg->src_ip, 0, sizeof(cfg->src_ip));
+			memset(&cfg->dst_ip, 0, sizeof(cfg->dst_ip));
 			cfg->ipv6 = 1;
 		} else {
 			PMD_DRV_LOG(ERR, "Unsupported address family!");
@@ -1179,18 +1197,18 @@ nfp_security_set_pkt_metadata(void *device,
 		desc_md = RTE_MBUF_DYNFIELD(m, offset, struct nfp_tx_ipsec_desc_msg *);
 
 		if (priv_session->msg.ctrl_word.ext_seq != 0 && sqn != NULL) {
-			desc_md->esn.low = rte_cpu_to_be_32(*sqn);
-			desc_md->esn.hi = rte_cpu_to_be_32(*sqn >> 32);
+			desc_md->esn.low = (uint32_t)*sqn;
+			desc_md->esn.hi = (uint32_t)(*sqn >> 32);
 		} else if (priv_session->msg.ctrl_word.ext_seq != 0) {
-			desc_md->esn.low = rte_cpu_to_be_32(priv_session->ipsec.esn.low);
-			desc_md->esn.hi = rte_cpu_to_be_32(priv_session->ipsec.esn.hi);
+			desc_md->esn.low = priv_session->ipsec.esn.low;
+			desc_md->esn.hi = priv_session->ipsec.esn.hi;
 		} else {
-			desc_md->esn.low = rte_cpu_to_be_32(priv_session->ipsec.esn.value);
+			desc_md->esn.low = priv_session->ipsec.esn.low;
 			desc_md->esn.hi = 0;
 		}
 
 		desc_md->enc = 1;
-		desc_md->sa_idx = rte_cpu_to_be_32(priv_session->sa_index);
+		desc_md->sa_idx = priv_session->sa_index;
 	}
 
 	return 0;
diff --git a/drivers/net/nfp/nfp_ipsec.h b/drivers/net/nfp/nfp_ipsec.h
index d7a729398a..f7c4f3f225 100644
--- a/drivers/net/nfp/nfp_ipsec.h
+++ b/drivers/net/nfp/nfp_ipsec.h
@@ -36,11 +36,6 @@ struct sa_ctrl_word {
 	uint32_t spare2 :1;      /**< Must be set to 0 */
 };
 
-union nfp_ip_addr {
-	struct in6_addr v6;
-	struct in_addr v4;
-};
-
 struct ipsec_add_sa {
 	uint32_t cipher_key[8];           /**< Cipher Key */
 	union {
@@ -60,8 +55,8 @@ struct ipsec_add_sa {
 	uint8_t spare1;
 	uint32_t soft_byte_cnt;           /**< Soft lifetime byte count */
 	uint32_t hard_byte_cnt;           /**< Hard lifetime byte count */
-	union nfp_ip_addr src_ip;         /**< Src IP addr */
-	union nfp_ip_addr dst_ip;         /**< Dst IP addr */
+	uint32_t src_ip[4];               /**< Src IP addr */
+	uint32_t dst_ip[4];               /**< Dst IP addr */
 	uint16_t natt_dst_port;           /**< NAT-T UDP Header dst port */
 	uint16_t natt_src_port;           /**< NAT-T UDP Header src port */
 	uint32_t soft_lifetime_limit;     /**< Soft lifetime time limit */
-- 
2.39.1

Re: [PATCH 0/2] fix IPSec endianness problem

[Ferruh Yigit]

On 3/11/2024 2:49 AM, Chaoyong He wrote:
> This patch series aims to fix the IPSec problem related with SA
> salt endianness.
> 
> Shihong Wang (2):
>   examples/ipsec-secgw: fix SA salt endianness problem
>   net/nfp: fix data endianness problem
>

Hi Chaoyong,

Just to confirm, there is no dependency between above two patches, right?
So it will be OK for me and Akhil merge them to different trees, and
they will end up merged in a different order.

Re: [PATCH 2/2] net/nfp: fix data endianness problem

[Ferruh Yigit]

On 3/11/2024 2:49 AM, Chaoyong He wrote:
> From: Shihong Wang <shihong.wang@corigine.com>
> 
> The algorithm key of the security framework is stored in the u8
> array according to big-endian, and the driver algorithm key is
> CPU-endian of u32, so it maybe need to convert the endianness order
> to ensure that the value assigned to the driver is CPU-endian.
> 
> This patch removes the operation of converting IPsec Tx metadata
> to big-endian to ensure that IPsec Tx metadata is CPU-endian.
> 
> Fixes: 547137405be7 ("net/nfp: initialize IPsec related content")
> Fixes: 3d21da66c06b ("net/nfp: create security session")
> Fixes: 310a1780581e ("net/nfp: support IPsec Rx and Tx offload")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Shihong Wang <shihong.wang@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>

Applied to dpdk-next-net/main, thanks.
(not the set, just this patch)

RE: [EXTERNAL] [PATCH 1/2] examples/ipsec-secgw: fix SA salt endianness problem

[Akhil Goyal]

> Subject: [EXTERNAL] [PATCH 1/2] examples/ipsec-secgw: fix SA salt endianness
> problem
> From: Shihong Wang <shihong.wang@corigine.com>
> 
> The SA salt of struct ipsec_sa is a CPU-endian u32 variable, but it’s
> value is stored in an array of encryption or authentication keys
> according to big-endian. So it maybe need to convert the endianness
> order to ensure that the value assigned to the SA salt is CPU-endian.
> 
> Fixes: 50d75cae2a2c ("examples/ipsec-secgw: initialize SA salt")
> Fixes: 9413c3901f31 ("examples/ipsec-secgw: support additional algorithms")
> Fixes: 501e9c226adf ("examples/ipsec-secgw: add AEAD parameters")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Shihong Wang <shihong.wang@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> ---
>  examples/ipsec-secgw/sa.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> index c4bac17cd7..4018b0558a 100644
> --- a/examples/ipsec-secgw/sa.c
> +++ b/examples/ipsec-secgw/sa.c
> @@ -374,6 +374,7 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
>  	uint32_t ti; /*token index*/
>  	uint32_t *ri /*rule index*/;
>  	struct ipsec_sa_cnt *sa_cnt;
> +	rte_be32_t salt; /*big-endian salt*/
>  	uint32_t cipher_algo_p = 0;
>  	uint32_t auth_algo_p = 0;
>  	uint32_t aead_algo_p = 0;
> @@ -508,8 +509,9 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
>  			if (algo->algo == RTE_CRYPTO_CIPHER_AES_CTR) {
>  				key_len -= 4;
>  				rule->cipher_key_len = key_len;
> -				memcpy(&rule->salt,
> +				memcpy(&salt,
>  					&rule->cipher_key[key_len], 4);
> +				rule->salt = rte_be_to_cpu_32(salt);
>  			}
> 
>  			cipher_algo_p = 1;
> @@ -573,8 +575,9 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
>  				key_len -= 4;
>  				rule->auth_key_len = key_len;
>  				rule->iv_len = algo->iv_len;
> -				memcpy(&rule->salt,
> +				memcpy(&salt,
>  					&rule->auth_key[key_len], 4);
> +				rule->salt = rte_be_to_cpu_32(salt);
>  			}
> 
>  			auth_algo_p = 1;
> @@ -632,8 +635,9 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> 
>  			key_len -= 4;
>  			rule->cipher_key_len = key_len;
> -			memcpy(&rule->salt,
> +			memcpy(&salt,
>  				&rule->cipher_key[key_len], 4);
Can you put the memcpy call in a single line?

> +			rule->salt = rte_be_to_cpu_32(salt);

RE: [EXTERNAL] [PATCH 1/2] examples/ipsec-secgw: fix SA salt endianness problem

[Chaoyong He]

> > Subject: [EXTERNAL] [PATCH 1/2] examples/ipsec-secgw: fix SA salt
> > endianness problem
> > From: Shihong Wang <shihong.wang@corigine.com>
> >
> > The SA salt of struct ipsec_sa is a CPU-endian u32 variable, but it’s
> > value is stored in an array of encryption or authentication keys
> > according to big-endian. So it maybe need to convert the endianness
> > order to ensure that the value assigned to the SA salt is CPU-endian.
> >
> > Fixes: 50d75cae2a2c ("examples/ipsec-secgw: initialize SA salt")
> > Fixes: 9413c3901f31 ("examples/ipsec-secgw: support additional
> > algorithms")
> > Fixes: 501e9c226adf ("examples/ipsec-secgw: add AEAD parameters")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Shihong Wang <shihong.wang@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > ---
> >  examples/ipsec-secgw/sa.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
> > index c4bac17cd7..4018b0558a 100644
> > --- a/examples/ipsec-secgw/sa.c
> > +++ b/examples/ipsec-secgw/sa.c
> > @@ -374,6 +374,7 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> >  	uint32_t ti; /*token index*/
> >  	uint32_t *ri /*rule index*/;
> >  	struct ipsec_sa_cnt *sa_cnt;
> > +	rte_be32_t salt; /*big-endian salt*/
> >  	uint32_t cipher_algo_p = 0;
> >  	uint32_t auth_algo_p = 0;
> >  	uint32_t aead_algo_p = 0;
> > @@ -508,8 +509,9 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> >  			if (algo->algo == RTE_CRYPTO_CIPHER_AES_CTR) {
> >  				key_len -= 4;
> >  				rule->cipher_key_len = key_len;
> > -				memcpy(&rule->salt,
> > +				memcpy(&salt,
> >  					&rule->cipher_key[key_len], 4);
> > +				rule->salt = rte_be_to_cpu_32(salt);
> >  			}
> >
> >  			cipher_algo_p = 1;
> > @@ -573,8 +575,9 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> >  				key_len -= 4;
> >  				rule->auth_key_len = key_len;
> >  				rule->iv_len = algo->iv_len;
> > -				memcpy(&rule->salt,
> > +				memcpy(&salt,
> >  					&rule->auth_key[key_len], 4);
> > +				rule->salt = rte_be_to_cpu_32(salt);
> >  			}
> >
> >  			auth_algo_p = 1;
> > @@ -632,8 +635,9 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
> >
> >  			key_len -= 4;
> >  			rule->cipher_key_len = key_len;
> > -			memcpy(&rule->salt,
> > +			memcpy(&salt,
> >  				&rule->cipher_key[key_len], 4);
> Can you put the memcpy call in a single line?

Okay, no problem.
I will send a v2 patch to fix that, thanks.

> 
> > +			rule->salt = rte_be_to_cpu_32(salt);

[PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Chaoyong He]

From: Shihong Wang <shihong.wang@corigine.com>

The SA salt of struct ipsec_sa is a CPU-endian u32 variable, but it’s
value is stored in an array of encryption or authentication keys
according to big-endian. So it maybe need to convert the endianness
order to ensure that the value assigned to the SA salt is CPU-endian.

Fixes: 50d75cae2a2c ("examples/ipsec-secgw: initialize SA salt")
Fixes: 9413c3901f31 ("examples/ipsec-secgw: support additional algorithms")
Fixes: 501e9c226adf ("examples/ipsec-secgw: add AEAD parameters")
Cc: stable@dpdk.org

Signed-off-by: Shihong Wang <shihong.wang@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>

---
v2:
* Put the 'memcpy()' call in a singal line as the review comment.
---
 examples/ipsec-secgw/sa.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
index c4bac17cd7..8aa9aca739 100644
--- a/examples/ipsec-secgw/sa.c
+++ b/examples/ipsec-secgw/sa.c
@@ -374,6 +374,7 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
 	uint32_t ti; /*token index*/
 	uint32_t *ri /*rule index*/;
 	struct ipsec_sa_cnt *sa_cnt;
+	rte_be32_t salt; /*big-endian salt*/
 	uint32_t cipher_algo_p = 0;
 	uint32_t auth_algo_p = 0;
 	uint32_t aead_algo_p = 0;
@@ -508,8 +509,8 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
 			if (algo->algo == RTE_CRYPTO_CIPHER_AES_CTR) {
 				key_len -= 4;
 				rule->cipher_key_len = key_len;
-				memcpy(&rule->salt,
-					&rule->cipher_key[key_len], 4);
+				memcpy(&salt, &rule->cipher_key[key_len], 4);
+				rule->salt = rte_be_to_cpu_32(salt);
 			}
 
 			cipher_algo_p = 1;
@@ -573,8 +574,8 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
 				key_len -= 4;
 				rule->auth_key_len = key_len;
 				rule->iv_len = algo->iv_len;
-				memcpy(&rule->salt,
-					&rule->auth_key[key_len], 4);
+				memcpy(&salt, &rule->auth_key[key_len], 4);
+				rule->salt = rte_be_to_cpu_32(salt);
 			}
 
 			auth_algo_p = 1;
@@ -632,8 +633,8 @@ parse_sa_tokens(char **tokens, uint32_t n_tokens,
 
 			key_len -= 4;
 			rule->cipher_key_len = key_len;
-			memcpy(&rule->salt,
-				&rule->cipher_key[key_len], 4);
+			memcpy(&salt, &rule->cipher_key[key_len], 4);
+			rule->salt = rte_be_to_cpu_32(salt);
 
 			aead_algo_p = 1;
 			continue;
-- 
2.39.1

RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Akhil Goyal]

> From: Shihong Wang <shihong.wang@corigine.com>
> 
> The SA salt of struct ipsec_sa is a CPU-endian u32 variable, but it’s
> value is stored in an array of encryption or authentication keys
> according to big-endian. So it maybe need to convert the endianness
> order to ensure that the value assigned to the SA salt is CPU-endian.
> 
> Fixes: 50d75cae2a2c ("examples/ipsec-secgw: initialize SA salt")
> Fixes: 9413c3901f31 ("examples/ipsec-secgw: support additional algorithms")
> Fixes: 501e9c226adf ("examples/ipsec-secgw: add AEAD parameters")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Shihong Wang <shihong.wang@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> 
Acked-by: Akhil Goyal <gakhil@marvell.com>

Applied to dpdk-next-crypto
Thanks

RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Akhil Goyal]

> Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt
> endianness problem
> 
> > From: Shihong Wang <shihong.wang@corigine.com>
> >
> > The SA salt of struct ipsec_sa is a CPU-endian u32 variable, but it’s
> > value is stored in an array of encryption or authentication keys
> > according to big-endian. So it maybe need to convert the endianness
> > order to ensure that the value assigned to the SA salt is CPU-endian.
> >
> > Fixes: 50d75cae2a2c ("examples/ipsec-secgw: initialize SA salt")
> > Fixes: 9413c3901f31 ("examples/ipsec-secgw: support additional algorithms")
> > Fixes: 501e9c226adf ("examples/ipsec-secgw: add AEAD parameters")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Shihong Wang <shihong.wang@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> >
> Acked-by: Akhil Goyal <gakhil@marvell.com>
> 
> Applied to dpdk-next-crypto

The patch is pulled back from dpdk-next-crypto.
This change may cause all the PMDs to fail these cases.
Would need acks from PMDs.

RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Akhil Goyal]

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Friday, March 15, 2024 12:42 AM
> To: Akhil Goyal <gakhil@marvell.com>; Chaoyong He
> <chaoyong.he@corigine.com>; dev@dpdk.org
> Cc: oss-drivers@corigine.com; Shihong Wang <shihong.wang@corigine.com>;
> stable@dpdk.org
> Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt
> endianness problem
> 
> > Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt
> > endianness problem
> >
> > > From: Shihong Wang <shihong.wang@corigine.com>
> > >
> > > The SA salt of struct ipsec_sa is a CPU-endian u32 variable, but it’s
> > > value is stored in an array of encryption or authentication keys
> > > according to big-endian. So it maybe need to convert the endianness
> > > order to ensure that the value assigned to the SA salt is CPU-endian.
> > >
> > > Fixes: 50d75cae2a2c ("examples/ipsec-secgw: initialize SA salt")
> > > Fixes: 9413c3901f31 ("examples/ipsec-secgw: support additional algorithms")
> > > Fixes: 501e9c226adf ("examples/ipsec-secgw: add AEAD parameters")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Shihong Wang <shihong.wang@corigine.com>
> > > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > >
> > Acked-by: Akhil Goyal <gakhil@marvell.com>
> >
> > Applied to dpdk-next-crypto
> 
> The patch is pulled back from dpdk-next-crypto.
> This change may cause all the PMDs to fail these cases.
> Would need acks from PMDs.

Applied to dpdk-next-crypto
No update from PMD owners.
Applying it before RC2 so that we have time for fixes if needed.

Re: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Medvedkin, Vladimir]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

Hi all,

This patch breaks ipsec tests with ipsec-secgw:

./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
...
ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for dst=192.168.31.14, sz=1
  test IPv4 trs_aesctr_sha1 finished with status 1
ERROR  test trs_aesctr_sha1 FAILED


On 03/07/2024 18:58, Akhil Goyal wrote:
>
>> -----Original Message-----
>> From: Akhil Goyal<gakhil@marvell.com>
>> Sent: Friday, March 15, 2024 12:42 AM
>> To: Akhil Goyal<gakhil@marvell.com>; Chaoyong He
>> <chaoyong.he@corigine.com>;dev@dpdk.org
>> Cc:oss-drivers@corigine.com; Shihong Wang<shihong.wang@corigine.com>;
>> stable@dpdk.org
>> Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt
>> endianness problem
>>
>>> Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt
>>> endianness problem
>>>
>>>> From: Shihong Wang<shihong.wang@corigine.com>
>>>>
>>>> The SA salt of struct ipsec_sa is a CPU-endian u32 variable, but it’s
>>>> value is stored in an array of encryption or authentication keys
>>>> according to big-endian. So it maybe need to convert the endianness
>>>> order to ensure that the value assigned to the SA salt is CPU-endian.
>>>>
>>>> Fixes: 50d75cae2a2c ("examples/ipsec-secgw: initialize SA salt")
>>>> Fixes: 9413c3901f31 ("examples/ipsec-secgw: support additional algorithms")
>>>> Fixes: 501e9c226adf ("examples/ipsec-secgw: add AEAD parameters")
>>>> Cc:stable@dpdk.org
>>>>
>>>> Signed-off-by: Shihong Wang<shihong.wang@corigine.com>
>>>> Reviewed-by: Chaoyong He<chaoyong.he@corigine.com>
>>>>
>>> Acked-by: Akhil Goyal<gakhil@marvell.com>
>>>
>>> Applied to dpdk-next-crypto
>> The patch is pulled back from dpdk-next-crypto.
>> This change may cause all the PMDs to fail these cases.
>> Would need acks from PMDs.
> Applied to dpdk-next-crypto
> No update from PMD owners.
> Applying it before RC2 so that we have time for fixes if needed.
>
-- 
Regards,
Vladimir

[-- Attachment #2: Type: text/html, Size: 4323 bytes --]

RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Akhil Goyal]

> 
> Hi all,
> 
> This patch breaks ipsec tests with ipsec-secgw:
> 
> 
> ./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
> ...
> ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for dst=192.168.31.14,
> sz=1
>  test IPv4 trs_aesctr_sha1 finished with status 1
> ERROR  test trs_aesctr_sha1 FAILED
> 

The patch seems to be correct.
Please check endianness in the PMD you are testing.


> 
> 
> 
> On 03/07/2024 18:58, Akhil Goyal wrote:
> 
> 
> 
> 
> 
> 		-----Original Message-----
> 		From: Akhil Goyal <gakhil@marvell.com>
> <mailto:gakhil@marvell.com>
> 		Sent: Friday, March 15, 2024 12:42 AM
> 		To: Akhil Goyal <gakhil@marvell.com>
> <mailto:gakhil@marvell.com> ; Chaoyong He
> 		<chaoyong.he@corigine.com>
> <mailto:chaoyong.he@corigine.com> ; dev@dpdk.org <mailto:dev@dpdk.org>
> 		Cc: oss-drivers@corigine.com <mailto:oss-
> drivers@corigine.com> ; Shihong Wang <shihong.wang@corigine.com>
> <mailto:shihong.wang@corigine.com> ;
> 		stable@dpdk.org <mailto:stable@dpdk.org>
> 		Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix
> SA salt
> 		endianness problem
> 
> 
> 			Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-
> secgw: fix SA salt
> 			endianness problem
> 
> 
> 				From: Shihong Wang
> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
> 
> 				The SA salt of struct ipsec_sa is a CPU-endian
> u32 variable, but it’s
> 				value is stored in an array of encryption or
> authentication keys
> 				according to big-endian. So it maybe need to
> convert the endianness
> 				order to ensure that the value assigned to the
> SA salt is CPU-endian.
> 
> 				Fixes: 50d75cae2a2c ("examples/ipsec-secgw:
> initialize SA salt")
> 				Fixes: 9413c3901f31 ("examples/ipsec-secgw:
> support additional algorithms")
> 				Fixes: 501e9c226adf ("examples/ipsec-secgw:
> add AEAD parameters")
> 				Cc: stable@dpdk.org <mailto:stable@dpdk.org>
> 
> 				Signed-off-by: Shihong Wang
> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
> 				Reviewed-by: Chaoyong He
> <chaoyong.he@corigine.com> <mailto:chaoyong.he@corigine.com>
> 
> 
> 			Acked-by: Akhil Goyal <gakhil@marvell.com>
> <mailto:gakhil@marvell.com>
> 
> 			Applied to dpdk-next-crypto
> 
> 
> 		The patch is pulled back from dpdk-next-crypto.
> 		This change may cause all the PMDs to fail these cases.
> 		Would need acks from PMDs.
> 
> 
> 	Applied to dpdk-next-crypto
> 	No update from PMD owners.
> 	Applying it before RC2 so that we have time for fixes if needed.
> 
> 
> --
> Regards,
> Vladimir

Re: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Radu Nicolau]

On 23-Jul-24 5:57 PM, Akhil Goyal wrote:
>> Hi all,
>>
>> This patch breaks ipsec tests with ipsec-secgw:
>>
>>
>> ./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
>> ...
>> ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for dst=192.168.31.14,
>> sz=1
>>   test IPv4 trs_aesctr_sha1 finished with status 1
>> ERROR  test trs_aesctr_sha1 FAILED
>>
> The patch seems to be correct.
> Please check endianness in the PMD you are testing.

In my opinion salt should not be affected by endianness and it should be 
used as it is in the key parameter. I think the patch is wrong to make 
it CPU endianness dependent before being passed to the PMDs, any PMD 
that needs the endianness swapped should do it in the PMD code. Indeed 
it's passed around as a 32 bit integer but it's not used as such, and 
when it's actually used it should be evaluated as a byte array.

https://datatracker.ietf.org/doc/html/rfc4106#section-4
https://datatracker.ietf.org/doc/html/rfc4106#section-8.1

>
>
>>
>>
>> On 03/07/2024 18:58, Akhil Goyal wrote:
>>
>>
>>
>>
>>
>> 		-----Original Message-----
>> 		From: Akhil Goyal <gakhil@marvell.com>
>> <mailto:gakhil@marvell.com>
>> 		Sent: Friday, March 15, 2024 12:42 AM
>> 		To: Akhil Goyal <gakhil@marvell.com>
>> <mailto:gakhil@marvell.com> ; Chaoyong He
>> 		<chaoyong.he@corigine.com>
>> <mailto:chaoyong.he@corigine.com> ; dev@dpdk.org <mailto:dev@dpdk.org>
>> 		Cc: oss-drivers@corigine.com <mailto:oss-
>> drivers@corigine.com> ; Shihong Wang <shihong.wang@corigine.com>
>> <mailto:shihong.wang@corigine.com> ;
>> 		stable@dpdk.org <mailto:stable@dpdk.org>
>> 		Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix
>> SA salt
>> 		endianness problem
>>
>>
>> 			Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-
>> secgw: fix SA salt
>> 			endianness problem
>>
>>
>> 				From: Shihong Wang
>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
>>
>> 				The SA salt of struct ipsec_sa is a CPU-endian
>> u32 variable, but it’s
>> 				value is stored in an array of encryption or
>> authentication keys
>> 				according to big-endian. So it maybe need to
>> convert the endianness
>> 				order to ensure that the value assigned to the
>> SA salt is CPU-endian.
>>
>> 				Fixes: 50d75cae2a2c ("examples/ipsec-secgw:
>> initialize SA salt")
>> 				Fixes: 9413c3901f31 ("examples/ipsec-secgw:
>> support additional algorithms")
>> 				Fixes: 501e9c226adf ("examples/ipsec-secgw:
>> add AEAD parameters")
>> 				Cc: stable@dpdk.org <mailto:stable@dpdk.org>
>>
>> 				Signed-off-by: Shihong Wang
>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
>> 				Reviewed-by: Chaoyong He
>> <chaoyong.he@corigine.com> <mailto:chaoyong.he@corigine.com>
>>
>>
>> 			Acked-by: Akhil Goyal <gakhil@marvell.com>
>> <mailto:gakhil@marvell.com>
>>
>> 			Applied to dpdk-next-crypto
>>
>>
>> 		The patch is pulled back from dpdk-next-crypto.
>> 		This change may cause all the PMDs to fail these cases.
>> 		Would need acks from PMDs.
>>
>>
>> 	Applied to dpdk-next-crypto
>> 	No update from PMD owners.
>> 	Applying it before RC2 so that we have time for fixes if needed.
>>
>>
>> --
>> Regards,
>> Vladimir

RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Akhil Goyal]

> 
> On 23-Jul-24 5:57 PM, Akhil Goyal wrote:
> >> Hi all,
> >>
> >> This patch breaks ipsec tests with ipsec-secgw:
> >>
> >>
> >> ./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
> >> ...
> >> ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for
> dst=192.168.31.14,
> >> sz=1
> >>   test IPv4 trs_aesctr_sha1 finished with status 1
> >> ERROR  test trs_aesctr_sha1 FAILED
> >>
> > The patch seems to be correct.
> > Please check endianness in the PMD you are testing.
> 
> In my opinion salt should not be affected by endianness and it should be
> used as it is in the key parameter. I think the patch is wrong to make
> it CPU endianness dependent before being passed to the PMDs, any PMD
> that needs the endianness swapped should do it in the PMD code. Indeed
> it's passed around as a 32 bit integer but it's not used as such, and
> when it's actually used it should be evaluated as a byte array.
> 
> https://datatracker.ietf.org/doc/html/rfc4106#section-4
> https://datatracker.ietf.org/doc/html/rfc4106#section-8.1

As per the rfc, it should be treated as byte order(i.e. big endian).
But here the problem is we treat it as uint32_t which makes it CPU endian when stored in ipsec_sa struct.
The keys are stored as an array of uint8_t, so keys are stored in byte order(Big endian).

So either we save salt as "uint8_t salt[4]" or do a conversion of cpu_to_be
So that when it is stored in PMD/HW, and we convert it from uint32_t to uint_8 *, there wont be issue.

> 
> >
> >
> >>
> >>
> >> On 03/07/2024 18:58, Akhil Goyal wrote:
> >>
> >>
> >>
> >>
> >>
> >> 		-----Original Message-----
> >> 		From: Akhil Goyal <gakhil@marvell.com>
> >> <mailto:gakhil@marvell.com>
> >> 		Sent: Friday, March 15, 2024 12:42 AM
> >> 		To: Akhil Goyal <gakhil@marvell.com>
> >> <mailto:gakhil@marvell.com> ; Chaoyong He
> >> 		<chaoyong.he@corigine.com>
> >> <mailto:chaoyong.he@corigine.com> ; dev@dpdk.org <mailto:dev@dpdk.org>
> >> 		Cc: oss-drivers@corigine.com <mailto:oss-
> >> drivers@corigine.com> ; Shihong Wang <shihong.wang@corigine.com>
> >> <mailto:shihong.wang@corigine.com> ;
> >> 		stable@dpdk.org <mailto:stable@dpdk.org>
> >> 		Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix
> >> SA salt
> >> 		endianness problem
> >>
> >>
> >> 			Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-
> >> secgw: fix SA salt
> >> 			endianness problem
> >>
> >>
> >> 				From: Shihong Wang
> >> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
> >>
> >> 				The SA salt of struct ipsec_sa is a CPU-endian
> >> u32 variable, but it’s
> >> 				value is stored in an array of encryption or
> >> authentication keys
> >> 				according to big-endian. So it maybe need to
> >> convert the endianness
> >> 				order to ensure that the value assigned to the
> >> SA salt is CPU-endian.
> >>
> >> 				Fixes: 50d75cae2a2c ("examples/ipsec-secgw:
> >> initialize SA salt")
> >> 				Fixes: 9413c3901f31 ("examples/ipsec-secgw:
> >> support additional algorithms")
> >> 				Fixes: 501e9c226adf ("examples/ipsec-secgw:
> >> add AEAD parameters")
> >> 				Cc: stable@dpdk.org <mailto:stable@dpdk.org>
> >>
> >> 				Signed-off-by: Shihong Wang
> >> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
> >> 				Reviewed-by: Chaoyong He
> >> <chaoyong.he@corigine.com> <mailto:chaoyong.he@corigine.com>
> >>
> >>
> >> 			Acked-by: Akhil Goyal <gakhil@marvell.com>
> >> <mailto:gakhil@marvell.com>
> >>
> >> 			Applied to dpdk-next-crypto
> >>
> >>
> >> 		The patch is pulled back from dpdk-next-crypto.
> >> 		This change may cause all the PMDs to fail these cases.
> >> 		Would need acks from PMDs.
> >>
> >>
> >> 	Applied to dpdk-next-crypto
> >> 	No update from PMD owners.
> >> 	Applying it before RC2 so that we have time for fixes if needed.
> >>
> >>
> >> --
> >> Regards,
> >> Vladimir

Re: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Radu Nicolau]

On 24-Jul-24 12:20 PM, Akhil Goyal wrote:
>> On 23-Jul-24 5:57 PM, Akhil Goyal wrote:
>>>> Hi all,
>>>>
>>>> This patch breaks ipsec tests with ipsec-secgw:
>>>>
>>>>
>>>> ./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
>>>> ...
>>>> ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for
>> dst=192.168.31.14,
>>>> sz=1
>>>>    test IPv4 trs_aesctr_sha1 finished with status 1
>>>> ERROR  test trs_aesctr_sha1 FAILED
>>>>
>>> The patch seems to be correct.
>>> Please check endianness in the PMD you are testing.
>> In my opinion salt should not be affected by endianness and it should be
>> used as it is in the key parameter. I think the patch is wrong to make
>> it CPU endianness dependent before being passed to the PMDs, any PMD
>> that needs the endianness swapped should do it in the PMD code. Indeed
>> it's passed around as a 32 bit integer but it's not used as such, and
>> when it's actually used it should be evaluated as a byte array.
>>
>> https://datatracker.ietf.org/doc/html/rfc4106#section-4
>> https://datatracker.ietf.org/doc/html/rfc4106#section-8.1
> As per the rfc, it should be treated as byte order(i.e. big endian).
> But here the problem is we treat it as uint32_t which makes it CPU endian when stored in ipsec_sa struct.
> The keys are stored as an array of uint8_t, so keys are stored in byte order(Big endian).
>
> So either we save salt as "uint8_t salt[4]" or do a conversion of cpu_to_be
> So that when it is stored in PMD/HW, and we convert it from uint32_t to uint_8 *, there wont be issue.

RFC treats it as a "four octet value" - there is no endianness until 
it's treated like an integer, which it never is. Even if it code it's 
being stored and passed as an unsigned 32bit integer it is never 
evaluated as such so its endianness doesn't matter.

I agree that we should have it everywhere as "uint8_t salt[4]" but that 
implies API changes and it doesn't change how the bytes are stored, so 
the patch will still be wrong.


>
>>>
>>>>
>>>> On 03/07/2024 18:58, Akhil Goyal wrote:
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> 		-----Original Message-----
>>>> 		From: Akhil Goyal <gakhil@marvell.com>
>>>> <mailto:gakhil@marvell.com>
>>>> 		Sent: Friday, March 15, 2024 12:42 AM
>>>> 		To: Akhil Goyal <gakhil@marvell.com>
>>>> <mailto:gakhil@marvell.com> ; Chaoyong He
>>>> 		<chaoyong.he@corigine.com>
>>>> <mailto:chaoyong.he@corigine.com> ; dev@dpdk.org <mailto:dev@dpdk.org>
>>>> 		Cc: oss-drivers@corigine.com <mailto:oss-
>>>> drivers@corigine.com> ; Shihong Wang <shihong.wang@corigine.com>
>>>> <mailto:shihong.wang@corigine.com> ;
>>>> 		stable@dpdk.org <mailto:stable@dpdk.org>
>>>> 		Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix
>>>> SA salt
>>>> 		endianness problem
>>>>
>>>>
>>>> 			Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-
>>>> secgw: fix SA salt
>>>> 			endianness problem
>>>>
>>>>
>>>> 				From: Shihong Wang
>>>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
>>>>
>>>> 				The SA salt of struct ipsec_sa is a CPU-endian
>>>> u32 variable, but it’s
>>>> 				value is stored in an array of encryption or
>>>> authentication keys
>>>> 				according to big-endian. So it maybe need to
>>>> convert the endianness
>>>> 				order to ensure that the value assigned to the
>>>> SA salt is CPU-endian.
>>>>
>>>> 				Fixes: 50d75cae2a2c ("examples/ipsec-secgw:
>>>> initialize SA salt")
>>>> 				Fixes: 9413c3901f31 ("examples/ipsec-secgw:
>>>> support additional algorithms")
>>>> 				Fixes: 501e9c226adf ("examples/ipsec-secgw:
>>>> add AEAD parameters")
>>>> 				Cc: stable@dpdk.org <mailto:stable@dpdk.org>
>>>>
>>>> 				Signed-off-by: Shihong Wang
>>>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
>>>> 				Reviewed-by: Chaoyong He
>>>> <chaoyong.he@corigine.com> <mailto:chaoyong.he@corigine.com>
>>>>
>>>>
>>>> 			Acked-by: Akhil Goyal <gakhil@marvell.com>
>>>> <mailto:gakhil@marvell.com>
>>>>
>>>> 			Applied to dpdk-next-crypto
>>>>
>>>>
>>>> 		The patch is pulled back from dpdk-next-crypto.
>>>> 		This change may cause all the PMDs to fail these cases.
>>>> 		Would need acks from PMDs.
>>>>
>>>>
>>>> 	Applied to dpdk-next-crypto
>>>> 	No update from PMD owners.
>>>> 	Applying it before RC2 so that we have time for fixes if needed.
>>>>
>>>>
>>>> --
>>>> Regards,
>>>> Vladimir

RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Akhil Goyal]

> On 24-Jul-24 12:20 PM, Akhil Goyal wrote:
> >> On 23-Jul-24 5:57 PM, Akhil Goyal wrote:
> >>>> Hi all,
> >>>>
> >>>> This patch breaks ipsec tests with ipsec-secgw:
> >>>>
> >>>>
> >>>> ./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
> >>>> ...
> >>>> ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for
> >> dst=192.168.31.14,
> >>>> sz=1
> >>>>    test IPv4 trs_aesctr_sha1 finished with status 1
> >>>> ERROR  test trs_aesctr_sha1 FAILED
> >>>>
> >>> The patch seems to be correct.
> >>> Please check endianness in the PMD you are testing.
> >> In my opinion salt should not be affected by endianness and it should be
> >> used as it is in the key parameter. I think the patch is wrong to make
> >> it CPU endianness dependent before being passed to the PMDs, any PMD
> >> that needs the endianness swapped should do it in the PMD code. Indeed
> >> it's passed around as a 32 bit integer but it's not used as such, and
> >> when it's actually used it should be evaluated as a byte array.
> >>
> > As per the rfc, it should be treated as byte order(i.e. big endian).
> > But here the problem is we treat it as uint32_t which makes it CPU endian
> when stored in ipsec_sa struct.
> > The keys are stored as an array of uint8_t, so keys are stored in byte order(Big
> endian).
> >
> > So either we save salt as "uint8_t salt[4]" or do a conversion of cpu_to_be
> > So that when it is stored in PMD/HW, and we convert it from uint32_t to uint_8
> *, there wont be issue.
> 
> RFC treats it as a "four octet value" - there is no endianness until
> it's treated like an integer, which it never is. Even if it code it's
> being stored and passed as an unsigned 32bit integer it is never
> evaluated as such so its endianness doesn't matter.

The endianness matters the moment it is stored as uint32_t in ipsec_sa
It means the value is stored in CPU endianness in that integer unless it is specified.

Now looking at the code again, I see the patch is incomplete for the case of lookaside crypto
Where the salt is copied as cnt_blk in the mbuf priv without conversion.

So, this patch can be reverted and a simple fix can be added to mark ipsec_sa-> salt as rte_be32_t
diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
index a83fd2283b..1fe6b97168 100644
--- a/examples/ipsec-secgw/ipsec.h
+++ b/examples/ipsec-secgw/ipsec.h
@@ -117,7 +117,7 @@ struct __rte_cache_aligned ipsec_sa {
        uint32_t spi;
        struct cdev_qp *cqp[RTE_MAX_LCORE];
        uint64_t seq;
-       uint32_t salt;
+       rte_be32_t salt;
        uint32_t fallback_sessions;
        enum rte_crypto_cipher_algorithm cipher_algo;
        enum rte_crypto_auth_algorithm auth_algo;

Can you verify and send the patch?
And this may be updated in cryptodev and security lib as well in next release.


> 
> I agree that we should have it everywhere as "uint8_t salt[4]" but that
> implies API changes and it doesn't change how the bytes are stored, so
> the patch will still be wrong.
> 
> 
> >
> >>>
> >>>>
> >>>> On 03/07/2024 18:58, Akhil Goyal wrote:
> >>>>
> >>>>
> >>>>
> >>>>
> >>>>
> >>>> 		-----Original Message-----
> >>>> 		From: Akhil Goyal <gakhil@marvell.com>
> >>>> <mailto:gakhil@marvell.com>
> >>>> 		Sent: Friday, March 15, 2024 12:42 AM
> >>>> 		To: Akhil Goyal <gakhil@marvell.com>
> >>>> <mailto:gakhil@marvell.com> ; Chaoyong He
> >>>> 		<chaoyong.he@corigine.com>
> >>>> <mailto:chaoyong.he@corigine.com> ; dev@dpdk.org
> <mailto:dev@dpdk.org>
> >>>> 		Cc: oss-drivers@corigine.com <mailto:oss-
> >>>> drivers@corigine.com> ; Shihong Wang <shihong.wang@corigine.com>
> >>>> <mailto:shihong.wang@corigine.com> ;
> >>>> 		stable@dpdk.org <mailto:stable@dpdk.org>
> >>>> 		Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix
> >>>> SA salt
> >>>> 		endianness problem
> >>>>
> >>>>
> >>>> 			Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-
> >>>> secgw: fix SA salt
> >>>> 			endianness problem
> >>>>
> >>>>
> >>>> 				From: Shihong Wang
> >>>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
> >>>>
> >>>> 				The SA salt of struct ipsec_sa is a CPU-endian
> >>>> u32 variable, but it’s
> >>>> 				value is stored in an array of encryption or
> >>>> authentication keys
> >>>> 				according to big-endian. So it maybe need to
> >>>> convert the endianness
> >>>> 				order to ensure that the value assigned to the
> >>>> SA salt is CPU-endian.
> >>>>
> >>>> 				Fixes: 50d75cae2a2c ("examples/ipsec-secgw:
> >>>> initialize SA salt")
> >>>> 				Fixes: 9413c3901f31 ("examples/ipsec-secgw:
> >>>> support additional algorithms")
> >>>> 				Fixes: 501e9c226adf ("examples/ipsec-secgw:
> >>>> add AEAD parameters")
> >>>> 				Cc: stable@dpdk.org <mailto:stable@dpdk.org>
> >>>>
> >>>> 				Signed-off-by: Shihong Wang
> >>>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
> >>>> 				Reviewed-by: Chaoyong He
> >>>> <chaoyong.he@corigine.com> <mailto:chaoyong.he@corigine.com>
> >>>>
> >>>>
> >>>> 			Acked-by: Akhil Goyal <gakhil@marvell.com>
> >>>> <mailto:gakhil@marvell.com>
> >>>>
> >>>> 			Applied to dpdk-next-crypto
> >>>>
> >>>>
> >>>> 		The patch is pulled back from dpdk-next-crypto.
> >>>> 		This change may cause all the PMDs to fail these cases.
> >>>> 		Would need acks from PMDs.
> >>>>
> >>>>
> >>>> 	Applied to dpdk-next-crypto
> >>>> 	No update from PMD owners.
> >>>> 	Applying it before RC2 so that we have time for fixes if needed.
> >>>>
> >>>>
> >>>> --
> >>>> Regards,
> >>>> Vladimir

Re: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Radu Nicolau]

On 24-Jul-24 2:04 PM, Akhil Goyal wrote:
>> On 24-Jul-24 12:20 PM, Akhil Goyal wrote:
>>>> On 23-Jul-24 5:57 PM, Akhil Goyal wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This patch breaks ipsec tests with ipsec-secgw:
>>>>>>
>>>>>>
>>>>>> ./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
>>>>>> ...
>>>>>> ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for
>>>> dst=192.168.31.14,
>>>>>> sz=1
>>>>>>     test IPv4 trs_aesctr_sha1 finished with status 1
>>>>>> ERROR  test trs_aesctr_sha1 FAILED
>>>>>>
>>>>> The patch seems to be correct.
>>>>> Please check endianness in the PMD you are testing.
>>>> In my opinion salt should not be affected by endianness and it should be
>>>> used as it is in the key parameter. I think the patch is wrong to make
>>>> it CPU endianness dependent before being passed to the PMDs, any PMD
>>>> that needs the endianness swapped should do it in the PMD code. Indeed
>>>> it's passed around as a 32 bit integer but it's not used as such, and
>>>> when it's actually used it should be evaluated as a byte array.
>>>>
>>> As per the rfc, it should be treated as byte order(i.e. big endian).
>>> But here the problem is we treat it as uint32_t which makes it CPU endian
>> when stored in ipsec_sa struct.
>>> The keys are stored as an array of uint8_t, so keys are stored in byte order(Big
>> endian).
>>> So either we save salt as "uint8_t salt[4]" or do a conversion of cpu_to_be
>>> So that when it is stored in PMD/HW, and we convert it from uint32_t to uint_8
>> *, there wont be issue.
>>
>> RFC treats it as a "four octet value" - there is no endianness until
>> it's treated like an integer, which it never is. Even if it code it's
>> being stored and passed as an unsigned 32bit integer it is never
>> evaluated as such so its endianness doesn't matter.
> The endianness matters the moment it is stored as uint32_t in ipsec_sa
> It means the value is stored in CPU endianness in that integer unless it is specified.

What matters is that the four byte value in the key ends up in the 
memory in the same order, and that was always the case before the patch, 
regardless of the endianness of the CPU because load and store 
operations are not affected by endianness. With the patch the same four 
bytes from the configuration file will be stored differently in memory 
depending on the CPU. There is no need to fix the endianness of the 
salt, just as there is no need to fix the byte order of the key itself.

>
> Now looking at the code again, I see the patch is incomplete for the case of lookaside crypto
> Where the salt is copied as cnt_blk in the mbuf priv without conversion.
>
> So, this patch can be reverted and a simple fix can be added to mark ipsec_sa-> salt as rte_be32_t
> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> index a83fd2283b..1fe6b97168 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -117,7 +117,7 @@ struct __rte_cache_aligned ipsec_sa {
>          uint32_t spi;
>          struct cdev_qp *cqp[RTE_MAX_LCORE];
>          uint64_t seq;
> -       uint32_t salt;
> +       rte_be32_t salt;
>          uint32_t fallback_sessions;
>          enum rte_crypto_cipher_algorithm cipher_algo;
>          enum rte_crypto_auth_algorithm auth_algo;
>
> Can you verify and send the patch?
> And this may be updated in cryptodev and security lib as well in next release.
>
>
>> I agree that we should have it everywhere as "uint8_t salt[4]" but that
>> implies API changes and it doesn't change how the bytes are stored, so
>> the patch will still be wrong.
>>
>>
>>>>>> On 03/07/2024 18:58, Akhil Goyal wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> 		-----Original Message-----
>>>>>> 		From: Akhil Goyal <gakhil@marvell.com>
>>>>>> <mailto:gakhil@marvell.com>
>>>>>> 		Sent: Friday, March 15, 2024 12:42 AM
>>>>>> 		To: Akhil Goyal <gakhil@marvell.com>
>>>>>> <mailto:gakhil@marvell.com> ; Chaoyong He
>>>>>> 		<chaoyong.he@corigine.com>
>>>>>> <mailto:chaoyong.he@corigine.com> ; dev@dpdk.org
>> <mailto:dev@dpdk.org>
>>>>>> 		Cc: oss-drivers@corigine.com <mailto:oss-
>>>>>> drivers@corigine.com> ; Shihong Wang <shihong.wang@corigine.com>
>>>>>> <mailto:shihong.wang@corigine.com> ;
>>>>>> 		stable@dpdk.org <mailto:stable@dpdk.org>
>>>>>> 		Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix
>>>>>> SA salt
>>>>>> 		endianness problem
>>>>>>
>>>>>>
>>>>>> 			Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-
>>>>>> secgw: fix SA salt
>>>>>> 			endianness problem
>>>>>>
>>>>>>
>>>>>> 				From: Shihong Wang
>>>>>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
>>>>>>
>>>>>> 				The SA salt of struct ipsec_sa is a CPU-endian
>>>>>> u32 variable, but it’s
>>>>>> 				value is stored in an array of encryption or
>>>>>> authentication keys
>>>>>> 				according to big-endian. So it maybe need to
>>>>>> convert the endianness
>>>>>> 				order to ensure that the value assigned to the
>>>>>> SA salt is CPU-endian.
>>>>>>
>>>>>> 				Fixes: 50d75cae2a2c ("examples/ipsec-secgw:
>>>>>> initialize SA salt")
>>>>>> 				Fixes: 9413c3901f31 ("examples/ipsec-secgw:
>>>>>> support additional algorithms")
>>>>>> 				Fixes: 501e9c226adf ("examples/ipsec-secgw:
>>>>>> add AEAD parameters")
>>>>>> 				Cc: stable@dpdk.org <mailto:stable@dpdk.org>
>>>>>>
>>>>>> 				Signed-off-by: Shihong Wang
>>>>>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
>>>>>> 				Reviewed-by: Chaoyong He
>>>>>> <chaoyong.he@corigine.com> <mailto:chaoyong.he@corigine.com>
>>>>>>
>>>>>>
>>>>>> 			Acked-by: Akhil Goyal <gakhil@marvell.com>
>>>>>> <mailto:gakhil@marvell.com>
>>>>>>
>>>>>> 			Applied to dpdk-next-crypto
>>>>>>
>>>>>>
>>>>>> 		The patch is pulled back from dpdk-next-crypto.
>>>>>> 		This change may cause all the PMDs to fail these cases.
>>>>>> 		Would need acks from PMDs.
>>>>>>
>>>>>>
>>>>>> 	Applied to dpdk-next-crypto
>>>>>> 	No update from PMD owners.
>>>>>> 	Applying it before RC2 so that we have time for fixes if needed.
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Regards,
>>>>>> Vladimir

RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Akhil Goyal]

> On 24-Jul-24 2:04 PM, Akhil Goyal wrote:
> >> On 24-Jul-24 12:20 PM, Akhil Goyal wrote:
> >>>> On 23-Jul-24 5:57 PM, Akhil Goyal wrote:
> >>>>>> Hi all,
> >>>>>>
> >>>>>> This patch breaks ipsec tests with ipsec-secgw:
> >>>>>>
> >>>>>>
> >>>>>> ./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
> >>>>>> ...
> >>>>>> ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for
> >>>> dst=192.168.31.14,
> >>>>>> sz=1
> >>>>>>     test IPv4 trs_aesctr_sha1 finished with status 1
> >>>>>> ERROR  test trs_aesctr_sha1 FAILED
> >>>>>>
> >>>>> The patch seems to be correct.
> >>>>> Please check endianness in the PMD you are testing.
> >>>> In my opinion salt should not be affected by endianness and it should be
> >>>> used as it is in the key parameter. I think the patch is wrong to make
> >>>> it CPU endianness dependent before being passed to the PMDs, any PMD
> >>>> that needs the endianness swapped should do it in the PMD code. Indeed
> >>>> it's passed around as a 32 bit integer but it's not used as such, and
> >>>> when it's actually used it should be evaluated as a byte array.
> >>>>
> >>> As per the rfc, it should be treated as byte order(i.e. big endian).
> >>> But here the problem is we treat it as uint32_t which makes it CPU endian
> >> when stored in ipsec_sa struct.
> >>> The keys are stored as an array of uint8_t, so keys are stored in byte
> order(Big
> >> endian).
> >>> So either we save salt as "uint8_t salt[4]" or do a conversion of cpu_to_be
> >>> So that when it is stored in PMD/HW, and we convert it from uint32_t to
> uint_8
> >> *, there wont be issue.
> >>
> >> RFC treats it as a "four octet value" - there is no endianness until
> >> it's treated like an integer, which it never is. Even if it code it's
> >> being stored and passed as an unsigned 32bit integer it is never
> >> evaluated as such so its endianness doesn't matter.
> > The endianness matters the moment it is stored as uint32_t in ipsec_sa
> > It means the value is stored in CPU endianness in that integer unless it is
> specified.
> 
> What matters is that the four byte value in the key ends up in the
> memory in the same order, and that was always the case before the patch,
> regardless of the endianness of the CPU because load and store
> operations are not affected by endianness. With the patch the same four
> bytes from the configuration file will be stored differently in memory
> depending on the CPU. There is no need to fix the endianness of the
> salt, just as there is no need to fix the byte order of the key itself.
> 

When a uint32 is used, there is no clarity whether it is in BE or LE.
So that is where the confusion comes.
For any new person, uint32 means it is in CPU byte order.
This patch tried to address that but it failed to address all the cases.
So my simple suggestion is instead of fixing the byte order at every place,
Lets just change the ipsec_sa->salt as rte_be32_t from uint32_t and revert this patch.
This will make things clear.
I am suggesting to convert this uint32_t to rte_be32_t for library as well for salt.
This change is not swapping anything, but making things explicitly clear that salt is in BE.

> >
> > Now looking at the code again, I see the patch is incomplete for the case of
> lookaside crypto
> > Where the salt is copied as cnt_blk in the mbuf priv without conversion.
> >
> > So, this patch can be reverted and a simple fix can be added to mark ipsec_sa->
> salt as rte_be32_t
> > diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
> > index a83fd2283b..1fe6b97168 100644
> > --- a/examples/ipsec-secgw/ipsec.h
> > +++ b/examples/ipsec-secgw/ipsec.h
> > @@ -117,7 +117,7 @@ struct __rte_cache_aligned ipsec_sa {
> >          uint32_t spi;
> >          struct cdev_qp *cqp[RTE_MAX_LCORE];
> >          uint64_t seq;
> > -       uint32_t salt;
> > +       rte_be32_t salt;
> >          uint32_t fallback_sessions;
> >          enum rte_crypto_cipher_algorithm cipher_algo;
> >          enum rte_crypto_auth_algorithm auth_algo;
> >
> > Can you verify and send the patch?
> > And this may be updated in cryptodev and security lib as well in next release.
> >
> >
> >> I agree that we should have it everywhere as "uint8_t salt[4]" but that
> >> implies API changes and it doesn't change how the bytes are stored, so
> >> the patch will still be wrong.
> >>
> >>
> >>>>>> On 03/07/2024 18:58, Akhil Goyal wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> 		-----Original Message-----
> >>>>>> 		From: Akhil Goyal <gakhil@marvell.com>
> >>>>>> <mailto:gakhil@marvell.com>
> >>>>>> 		Sent: Friday, March 15, 2024 12:42 AM
> >>>>>> 		To: Akhil Goyal <gakhil@marvell.com>
> >>>>>> <mailto:gakhil@marvell.com> ; Chaoyong He
> >>>>>> 		<chaoyong.he@corigine.com>
> >>>>>> <mailto:chaoyong.he@corigine.com> ; dev@dpdk.org
> >> <mailto:dev@dpdk.org>
> >>>>>> 		Cc: oss-drivers@corigine.com <mailto:oss-
> >>>>>> drivers@corigine.com> ; Shihong Wang <shihong.wang@corigine.com>
> >>>>>> <mailto:shihong.wang@corigine.com> ;
> >>>>>> 		stable@dpdk.org <mailto:stable@dpdk.org>
> >>>>>> 		Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix
> >>>>>> SA salt
> >>>>>> 		endianness problem
> >>>>>>
> >>>>>>
> >>>>>> 			Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-
> >>>>>> secgw: fix SA salt
> >>>>>> 			endianness problem
> >>>>>>
> >>>>>>
> >>>>>> 				From: Shihong Wang
> >>>>>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
> >>>>>>
> >>>>>> 				The SA salt of struct ipsec_sa is a CPU-endian
> >>>>>> u32 variable, but it’s
> >>>>>> 				value is stored in an array of encryption or
> >>>>>> authentication keys
> >>>>>> 				according to big-endian. So it maybe need to
> >>>>>> convert the endianness
> >>>>>> 				order to ensure that the value assigned to the
> >>>>>> SA salt is CPU-endian.
> >>>>>>
> >>>>>> 				Fixes: 50d75cae2a2c ("examples/ipsec-secgw:
> >>>>>> initialize SA salt")
> >>>>>> 				Fixes: 9413c3901f31 ("examples/ipsec-secgw:
> >>>>>> support additional algorithms")
> >>>>>> 				Fixes: 501e9c226adf ("examples/ipsec-secgw:
> >>>>>> add AEAD parameters")
> >>>>>> 				Cc: stable@dpdk.org <mailto:stable@dpdk.org>
> >>>>>>
> >>>>>> 				Signed-off-by: Shihong Wang
> >>>>>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
> >>>>>> 				Reviewed-by: Chaoyong He
> >>>>>> <chaoyong.he@corigine.com> <mailto:chaoyong.he@corigine.com>
> >>>>>>
> >>>>>>
> >>>>>> 			Acked-by: Akhil Goyal <gakhil@marvell.com>
> >>>>>> <mailto:gakhil@marvell.com>
> >>>>>>
> >>>>>> 			Applied to dpdk-next-crypto
> >>>>>>
> >>>>>>
> >>>>>> 		The patch is pulled back from dpdk-next-crypto.
> >>>>>> 		This change may cause all the PMDs to fail these cases.
> >>>>>> 		Would need acks from PMDs.
> >>>>>>
> >>>>>>
> >>>>>> 	Applied to dpdk-next-crypto
> >>>>>> 	No update from PMD owners.
> >>>>>> 	Applying it before RC2 so that we have time for fixes if needed.
> >>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> Regards,
> >>>>>> Vladimir

Re: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Radu Nicolau]

On 25-Jul-24 5:47 AM, Akhil Goyal wrote:
>> On 24-Jul-24 2:04 PM, Akhil Goyal wrote:
>>>> On 24-Jul-24 12:20 PM, Akhil Goyal wrote:
>>>>>> On 23-Jul-24 5:57 PM, Akhil Goyal wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> This patch breaks ipsec tests with ipsec-secgw:
>>>>>>>>
>>>>>>>>
>>>>>>>> ./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
>>>>>>>> ...
>>>>>>>> ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for
>>>>>> dst=192.168.31.14,
>>>>>>>> sz=1
>>>>>>>>      test IPv4 trs_aesctr_sha1 finished with status 1
>>>>>>>> ERROR  test trs_aesctr_sha1 FAILED
>>>>>>>>
>>>>>>> The patch seems to be correct.
>>>>>>> Please check endianness in the PMD you are testing.
>>>>>> In my opinion salt should not be affected by endianness and it should be
>>>>>> used as it is in the key parameter. I think the patch is wrong to make
>>>>>> it CPU endianness dependent before being passed to the PMDs, any PMD
>>>>>> that needs the endianness swapped should do it in the PMD code. Indeed
>>>>>> it's passed around as a 32 bit integer but it's not used as such, and
>>>>>> when it's actually used it should be evaluated as a byte array.
>>>>>>
>>>>> As per the rfc, it should be treated as byte order(i.e. big endian).
>>>>> But here the problem is we treat it as uint32_t which makes it CPU endian
>>>> when stored in ipsec_sa struct.
>>>>> The keys are stored as an array of uint8_t, so keys are stored in byte
>> order(Big
>>>> endian).
>>>>> So either we save salt as "uint8_t salt[4]" or do a conversion of cpu_to_be
>>>>> So that when it is stored in PMD/HW, and we convert it from uint32_t to
>> uint_8
>>>> *, there wont be issue.
>>>>
>>>> RFC treats it as a "four octet value" - there is no endianness until
>>>> it's treated like an integer, which it never is. Even if it code it's
>>>> being stored and passed as an unsigned 32bit integer it is never
>>>> evaluated as such so its endianness doesn't matter.
>>> The endianness matters the moment it is stored as uint32_t in ipsec_sa
>>> It means the value is stored in CPU endianness in that integer unless it is
>> specified.
>>
>> What matters is that the four byte value in the key ends up in the
>> memory in the same order, and that was always the case before the patch,
>> regardless of the endianness of the CPU because load and store
>> operations are not affected by endianness. With the patch the same four
>> bytes from the configuration file will be stored differently in memory
>> depending on the CPU. There is no need to fix the endianness of the
>> salt, just as there is no need to fix the byte order of the key itself.
>>
> When a uint32 is used, there is no clarity whether it is in BE or LE.
> So that is where the confusion comes.
> For any new person, uint32 means it is in CPU byte order.
> This patch tried to address that but it failed to address all the cases.
> So my simple suggestion is instead of fixing the byte order at every place,
> Lets just change the ipsec_sa->salt as rte_be32_t from uint32_t and revert this patch.
> This will make things clear.
> I am suggesting to convert this uint32_t to rte_be32_t for library as well for salt.
> This change is not swapping anything, but making things explicitly clear that salt is in BE.

I agree with the suggestion of using rte_be32_t and reverting the patch.

(I still think it would be even better to use uint8_t[4] to reflect that 
it is a four byte value with no endianness but that's just IMHO, the 
important thing is to revert the patch that broke the functionality)


>
>>> Now looking at the code again, I see the patch is incomplete for the case of
>> lookaside crypto
>>> Where the salt is copied as cnt_blk in the mbuf priv without conversion.
>>>
>>> So, this patch can be reverted and a simple fix can be added to mark ipsec_sa->
>> salt as rte_be32_t
>>> diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
>>> index a83fd2283b..1fe6b97168 100644
>>> --- a/examples/ipsec-secgw/ipsec.h
>>> +++ b/examples/ipsec-secgw/ipsec.h
>>> @@ -117,7 +117,7 @@ struct __rte_cache_aligned ipsec_sa {
>>>           uint32_t spi;
>>>           struct cdev_qp *cqp[RTE_MAX_LCORE];
>>>           uint64_t seq;
>>> -       uint32_t salt;
>>> +       rte_be32_t salt;
>>>           uint32_t fallback_sessions;
>>>           enum rte_crypto_cipher_algorithm cipher_algo;
>>>           enum rte_crypto_auth_algorithm auth_algo;
>>>
>>> Can you verify and send the patch?
>>> And this may be updated in cryptodev and security lib as well in next release.
>>>
>>>
>>>> I agree that we should have it everywhere as "uint8_t salt[4]" but that
>>>> implies API changes and it doesn't change how the bytes are stored, so
>>>> the patch will still be wrong.
>>>>
>>>>
>>>>>>>> On 03/07/2024 18:58, Akhil Goyal wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> 		-----Original Message-----
>>>>>>>> 		From: Akhil Goyal <gakhil@marvell.com>
>>>>>>>> <mailto:gakhil@marvell.com>
>>>>>>>> 		Sent: Friday, March 15, 2024 12:42 AM
>>>>>>>> 		To: Akhil Goyal <gakhil@marvell.com>
>>>>>>>> <mailto:gakhil@marvell.com> ; Chaoyong He
>>>>>>>> 		<chaoyong.he@corigine.com>
>>>>>>>> <mailto:chaoyong.he@corigine.com> ; dev@dpdk.org
>>>> <mailto:dev@dpdk.org>
>>>>>>>> 		Cc: oss-drivers@corigine.com <mailto:oss-
>>>>>>>> drivers@corigine.com> ; Shihong Wang <shihong.wang@corigine.com>
>>>>>>>> <mailto:shihong.wang@corigine.com> ;
>>>>>>>> 		stable@dpdk.org <mailto:stable@dpdk.org>
>>>>>>>> 		Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix
>>>>>>>> SA salt
>>>>>>>> 		endianness problem
>>>>>>>>
>>>>>>>>
>>>>>>>> 			Subject: RE: [EXTERNAL] [PATCH v2] examples/ipsec-
>>>>>>>> secgw: fix SA salt
>>>>>>>> 			endianness problem
>>>>>>>>
>>>>>>>>
>>>>>>>> 				From: Shihong Wang
>>>>>>>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
>>>>>>>>
>>>>>>>> 				The SA salt of struct ipsec_sa is a CPU-endian
>>>>>>>> u32 variable, but it’s
>>>>>>>> 				value is stored in an array of encryption or
>>>>>>>> authentication keys
>>>>>>>> 				according to big-endian. So it maybe need to
>>>>>>>> convert the endianness
>>>>>>>> 				order to ensure that the value assigned to the
>>>>>>>> SA salt is CPU-endian.
>>>>>>>>
>>>>>>>> 				Fixes: 50d75cae2a2c ("examples/ipsec-secgw:
>>>>>>>> initialize SA salt")
>>>>>>>> 				Fixes: 9413c3901f31 ("examples/ipsec-secgw:
>>>>>>>> support additional algorithms")
>>>>>>>> 				Fixes: 501e9c226adf ("examples/ipsec-secgw:
>>>>>>>> add AEAD parameters")
>>>>>>>> 				Cc: stable@dpdk.org <mailto:stable@dpdk.org>
>>>>>>>>
>>>>>>>> 				Signed-off-by: Shihong Wang
>>>>>>>> <shihong.wang@corigine.com> <mailto:shihong.wang@corigine.com>
>>>>>>>> 				Reviewed-by: Chaoyong He
>>>>>>>> <chaoyong.he@corigine.com> <mailto:chaoyong.he@corigine.com>
>>>>>>>>
>>>>>>>>
>>>>>>>> 			Acked-by: Akhil Goyal <gakhil@marvell.com>
>>>>>>>> <mailto:gakhil@marvell.com>
>>>>>>>>
>>>>>>>> 			Applied to dpdk-next-crypto
>>>>>>>>
>>>>>>>>
>>>>>>>> 		The patch is pulled back from dpdk-next-crypto.
>>>>>>>> 		This change may cause all the PMDs to fail these cases.
>>>>>>>> 		Would need acks from PMDs.
>>>>>>>>
>>>>>>>>
>>>>>>>> 	Applied to dpdk-next-crypto
>>>>>>>> 	No update from PMD owners.
>>>>>>>> 	Applying it before RC2 so that we have time for fixes if needed.
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Regards,
>>>>>>>> Vladimir

RE: [EXTERNAL] [PATCH v2] examples/ipsec-secgw: fix SA salt endianness problem

[Akhil Goyal]

> 
> On 25-Jul-24 5:47 AM, Akhil Goyal wrote:
> >> On 24-Jul-24 2:04 PM, Akhil Goyal wrote:
> >>>> On 24-Jul-24 12:20 PM, Akhil Goyal wrote:
> >>>>>> On 23-Jul-24 5:57 PM, Akhil Goyal wrote:
> >>>>>>>> Hi all,
> >>>>>>>>
> >>>>>>>> This patch breaks ipsec tests with ipsec-secgw:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> ./examples/ipsec-secgw/test/run_test.sh -4 trs_aesctr_sha1
> >>>>>>>> ...
> >>>>>>>> ERROR: ./examples/ipsec-secgw/test/linux_test.sh failed for
> >>>>>> dst=192.168.31.14,
> >>>>>>>> sz=1
> >>>>>>>>      test IPv4 trs_aesctr_sha1 finished with status 1
> >>>>>>>> ERROR  test trs_aesctr_sha1 FAILED
> >>>>>>>>
> >>>>>>> The patch seems to be correct.
> >>>>>>> Please check endianness in the PMD you are testing.
> >>>>>> In my opinion salt should not be affected by endianness and it should be
> >>>>>> used as it is in the key parameter. I think the patch is wrong to make
> >>>>>> it CPU endianness dependent before being passed to the PMDs, any PMD
> >>>>>> that needs the endianness swapped should do it in the PMD code. Indeed
> >>>>>> it's passed around as a 32 bit integer but it's not used as such, and
> >>>>>> when it's actually used it should be evaluated as a byte array.
> >>>>>>
> >>>>> As per the rfc, it should be treated as byte order(i.e. big endian).
> >>>>> But here the problem is we treat it as uint32_t which makes it CPU endian
> >>>> when stored in ipsec_sa struct.
> >>>>> The keys are stored as an array of uint8_t, so keys are stored in byte
> >> order(Big
> >>>> endian).
> >>>>> So either we save salt as "uint8_t salt[4]" or do a conversion of cpu_to_be
> >>>>> So that when it is stored in PMD/HW, and we convert it from uint32_t to
> >> uint_8
> >>>> *, there wont be issue.
> >>>>
> >>>> RFC treats it as a "four octet value" - there is no endianness until
> >>>> it's treated like an integer, which it never is. Even if it code it's
> >>>> being stored and passed as an unsigned 32bit integer it is never
> >>>> evaluated as such so its endianness doesn't matter.
> >>> The endianness matters the moment it is stored as uint32_t in ipsec_sa
> >>> It means the value is stored in CPU endianness in that integer unless it is
> >> specified.
> >>
> >> What matters is that the four byte value in the key ends up in the
> >> memory in the same order, and that was always the case before the patch,
> >> regardless of the endianness of the CPU because load and store
> >> operations are not affected by endianness. With the patch the same four
> >> bytes from the configuration file will be stored differently in memory
> >> depending on the CPU. There is no need to fix the endianness of the
> >> salt, just as there is no need to fix the byte order of the key itself.
> >>
> > When a uint32 is used, there is no clarity whether it is in BE or LE.
> > So that is where the confusion comes.
> > For any new person, uint32 means it is in CPU byte order.
> > This patch tried to address that but it failed to address all the cases.
> > So my simple suggestion is instead of fixing the byte order at every place,
> > Lets just change the ipsec_sa->salt as rte_be32_t from uint32_t and revert this
> patch.
> > This will make things clear.
> > I am suggesting to convert this uint32_t to rte_be32_t for library as well for salt.
> > This change is not swapping anything, but making things explicitly clear that salt
> is in BE.
> 
> I agree with the suggestion of using rte_be32_t and reverting the patch.

Can you send a patch with both the changes?
> 
> (I still think it would be even better to use uint8_t[4] to reflect that
> it is a four byte value with no endianness but that's just IMHO, the
> important thing is to revert the patch that broke the functionality)
> 
That can be for next release in library.