[dpdk-dev] [PATCH 1/3] crypto/octeontx2: fix member overlap

[dpdk-dev] [PATCH 1/3] crypto/octeontx2: fix member overlap

[Anoob Joseph]

The member 'dir' should not overlap with 'ip'. Usage of union for all
members would mean dir would get corrupt.

Fixes: e91b4f45ff54 ("net/octeontx2: support anti-replay for security session")
Cc: adwivedi@marvell.com

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
---
 drivers/crypto/octeontx2/otx2_security.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/crypto/octeontx2/otx2_security.h b/drivers/crypto/octeontx2/otx2_security.h
index 9f1ba71..29c8fc3 100644
--- a/drivers/crypto/octeontx2/otx2_security.h
+++ b/drivers/crypto/octeontx2/otx2_security.h
@@ -20,14 +20,16 @@
 #define OTX2_SEC_AES_GCM_ROUNDUP_BYTE_LEN	4
 #define OTX2_SEC_AES_CBC_ROUNDUP_BYTE_LEN	16
 
-union otx2_sec_session_ipsec {
-	struct otx2_sec_session_ipsec_ip ip;
-	struct otx2_sec_session_ipsec_lp lp;
+struct otx2_sec_session_ipsec {
+	union {
+		struct otx2_sec_session_ipsec_ip ip;
+		struct otx2_sec_session_ipsec_lp lp;
+	};
 	enum rte_security_ipsec_sa_direction dir;
 };
 
 struct otx2_sec_session {
-	union otx2_sec_session_ipsec ipsec;
+	struct otx2_sec_session_ipsec ipsec;
 	void *userdata;
 	/**< Userdata registered by the application */
 } __rte_cache_aligned;
-- 
2.7.4

[dpdk-dev] [PATCH 2/3] net/octeontx2: add locking for inline IPsec tbl updates

[Anoob Joseph]

Add locking for IPsec table updates.

Fixed error handling to clear SA entry if the SA
population functions encounters any error.

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
---
 drivers/net/octeontx2/otx2_ethdev.h     |  2 +
 drivers/net/octeontx2/otx2_ethdev_sec.c | 83 +++++++++++++++++++++++++--------
 2 files changed, 66 insertions(+), 19 deletions(-)

diff --git a/drivers/net/octeontx2/otx2_ethdev.h b/drivers/net/octeontx2/otx2_ethdev.h
index e95d933..7871e3d 100644
--- a/drivers/net/octeontx2/otx2_ethdev.h
+++ b/drivers/net/octeontx2/otx2_ethdev.h
@@ -14,6 +14,7 @@
 #include <rte_mbuf.h>
 #include <rte_mempool.h>
 #include <rte_security_driver.h>
+#include <rte_spinlock.h>
 #include <rte_string_fns.h>
 #include <rte_time.h>
 
@@ -356,6 +357,7 @@ struct otx2_eth_dev {
 	bool sdp_link; /* SDP flag */
 	/* Inline IPsec params */
 	uint16_t ipsec_in_max_spi;
+	rte_spinlock_t ipsec_tbl_lock;
 	uint8_t duplex;
 	uint32_t speed;
 } __rte_cache_aligned;
diff --git a/drivers/net/octeontx2/otx2_ethdev_sec.c b/drivers/net/octeontx2/otx2_ethdev_sec.c
index 1ee597f..72298cf 100644
--- a/drivers/net/octeontx2/otx2_ethdev_sec.c
+++ b/drivers/net/octeontx2/otx2_ethdev_sec.c
@@ -21,6 +21,8 @@
 #include "otx2_sec_idev.h"
 #include "otx2_security.h"
 
+#define ERR_STR_SZ 256
+
 struct eth_sec_tag_const {
 	RTE_STD_C11
 	union {
@@ -162,7 +164,8 @@ lookup_mem_sa_tbl_clear(struct rte_eth_dev *eth_dev)
 }
 
 static int
-lookup_mem_sa_index_update(struct rte_eth_dev *eth_dev, int spi, void *sa)
+lookup_mem_sa_index_update(struct rte_eth_dev *eth_dev, int spi, void *sa,
+			   char *err_str)
 {
 	static const char name[] = OTX2_NIX_FASTPATH_LOOKUP_MEM;
 	struct otx2_eth_dev *dev = otx2_eth_pmd_priv(eth_dev);
@@ -173,7 +176,8 @@ lookup_mem_sa_index_update(struct rte_eth_dev *eth_dev, int spi, void *sa)
 
 	mz = rte_memzone_lookup(name);
 	if (mz == NULL) {
-		otx2_err("Could not find fastpath lookup table");
+		snprintf(err_str, ERR_STR_SZ,
+			 "Could not find fastpath lookup table");
 		return -EINVAL;
 	}
 
@@ -472,23 +476,32 @@ eth_sec_ipsec_in_sess_create(struct rte_eth_dev *eth_dev,
 	struct otx2_ipsec_fp_sa_ctl *ctl;
 	struct otx2_ipsec_fp_in_sa *sa;
 	struct otx2_sec_session *priv;
+	char err_str[ERR_STR_SZ];
 	struct otx2_cpt_qp *qp;
 
+	memset(err_str, 0, ERR_STR_SZ);
+
 	if (ipsec->spi >= dev->ipsec_in_max_spi) {
 		otx2_err("SPI exceeds max supported");
 		return -EINVAL;
 	}
 
 	sa = in_sa_get(port, ipsec->spi);
+	if (sa == NULL)
+		return -ENOMEM;
+
 	ctl = &sa->ctl;
 
 	priv = get_sec_session_private_data(sec_sess);
 	priv->ipsec.dir = RTE_SECURITY_IPSEC_SA_DIR_INGRESS;
 	sess = &priv->ipsec.ip;
 
+	rte_spinlock_lock(&dev->ipsec_tbl_lock);
+
 	if (ctl->valid) {
-		otx2_err("SA already registered");
-		return -EINVAL;
+		snprintf(err_str, ERR_STR_SZ, "SA already registered");
+		ret = -EEXIST;
+		goto tbl_unlock;
 	}
 
 	memset(sa, 0, sizeof(struct otx2_ipsec_fp_in_sa));
@@ -512,10 +525,13 @@ eth_sec_ipsec_in_sess_create(struct rte_eth_dev *eth_dev,
 		auth_key_len = auth_xform->auth.key.length;
 	}
 
-	if (cipher_key_len != 0)
+	if (cipher_key_len != 0) {
 		memcpy(sa->cipher_key, cipher_key, cipher_key_len);
-	else
-		return -EINVAL;
+	} else {
+		snprintf(err_str, ERR_STR_SZ, "Invalid cipher key len");
+		ret = -EINVAL;
+		goto sa_clear;
+	}
 
 	sess->in_sa = sa;
 
@@ -523,33 +539,49 @@ eth_sec_ipsec_in_sess_create(struct rte_eth_dev *eth_dev,
 
 	sa->replay_win_sz = ipsec->replay_win_sz;
 
-	if (lookup_mem_sa_index_update(eth_dev, ipsec->spi, sa))
-		return -EINVAL;
+	if (lookup_mem_sa_index_update(eth_dev, ipsec->spi, sa, err_str)) {
+		ret = -EINVAL;
+		goto sa_clear;
+	}
 
 	ret = ipsec_fp_sa_ctl_set(ipsec, crypto_xform, ctl);
-	if (ret)
-		return ret;
+	if (ret) {
+		snprintf(err_str, ERR_STR_SZ,
+			"Could not set SA CTL word (err: %d)", ret);
+		goto sa_clear;
+	}
 
 	if (auth_key_len && auth_key) {
 		/* Get a queue pair for HMAC init */
 		ret = otx2_sec_idev_tx_cpt_qp_get(port, &qp);
-		if (ret)
-			return ret;
+		if (ret) {
+			snprintf(err_str, ERR_STR_SZ, "Could not get CPT QP");
+			goto sa_clear;
+		}
+
 		ret = hmac_init(ctl, qp, auth_key, auth_key_len, sa->hmac_key);
 		otx2_sec_idev_tx_cpt_qp_put(qp);
-		if (ret)
-			return ret;
+		if (ret) {
+			snprintf(err_str, ERR_STR_SZ, "Could not put CPT QP");
+			goto sa_clear;
+		}
 	}
 
 	if (sa->replay_win_sz) {
 		if (sa->replay_win_sz > OTX2_IPSEC_MAX_REPLAY_WIN_SZ) {
-			otx2_err("Replay window size is not supported");
-			return -ENOTSUP;
+			snprintf(err_str, ERR_STR_SZ,
+				 "Replay window size is not supported");
+			ret = -ENOTSUP;
+			goto sa_clear;
 		}
 		sa->replay = rte_zmalloc(NULL, sizeof(struct otx2_ipsec_replay),
 				0);
-		if (sa->replay == NULL)
-			return -ENOMEM;
+		if (sa->replay == NULL) {
+			snprintf(err_str, ERR_STR_SZ,
+				"Could not allocate memory");
+			ret = -ENOMEM;
+			goto sa_clear;
+		}
 
 		rte_spinlock_init(&sa->replay->lock);
 		/*
@@ -563,6 +595,17 @@ eth_sec_ipsec_in_sess_create(struct rte_eth_dev *eth_dev,
 		sa->esn_hi = 0;
 	}
 
+	rte_spinlock_unlock(&dev->ipsec_tbl_lock);
+	return 0;
+
+sa_clear:
+	memset(sa, 0, sizeof(struct otx2_ipsec_fp_in_sa));
+
+tbl_unlock:
+	rte_spinlock_unlock(&dev->ipsec_tbl_lock);
+
+	otx2_err("%s", err_str);
+
 	return ret;
 }
 
@@ -853,6 +896,8 @@ otx2_eth_sec_init(struct rte_eth_dev *eth_dev)
 		goto sec_fini;
 	}
 
+	rte_spinlock_init(&dev->ipsec_tbl_lock);
+
 	return 0;
 
 sec_fini:
-- 
2.7.4

[dpdk-dev] [PATCH 3/3] net/octeontx2: clear SA valid during session destroy

[Anoob Joseph]

SA table entry would be reserved for inline inbound operations. Clear
valid bit of the SA so that CPT would treat SA entry as invalid. Also,
move setting of valid bit to the end in case of session_create() to
eliminate possibility of hardware seeing partial data.

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
---
 drivers/crypto/octeontx2/otx2_ipsec_fp.h |  1 -
 drivers/net/octeontx2/otx2_ethdev_sec.c  | 28 ++++++++++++++++++++++++----
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/crypto/octeontx2/otx2_ipsec_fp.h b/drivers/crypto/octeontx2/otx2_ipsec_fp.h
index a33041d..58b24a2 100644
--- a/drivers/crypto/octeontx2/otx2_ipsec_fp.h
+++ b/drivers/crypto/octeontx2/otx2_ipsec_fp.h
@@ -365,7 +365,6 @@ ipsec_fp_sa_ctl_set(struct rte_security_ipsec_xform *ipsec,
 		ctl->esn_en = 1;
 
 	ctl->spi = rte_cpu_to_be_32(ipsec->spi);
-	ctl->valid = 1;
 
 	return 0;
 }
diff --git a/drivers/net/octeontx2/otx2_ethdev_sec.c b/drivers/net/octeontx2/otx2_ethdev_sec.c
index 72298cf..c2a3688 100644
--- a/drivers/net/octeontx2/otx2_ethdev_sec.c
+++ b/drivers/net/octeontx2/otx2_ethdev_sec.c
@@ -455,6 +455,9 @@ eth_sec_ipsec_out_sess_create(struct rte_eth_dev *eth_dev,
 			goto cpt_put;
 	}
 
+	rte_io_wmb();
+	ctl->valid = 1;
+
 	return 0;
 cpt_put:
 	otx2_sec_idev_tx_cpt_qp_put(sess->qp);
@@ -595,6 +598,9 @@ eth_sec_ipsec_in_sess_create(struct rte_eth_dev *eth_dev,
 		sa->esn_hi = 0;
 	}
 
+	rte_io_wmb();
+	ctl->valid = 1;
+
 	rte_spinlock_unlock(&dev->ipsec_tbl_lock);
 	return 0;
 
@@ -682,10 +688,12 @@ otx2_eth_sec_free_anti_replay(struct otx2_ipsec_fp_in_sa *sa)
 }
 
 static int
-otx2_eth_sec_session_destroy(void *device __rte_unused,
+otx2_eth_sec_session_destroy(void *device,
 			     struct rte_security_session *sess)
 {
+	struct otx2_eth_dev *dev = otx2_eth_pmd_priv(device);
 	struct otx2_sec_session_ipsec_ip *sess_ip;
+	struct otx2_ipsec_fp_in_sa *sa;
 	struct otx2_sec_session *priv;
 	struct rte_mempool *sess_mp;
 	int ret;
@@ -696,9 +704,21 @@ otx2_eth_sec_session_destroy(void *device __rte_unused,
 
 	sess_ip = &priv->ipsec.ip;
 
-	/* Release the anti replay window */
-	if (priv->ipsec.dir == RTE_SECURITY_IPSEC_SA_DIR_INGRESS)
-		otx2_eth_sec_free_anti_replay(sess_ip->in_sa);
+	if (priv->ipsec.dir == RTE_SECURITY_IPSEC_SA_DIR_INGRESS) {
+		rte_spinlock_lock(&dev->ipsec_tbl_lock);
+		sa = sess_ip->in_sa;
+
+		/* Release the anti replay window */
+		otx2_eth_sec_free_anti_replay(sa);
+
+		/* Clear SA table entry */
+		if (sa != NULL) {
+			sa->ctl.valid = 0;
+			rte_io_wmb();
+		}
+
+		rte_spinlock_unlock(&dev->ipsec_tbl_lock);
+	}
 
 	/* Release CPT LF used for this session */
 	if (sess_ip->qp != NULL) {
-- 
2.7.4

Re: [dpdk-dev] [PATCH 1/3] crypto/octeontx2: fix member overlap

[Akhil Goyal]

> The member 'dir' should not overlap with 'ip'. Usage of union for all
> members would mean dir would get corrupt.
> 
> Fixes: e91b4f45ff54 ("net/octeontx2: support anti-replay for security
> session")
> Cc: adwivedi@marvell.com
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
Cc: stable@dpdk.org

Series
Acked-by: Akhil Goyal <gakhil@marvell.com>

Applied to dpdk-next-crypto

Thanks.