[dpdk-dev] [PATCH 0/2] fixes for inline-crypto ipsec

[dpdk-dev] [PATCH 0/2] fixes for inline-crypto ipsec

[Mariusz Drost]

Fixes for ipsec-secgw application to work with inline crypto IPv4
transport mode and IPv6 tunnel/transport modes.

Mariusz Drost (2):
  net/ixgbe: fix lack of ip type for crypto session
  examples/ipsec-secgw: fix not working inline ipsec modes

 drivers/net/ixgbe/ixgbe_ipsec.c |   6 +-
 examples/ipsec-secgw/esp.c      |  12 +--
 examples/ipsec-secgw/ipsec.c    |  19 +++--
 examples/ipsec-secgw/ipsec.h    |  21 ++++-
 examples/ipsec-secgw/sa.c       | 142 ++++++++++++++++++++++++--------
 examples/ipsec-secgw/sp4.c      |  24 +++++-
 examples/ipsec-secgw/sp6.c      |  42 +++++++++-
 7 files changed, 210 insertions(+), 56 deletions(-)

-- 
2.17.1

[dpdk-dev] [PATCH 1/2] net/ixgbe: fix lack of ip type for crypto session

[Mariusz Drost]

When ixgbe_crypto_add_sa() is called, it checks whether the ip type is
IPv6 or IPv4 to write correct addresses to the registers. Type itself
is never specified, and act as IPv4, which is the default value.
It causes lack of support for IPv6.

To fix that, ip type needs to be stored in device private data, based on
crypto session ip type field, before the checking is done.

Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")

Signed-off-by: Mariusz Drost <mariuszx.drost@intel.com>
---
 drivers/net/ixgbe/ixgbe_ipsec.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ixgbe/ixgbe_ipsec.c b/drivers/net/ixgbe/ixgbe_ipsec.c
index 5a416885f..1eea70716 100644
--- a/drivers/net/ixgbe/ixgbe_ipsec.c
+++ b/drivers/net/ixgbe/ixgbe_ipsec.c
@@ -154,8 +154,12 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 		if (ic_session->op == IXGBE_OP_AUTHENTICATED_DECRYPTION)
 			priv->rx_sa_tbl[sa_index].mode |=
 					(IPSRXMOD_PROTO | IPSRXMOD_DECRYPT);
-		if (ic_session->dst_ip.type == IPv6)
+		if (ic_session->dst_ip.type == IPv6) {
 			priv->rx_sa_tbl[sa_index].mode |= IPSRXMOD_IPV6;
+			priv->rx_ip_tbl[ip_index].ip.type = IPv6;
+		} else if (ic_session->dst_ip.type == IPv4)
+			priv->rx_ip_tbl[ip_index].ip.type = IPv4;
+
 		priv->rx_sa_tbl[sa_index].used = 1;
 
 		/* write IP table entry*/
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/2] net/ixgbe: fix lack of ip type for crypto session

[Ananyev, Konstantin]

> -----Original Message-----
> From: Drost, MariuszX
> Sent: Tuesday, June 4, 2019 11:07 AM
> To: Nicolau, Radu <radu.nicolau@intel.com>; akhil.goyal@nxp.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; Drost, MariuszX <mariuszx.drost@intel.com>
> Subject: [PATCH 1/2] net/ixgbe: fix lack of ip type for crypto session
> 
> When ixgbe_crypto_add_sa() is called, it checks whether the ip type is
> IPv6 or IPv4 to write correct addresses to the registers. Type itself
> is never specified, and act as IPv4, which is the default value.
> It causes lack of support for IPv6.
> 
> To fix that, ip type needs to be stored in device private data, based on
> crypto session ip type field, before the checking is done.
> 
> Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
> Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
> 
> Signed-off-by: Mariusz Drost <mariuszx.drost@intel.com>
> ---
>  drivers/net/ixgbe/ixgbe_ipsec.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ipsec.c b/drivers/net/ixgbe/ixgbe_ipsec.c
> index 5a416885f..1eea70716 100644
> --- a/drivers/net/ixgbe/ixgbe_ipsec.c
> +++ b/drivers/net/ixgbe/ixgbe_ipsec.c
> @@ -154,8 +154,12 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
>  		if (ic_session->op == IXGBE_OP_AUTHENTICATED_DECRYPTION)
>  			priv->rx_sa_tbl[sa_index].mode |=
>  					(IPSRXMOD_PROTO | IPSRXMOD_DECRYPT);
> -		if (ic_session->dst_ip.type == IPv6)
> +		if (ic_session->dst_ip.type == IPv6) {
>  			priv->rx_sa_tbl[sa_index].mode |= IPSRXMOD_IPV6;
> +			priv->rx_ip_tbl[ip_index].ip.type = IPv6;
> +		} else if (ic_session->dst_ip.type == IPv4)
> +			priv->rx_ip_tbl[ip_index].ip.type = IPv4;
> +
>  		priv->rx_sa_tbl[sa_index].used = 1;
> 
>  		/* write IP table entry*/
> --

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Tested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.17.1

[dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes

[Mariusz Drost]

Application ipsec-secgw is not working for IPv4 transport mode and for
IPv6 both transport and tunnel mode.

IPv6 tunnel mode is not working due to wrongly assigned fields of
security association patterns, as it was IPv4, during creation of
inline crypto session.

IPv6 and IPv4 transport mode is iterating through security capabilities
until it reaches tunnel, which causes session to be created as tunnel,
instead of transport. Another issue, is that config file does not
provide source and destination ip addresses for transport mode, which
are required by NIC to perform inline crypto. It uses default addresses
stored in security association (all zeroes), which causes dropped
packages.

To fix that, reorganization of code in create_session() is needed,
to behave appropriately to given protocol (IPv6/IPv4). Change in
iteration through security capabilities is also required, to check
for expected mode (not only tunnel).

For lack of addresses issue, some resolving mechanism is needed.
Approach is to store addresses in security association, as it is
for tunnel mode. Difference is that they are obtained from sp rules,
instead of config file. To do that, sp[4/6]_spi_present() function
is used to find addresses based on spi value, and then stored in
corresponding sa rule. This approach assumes, that every sp rule
for inline crypto have valid addresses, as well as range of addresses
is not supported.

New flags for ipsec_sa structure are required to distinguish between
IPv4 and IPv6 transport modes. Because of that, there is need to
change all checks done on these flags, so they work as expected.

Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")

Signed-off-by: Mariusz Drost <mariuszx.drost@intel.com>
---
 examples/ipsec-secgw/esp.c   |  12 +--
 examples/ipsec-secgw/ipsec.c |  19 +++--
 examples/ipsec-secgw/ipsec.h |  21 +++++-
 examples/ipsec-secgw/sa.c    | 142 ++++++++++++++++++++++++++---------
 examples/ipsec-secgw/sp4.c   |  24 +++++-
 examples/ipsec-secgw/sp6.c   |  42 ++++++++++-
 6 files changed, 205 insertions(+), 55 deletions(-)

diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
index f11d095ba..764e08dcf 100644
--- a/examples/ipsec-secgw/esp.c
+++ b/examples/ipsec-secgw/esp.c
@@ -192,7 +192,7 @@ esp_inbound_post(struct rte_mbuf *m, struct ipsec_sa *sa,
 		}
 	}
 
-	if (unlikely(sa->flags == TRANSPORT)) {
+	if (unlikely(IS_TRANSPORT(sa->flags))) {
 		ip = rte_pktmbuf_mtod(m, struct ip *);
 		ip4 = (struct ip *)rte_pktmbuf_adj(m,
 				sizeof(struct rte_esp_hdr) + sa->iv_len);
@@ -233,13 +233,13 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 
 	ip4 = rte_pktmbuf_mtod(m, struct ip *);
 	if (likely(ip4->ip_v == IPVERSION)) {
-		if (unlikely(sa->flags == TRANSPORT)) {
+		if (unlikely(IS_TRANSPORT(sa->flags))) {
 			ip_hdr_len = ip4->ip_hl * 4;
 			nlp = ip4->ip_p;
 		} else
 			nlp = IPPROTO_IPIP;
 	} else if (ip4->ip_v == IP6_VERSION) {
-		if (unlikely(sa->flags == TRANSPORT)) {
+		if (unlikely(IS_TRANSPORT(sa->flags))) {
 			/* XXX No option headers supported */
 			ip_hdr_len = sizeof(struct ip6_hdr);
 			ip6 = (struct ip6_hdr *)ip4;
@@ -258,13 +258,13 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 	pad_len = pad_payload_len + ip_hdr_len - rte_pktmbuf_pkt_len(m);
 
 	RTE_ASSERT(sa->flags == IP4_TUNNEL || sa->flags == IP6_TUNNEL ||
-			sa->flags == TRANSPORT);
+			IS_TRANSPORT(sa->flags));
 
 	if (likely(sa->flags == IP4_TUNNEL))
 		ip_hdr_len = sizeof(struct ip);
 	else if (sa->flags == IP6_TUNNEL)
 		ip_hdr_len = sizeof(struct ip6_hdr);
-	else if (sa->flags != TRANSPORT) {
+	else if (!IS_TRANSPORT(sa->flags)) {
 		RTE_LOG(ERR, IPSEC_ESP, "Unsupported SA flags: 0x%x\n",
 				sa->flags);
 		return -EINVAL;
@@ -291,7 +291,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
 		rte_prefetch0(padding);
 	}
 
-	switch (sa->flags) {
+	switch (WITHOUT_TRANSPORT_VERSION(sa->flags)) {
 	case IP4_TUNNEL:
 		ip4 = ip4ip_outbound(m, sizeof(struct rte_esp_hdr) + sa->iv_len,
 				&sa->src, &sa->dst);
diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
index 7b8533077..afef72a1d 100644
--- a/examples/ipsec-secgw/ipsec.c
+++ b/examples/ipsec-secgw/ipsec.c
@@ -83,8 +83,7 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa)
 				.options = { 0 },
 				.direction = sa->direction,
 				.proto = RTE_SECURITY_IPSEC_SA_PROTO_ESP,
-				.mode = (sa->flags == IP4_TUNNEL ||
-						sa->flags == IP6_TUNNEL) ?
+				.mode = (IS_TUNNEL(sa->flags)) ?
 					RTE_SECURITY_IPSEC_SA_MODE_TUNNEL :
 					RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT,
 			} },
@@ -134,7 +133,7 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa)
 				    sec_cap->protocol ==
 					RTE_SECURITY_PROTOCOL_IPSEC &&
 				    sec_cap->ipsec.mode ==
-					RTE_SECURITY_IPSEC_SA_MODE_TUNNEL &&
+					sess_conf.ipsec.mode &&
 				    sec_cap->ipsec.direction == sa->direction)
 					break;
 				sec_cap++;
@@ -150,16 +149,20 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa)
 			sa->security_ctx = ctx;
 			sa->pattern[0].type = RTE_FLOW_ITEM_TYPE_ETH;
 
-			sa->pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV4;
-			sa->pattern[1].mask = &rte_flow_item_ipv4_mask;
-			if (sa->flags & IP6_TUNNEL) {
+			if (IS_IP6(sa->flags)) {
+				sa->pattern[1].mask = &rte_flow_item_ipv6_mask;
+				sa->pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV6;
 				sa->pattern[1].spec = &sa->ipv6_spec;
+
 				memcpy(sa->ipv6_spec.hdr.dst_addr,
 					sa->dst.ip.ip6.ip6_b, 16);
 				memcpy(sa->ipv6_spec.hdr.src_addr,
 				       sa->src.ip.ip6.ip6_b, 16);
-			} else {
+			} else if (IS_IP4(sa->flags)) {
+				sa->pattern[1].mask = &rte_flow_item_ipv4_mask;
+				sa->pattern[1].type = RTE_FLOW_ITEM_TYPE_IPV4;
 				sa->pattern[1].spec = &sa->ipv4_spec;
+
 				sa->ipv4_spec.hdr.dst_addr = sa->dst.ip.ip4;
 				sa->ipv4_spec.hdr.src_addr = sa->src.ip.ip4;
 			}
@@ -303,7 +306,7 @@ create_session(struct ipsec_ctx *ipsec_ctx, struct ipsec_sa *sa)
 				    sec_cap->protocol ==
 					RTE_SECURITY_PROTOCOL_IPSEC &&
 				    sec_cap->ipsec.mode ==
-					RTE_SECURITY_IPSEC_SA_MODE_TUNNEL &&
+					sess_conf.ipsec.mode &&
 				    sec_cap->ipsec.direction == sa->direction)
 					break;
 				sec_cap++;
diff --git a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h
index e9272d74b..5175f4adb 100644
--- a/examples/ipsec-secgw/ipsec.h
+++ b/examples/ipsec-secgw/ipsec.h
@@ -101,6 +101,8 @@ struct ipsec_sa {
 #define IP4_TUNNEL (1 << 0)
 #define IP6_TUNNEL (1 << 1)
 #define TRANSPORT  (1 << 2)
+#define IP4_TRANSPORT (1 << 3)
+#define IP6_TRANSPORT (1 << 4)
 	struct ip_addr src;
 	struct ip_addr dst;
 	uint8_t cipher_key[MAX_KEY_SIZE];
@@ -139,6 +141,19 @@ struct ipsec_mbuf_metadata {
 	uint8_t buf[32];
 } __rte_cache_aligned;
 
+#define IS_TRANSPORT(flags) ((flags) & TRANSPORT)
+
+#define IS_TUNNEL(flags) ((flags) & (IP4_TUNNEL | IP6_TUNNEL))
+
+#define IS_IP4(flags) ((flags) & (IP4_TUNNEL | IP4_TRANSPORT))
+
+#define IS_IP6(flags) ((flags) & (IP6_TUNNEL | IP6_TRANSPORT))
+
+#define WITHOUT_TRANSPORT_VERSION(flags) \
+		((flags) & (IP4_TUNNEL | \
+			IP6_TUNNEL | \
+			TRANSPORT))
+
 struct cdev_qp {
 	uint16_t id;
 	uint16_t qp;
@@ -283,9 +298,11 @@ sp6_init(struct socket_ctx *ctx, int32_t socket_id);
  * or -ENOENT otherwise.
  */
 int
-sp4_spi_present(uint32_t spi, int inbound);
+sp4_spi_present(uint32_t spi, int inbound, struct ip_addr ip_addr[2],
+			uint32_t mask[2]);
 int
-sp6_spi_present(uint32_t spi, int inbound);
+sp6_spi_present(uint32_t spi, int inbound, struct ip_addr ip_addr[2],
+			uint32_t mask[2]);
 
 /*
  * Search through SA entries for given SPI.
diff --git a/examples/ipsec-secgw/sa.c b/examples/ipsec-secgw/sa.c
index 8a3d89497..66821e904 100644
--- a/examples/ipsec-secgw/sa.c
+++ b/examples/ipsec-secgw/sa.c
@@ -27,6 +27,10 @@
 
 #define IPDEFTTL 64
 
+#define IP4_FULL_MASK (sizeof(((struct ip_addr *)NULL)->ip.ip4) * CHAR_BIT)
+
+#define IP6_FULL_MASK (sizeof(((struct ip_addr *)NULL)->ip.ip6.ip6) * CHAR_BIT)
+
 struct supported_cipher_algo {
 	const char *keyword;
 	enum rte_crypto_cipher_algorithm algo;
@@ -663,7 +667,7 @@ print_one_sa_rule(const struct ipsec_sa *sa, int inbound)
 
 	printf("mode:");
 
-	switch (sa->flags) {
+	switch (WITHOUT_TRANSPORT_VERSION(sa->flags)) {
 	case IP4_TUNNEL:
 		printf("IP4Tunnel ");
 		uint32_t_to_char(sa->src.ip.ip4, &a, &b, &c, &d);
@@ -774,6 +778,93 @@ check_eth_dev_caps(uint16_t portid, uint32_t inbound)
 	return 0;
 }
 
+/*
+ * Helper function, tries to determine next_proto for SPI
+ * by searching though SP rules.
+ */
+static int
+get_spi_proto(uint32_t spi, enum rte_security_ipsec_sa_direction dir,
+		struct ip_addr ip_addr[2], uint32_t mask[2])
+{
+	int32_t rc4, rc6;
+
+	rc4 = sp4_spi_present(spi, dir == RTE_SECURITY_IPSEC_SA_DIR_INGRESS,
+				ip_addr, mask);
+	rc6 = sp6_spi_present(spi, dir == RTE_SECURITY_IPSEC_SA_DIR_INGRESS,
+				ip_addr, mask);
+
+	if (rc4 >= 0) {
+		if (rc6 >= 0) {
+			RTE_LOG(ERR, IPSEC,
+				"%s: SPI %u used simultaeously by "
+				"IPv4(%d) and IPv6 (%d) SP rules\n",
+				__func__, spi, rc4, rc6);
+			return -EINVAL;
+		} else
+			return IPPROTO_IPIP;
+	} else if (rc6 < 0) {
+		RTE_LOG(ERR, IPSEC,
+			"%s: SPI %u is not used by any SP rule\n",
+			__func__, spi);
+		return -EINVAL;
+	} else
+		return IPPROTO_IPV6;
+}
+
+/*
+ * Helper function for getting source and destination IP addresses
+ * from SP. Needed for inline crypto transport mode, as addresses are not
+ * provided in config file for that mode. It checks if SP for current SA exists,
+ * and based on what type of protocol is returned, it stores appropriate
+ * addresses got from SP into SA.
+ */
+static int
+sa_add_address_inline_crypto(struct ipsec_sa *sa)
+{
+	int protocol;
+	struct ip_addr ip_addr[2];
+	uint32_t mask[2];
+
+	protocol = get_spi_proto(sa->spi, sa->direction, ip_addr, mask);
+	if (protocol < 0)
+		return protocol;
+	else if (protocol == IPPROTO_IPIP) {
+		sa->flags |= IP4_TRANSPORT;
+		if (mask[0] == IP4_FULL_MASK &&
+				mask[1] == IP4_FULL_MASK &&
+				ip_addr[0].ip.ip4 != 0 &&
+				ip_addr[1].ip.ip4 != 0) {
+
+			sa->src.ip.ip4 = ip_addr[0].ip.ip4;
+			sa->dst.ip.ip4 = ip_addr[1].ip.ip4;
+		} else {
+			RTE_LOG(ERR, IPSEC,
+			"%s: No valid address or mask entry in"
+			" IPv4 SP rule for SPI %u\n",
+			__func__, sa->spi);
+			return -EINVAL;
+		}
+	} else if (protocol == IPPROTO_IPV6) {
+		sa->flags |= IP6_TRANSPORT;
+		if (mask[0] == IP6_FULL_MASK &&
+				mask[1] == IP6_FULL_MASK &&
+				(ip_addr[0].ip.ip6.ip6[0] != 0 ||
+				ip_addr[0].ip.ip6.ip6[1] != 0) &&
+				(ip_addr[1].ip.ip6.ip6[0] != 0 ||
+				ip_addr[1].ip.ip6.ip6[1] != 0)) {
+
+			sa->src.ip.ip6 = ip_addr[0].ip.ip6;
+			sa->dst.ip.ip6 = ip_addr[1].ip.ip6;
+		} else {
+			RTE_LOG(ERR, IPSEC,
+			"%s: No valid address or mask entry in"
+			" IPv6 SP rule for SPI %u\n",
+			__func__, sa->spi);
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
 
 static int
 sa_add_rules(struct sa_ctx *sa_ctx, const struct ipsec_sa entries[],
@@ -782,6 +873,7 @@ sa_add_rules(struct sa_ctx *sa_ctx, const struct ipsec_sa entries[],
 	struct ipsec_sa *sa;
 	uint32_t i, idx;
 	uint16_t iv_length, aad_length;
+	int inline_status;
 
 	/* for ESN upper 32 bits of SQN also need to be part of AAD */
 	aad_length = (app_sa_prm.enable_esn != 0) ? sizeof(uint32_t) : 0;
@@ -807,10 +899,20 @@ sa_add_rules(struct sa_ctx *sa_ctx, const struct ipsec_sa entries[],
 				RTE_SECURITY_IPSEC_SA_DIR_INGRESS :
 				RTE_SECURITY_IPSEC_SA_DIR_EGRESS;
 
-		switch (sa->flags) {
+		switch (WITHOUT_TRANSPORT_VERSION(sa->flags)) {
 		case IP4_TUNNEL:
 			sa->src.ip.ip4 = rte_cpu_to_be_32(sa->src.ip.ip4);
 			sa->dst.ip.ip4 = rte_cpu_to_be_32(sa->dst.ip.ip4);
+			break;
+		case TRANSPORT:
+			if (sa->type ==
+				RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO) {
+				inline_status =
+					sa_add_address_inline_crypto(sa);
+				if (inline_status < 0)
+					return inline_status;
+			}
+			break;
 		}
 
 		if (sa->aead_algo == RTE_CRYPTO_AEAD_AES_GCM) {
@@ -935,36 +1037,6 @@ fill_ipsec_app_sa_prm(struct rte_ipsec_sa_prm *prm,
 	prm->replay_win_sz = app_prm->window_size;
 }
 
-/*
- * Helper function, tries to determine next_proto for SPI
- * by searching though SP rules.
- */
-static int
-get_spi_proto(uint32_t spi, enum rte_security_ipsec_sa_direction dir)
-{
-	int32_t rc4, rc6;
-
-	rc4 = sp4_spi_present(spi, dir == RTE_SECURITY_IPSEC_SA_DIR_INGRESS);
-	rc6 = sp6_spi_present(spi, dir == RTE_SECURITY_IPSEC_SA_DIR_INGRESS);
-
-	if (rc4 >= 0) {
-		if (rc6 >= 0) {
-			RTE_LOG(ERR, IPSEC,
-				"%s: SPI %u used simultaeously by "
-				"RTE_IPv4(%d) and IPv6 (%d) SP rules\n",
-				__func__, spi, rc4, rc6);
-			return -EINVAL;
-		} else
-			return IPPROTO_IPIP;
-	} else if (rc6 < 0) {
-		RTE_LOG(ERR, IPSEC,
-			"%s: SPI %u is not used by any SP rule\n",
-			__func__, spi);
-		return -EINVAL;
-	} else
-		return IPPROTO_IPV6;
-}
-
 static int
 fill_ipsec_sa_prm(struct rte_ipsec_sa_prm *prm, const struct ipsec_sa *ss,
 	const struct rte_ipv4_hdr *v4, struct rte_ipv6_hdr *v6)
@@ -976,7 +1048,7 @@ fill_ipsec_sa_prm(struct rte_ipsec_sa_prm *prm, const struct ipsec_sa *ss,
 	 * probably not the optimal way, but there seems nothing
 	 * better right now.
 	 */
-	rc = get_spi_proto(ss->spi, ss->direction);
+	rc = get_spi_proto(ss->spi, ss->direction, NULL, NULL);
 	if (rc < 0)
 		return rc;
 
@@ -988,7 +1060,7 @@ fill_ipsec_sa_prm(struct rte_ipsec_sa_prm *prm, const struct ipsec_sa *ss,
 	prm->ipsec_xform.salt = ss->salt;
 	prm->ipsec_xform.direction = ss->direction;
 	prm->ipsec_xform.proto = RTE_SECURITY_IPSEC_SA_PROTO_ESP;
-	prm->ipsec_xform.mode = (ss->flags == TRANSPORT) ?
+	prm->ipsec_xform.mode = (IS_TRANSPORT(ss->flags)) ?
 		RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT :
 		RTE_SECURITY_IPSEC_SA_MODE_TUNNEL;
 
@@ -1240,7 +1312,7 @@ single_inbound_lookup(struct ipsec_sa *sadb, struct rte_mbuf *pkt,
 	if (rte_be_to_cpu_32(esp->spi) != sa->spi)
 		return;
 
-	switch (sa->flags) {
+	switch (WITHOUT_TRANSPORT_VERSION(sa->flags)) {
 	case IP4_TUNNEL:
 		src4_addr = RTE_PTR_ADD(ip, offsetof(struct ip, ip_src));
 		if ((ip->ip_v == IPVERSION) &&
diff --git a/examples/ipsec-secgw/sp4.c b/examples/ipsec-secgw/sp4.c
index ca9ee7f24..3871c6cc1 100644
--- a/examples/ipsec-secgw/sp4.c
+++ b/examples/ipsec-secgw/sp4.c
@@ -17,6 +17,18 @@
 
 #define MAX_ACL_RULE_NUM	1024
 
+#define IPV4_DST_FROM_SP(acr) \
+		(rte_cpu_to_be_32((acr).field[DST_FIELD_IPV4].value.u32))
+
+#define IPV4_SRC_FROM_SP(acr) \
+		(rte_cpu_to_be_32((acr).field[SRC_FIELD_IPV4].value.u32))
+
+#define IPV4_DST_MASK_FROM_SP(acr) \
+		((acr).field[DST_FIELD_IPV4].mask_range.u32)
+
+#define IPV4_SRC_MASK_FROM_SP(acr) \
+		((acr).field[SRC_FIELD_IPV4].mask_range.u32)
+
 /*
  * Rule and trace formats definitions.
  */
@@ -552,7 +564,8 @@ sp4_init(struct socket_ctx *ctx, int32_t socket_id)
  * Search though SP rules for given SPI.
  */
 int
-sp4_spi_present(uint32_t spi, int inbound)
+sp4_spi_present(uint32_t spi, int inbound, struct ip_addr ip_addr[2],
+			uint32_t mask[2])
 {
 	uint32_t i, num;
 	const struct acl4_rules *acr;
@@ -566,8 +579,15 @@ sp4_spi_present(uint32_t spi, int inbound)
 	}
 
 	for (i = 0; i != num; i++) {
-		if (acr[i].data.userdata == spi)
+		if (acr[i].data.userdata == spi) {
+			if (NULL != ip_addr && NULL != mask) {
+				ip_addr[0].ip.ip4 = IPV4_SRC_FROM_SP(acr[i]);
+				ip_addr[1].ip.ip4 = IPV4_DST_FROM_SP(acr[i]);
+				mask[0] = IPV4_SRC_MASK_FROM_SP(acr[i]);
+				mask[1] = IPV4_DST_MASK_FROM_SP(acr[i]);
+			}
 			return i;
+		}
 	}
 
 	return -ENOENT;
diff --git a/examples/ipsec-secgw/sp6.c b/examples/ipsec-secgw/sp6.c
index 76be3d3e9..d8be6b1bd 100644
--- a/examples/ipsec-secgw/sp6.c
+++ b/examples/ipsec-secgw/sp6.c
@@ -17,6 +17,36 @@
 
 #define MAX_ACL_RULE_NUM	1024
 
+#define IPV6_FROM_SP(acr, fidx_low, fidx_high) \
+		(((uint64_t)(acr).field[(fidx_high)].value.u32 << 32) | \
+		(acr).field[(fidx_low)].value.u32)
+
+#define IPV6_DST_FROM_SP(addr, acr) do {\
+		(addr).ip.ip6.ip6[0] = rte_cpu_to_be_64(IPV6_FROM_SP((acr), \
+						IP6_DST1, IP6_DST0));\
+		(addr).ip.ip6.ip6[1] = rte_cpu_to_be_64(IPV6_FROM_SP((acr), \
+						IP6_DST3, IP6_DST2));\
+		} while (0)
+
+#define IPV6_SRC_FROM_SP(addr, acr) do {\
+		(addr).ip.ip6.ip6[0] = rte_cpu_to_be_64(IPV6_FROM_SP((acr), \
+							IP6_SRC1, IP6_SRC0));\
+		(addr).ip.ip6.ip6[1] = rte_cpu_to_be_64(IPV6_FROM_SP((acr), \
+							IP6_SRC3, IP6_SRC2));\
+		} while (0)
+
+#define IPV6_DST_MASK_FROM_SP(mask, acr) \
+		((mask) = (acr).field[IP6_DST0].mask_range.u32 + \
+			(acr).field[IP6_DST1].mask_range.u32 + \
+			(acr).field[IP6_DST2].mask_range.u32 + \
+			(acr).field[IP6_DST3].mask_range.u32)
+
+#define IPV6_SRC_MASK_FROM_SP(mask, acr) \
+		((mask) = (acr).field[IP6_SRC0].mask_range.u32 + \
+			(acr).field[IP6_SRC1].mask_range.u32 + \
+			(acr).field[IP6_SRC2].mask_range.u32 + \
+			(acr).field[IP6_SRC3].mask_range.u32)
+
 enum {
 	IP6_PROTO,
 	IP6_SRC0,
@@ -666,7 +696,8 @@ sp6_init(struct socket_ctx *ctx, int32_t socket_id)
  * Search though SP rules for given SPI.
  */
 int
-sp6_spi_present(uint32_t spi, int inbound)
+sp6_spi_present(uint32_t spi, int inbound, struct ip_addr ip_addr[2],
+			uint32_t mask[2])
 {
 	uint32_t i, num;
 	const struct acl6_rules *acr;
@@ -680,8 +711,15 @@ sp6_spi_present(uint32_t spi, int inbound)
 	}
 
 	for (i = 0; i != num; i++) {
-		if (acr[i].data.userdata == spi)
+		if (acr[i].data.userdata == spi) {
+			if (NULL != ip_addr && NULL != mask) {
+				IPV6_SRC_FROM_SP(ip_addr[0], acr[i]);
+				IPV6_DST_FROM_SP(ip_addr[1], acr[i]);
+				IPV6_SRC_MASK_FROM_SP(mask[0], acr[i]);
+				IPV6_DST_MASK_FROM_SP(mask[1], acr[i]);
+			}
 			return i;
+		}
 	}
 
 	return -ENOENT;
-- 
2.17.1

Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes

[Ananyev, Konstantin]

> -----Original Message-----
> From: Drost, MariuszX
> Sent: Tuesday, June 4, 2019 11:07 AM
> To: Nicolau, Radu <radu.nicolau@intel.com>; akhil.goyal@nxp.com; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org; Drost, MariuszX <mariuszx.drost@intel.com>
> Subject: [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes
> 
> Application ipsec-secgw is not working for IPv4 transport mode and for
> IPv6 both transport and tunnel mode.
> 
> IPv6 tunnel mode is not working due to wrongly assigned fields of
> security association patterns, as it was IPv4, during creation of
> inline crypto session.
> 
> IPv6 and IPv4 transport mode is iterating through security capabilities
> until it reaches tunnel, which causes session to be created as tunnel,
> instead of transport. Another issue, is that config file does not
> provide source and destination ip addresses for transport mode, which
> are required by NIC to perform inline crypto. It uses default addresses
> stored in security association (all zeroes), which causes dropped
> packages.
> 
> To fix that, reorganization of code in create_session() is needed,
> to behave appropriately to given protocol (IPv6/IPv4). Change in
> iteration through security capabilities is also required, to check
> for expected mode (not only tunnel).
> 
> For lack of addresses issue, some resolving mechanism is needed.
> Approach is to store addresses in security association, as it is
> for tunnel mode. Difference is that they are obtained from sp rules,
> instead of config file. To do that, sp[4/6]_spi_present() function
> is used to find addresses based on spi value, and then stored in
> corresponding sa rule. This approach assumes, that every sp rule
> for inline crypto have valid addresses, as well as range of addresses
> is not supported.
> 
> New flags for ipsec_sa structure are required to distinguish between
> IPv4 and IPv6 transport modes. Because of that, there is need to
> change all checks done on these flags, so they work as expected.
> 
> Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
> Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
> 
> Signed-off-by: Mariusz Drost <mariuszx.drost@intel.com>
> ---

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Tested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> --
> 2.17.1

Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes

[Akhil Goyal]

Hi Marius,


> Application ipsec-secgw is not working for IPv4 transport mode and for
> IPv6 both transport and tunnel mode.
> 
> IPv6 tunnel mode is not working due to wrongly assigned fields of
> security association patterns, as it was IPv4, during creation of
> inline crypto session.
> 
> IPv6 and IPv4 transport mode is iterating through security capabilities
> until it reaches tunnel, which causes session to be created as tunnel,
> instead of transport. Another issue, is that config file does not
> provide source and destination ip addresses for transport mode, which
> are required by NIC to perform inline crypto. It uses default addresses
> stored in security association (all zeroes), which causes dropped
> packages.
> 
> To fix that, reorganization of code in create_session() is needed,
> to behave appropriately to given protocol (IPv6/IPv4). Change in
> iteration through security capabilities is also required, to check
> for expected mode (not only tunnel).
> 
> For lack of addresses issue, some resolving mechanism is needed.
> Approach is to store addresses in security association, as it is
> for tunnel mode. Difference is that they are obtained from sp rules,
> instead of config file. To do that, sp[4/6]_spi_present() function
> is used to find addresses based on spi value, and then stored in
> corresponding sa rule. This approach assumes, that every sp rule
> for inline crypto have valid addresses, as well as range of addresses
> is not supported.
> 
> New flags for ipsec_sa structure are required to distinguish between
> IPv4 and IPv6 transport modes. Because of that, there is need to
> change all checks done on these flags, so they work as expected.
> 
> Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
> Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
> 
This is a very well written description. Thanks. This helps in review of the patch.

I have a few small comments, rest all is fine.

> Signed-off-by: Mariusz Drost <mariuszx.drost@intel.com>
> ---
>  examples/ipsec-secgw/esp.c   |  12 +--
>  examples/ipsec-secgw/ipsec.c |  19 +++--
>  examples/ipsec-secgw/ipsec.h |  21 +++++-
>  examples/ipsec-secgw/sa.c    | 142 ++++++++++++++++++++++++++---------
>  examples/ipsec-secgw/sp4.c   |  24 +++++-
>  examples/ipsec-secgw/sp6.c   |  42 ++++++++++-
>  6 files changed, 205 insertions(+), 55 deletions(-)
> 
> diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
> index f11d095ba..764e08dcf 100644
> --- a/examples/ipsec-secgw/esp.c
> +++ b/examples/ipsec-secgw/esp.c
> @@ -192,7 +192,7 @@ esp_inbound_post(struct rte_mbuf *m, struct ipsec_sa
> *sa,
>  		}
>  	}
> 
> -	if (unlikely(sa->flags == TRANSPORT)) {
> +	if (unlikely(IS_TRANSPORT(sa->flags))) {
>  		ip = rte_pktmbuf_mtod(m, struct ip *);
>  		ip4 = (struct ip *)rte_pktmbuf_adj(m,
>  				sizeof(struct rte_esp_hdr) + sa->iv_len);
> @@ -233,13 +233,13 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa
> *sa,
> 
>  	ip4 = rte_pktmbuf_mtod(m, struct ip *);
>  	if (likely(ip4->ip_v == IPVERSION)) {
> -		if (unlikely(sa->flags == TRANSPORT)) {
> +		if (unlikely(IS_TRANSPORT(sa->flags))) {
>  			ip_hdr_len = ip4->ip_hl * 4;
>  			nlp = ip4->ip_p;
>  		} else
>  			nlp = IPPROTO_IPIP;
>  	} else if (ip4->ip_v == IP6_VERSION) {
> -		if (unlikely(sa->flags == TRANSPORT)) {
> +		if (unlikely(IS_TRANSPORT(sa->flags))) {
>  			/* XXX No option headers supported */
>  			ip_hdr_len = sizeof(struct ip6_hdr);
>  			ip6 = (struct ip6_hdr *)ip4;
> @@ -258,13 +258,13 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa
> *sa,
>  	pad_len = pad_payload_len + ip_hdr_len - rte_pktmbuf_pkt_len(m);
> 
>  	RTE_ASSERT(sa->flags == IP4_TUNNEL || sa->flags == IP6_TUNNEL ||
> -			sa->flags == TRANSPORT);
> +			IS_TRANSPORT(sa->flags));
I can see that at multiple places, sa->flags are accessed without your defined macros. Could you please update this at all places, so that it will be uniform across the application.

> 
>  	if (likely(sa->flags == IP4_TUNNEL))
>  		ip_hdr_len = sizeof(struct ip);
>  	else if (sa->flags == IP6_TUNNEL)
>  		ip_hdr_len = sizeof(struct ip6_hdr);
> -	else if (sa->flags != TRANSPORT) {
> +	else if (!IS_TRANSPORT(sa->flags)) {
>  		RTE_LOG(ERR, IPSEC_ESP, "Unsupported SA flags: 0x%x\n",
>  				sa->flags);
>  		return -EINVAL;
> @@ -291,7 +291,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
>  		rte_prefetch0(padding);
>  	}
> 
> -	switch (sa->flags) {
> +	switch (WITHOUT_TRANSPORT_VERSION(sa->flags)) {
I do not get the intent of this macro " WITHOUT_TRANSPORT_VERSION ". could you explain this in comments or some better name of the macro.

>  	case IP4_TUNNEL:
>  		ip4 = ip4ip_outbound(m, sizeof(struct rte_esp_hdr) + sa->iv_len,
>  				&sa->src, &sa->dst);


Regards,
Akhil

Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes

[Akhil Goyal]

Hi Marius,

Could you please send the updated patch soon, so that they can be applied before RC1.

Thanks,
Akhil

> 
> Hi Marius,
> 
> 
> > Application ipsec-secgw is not working for IPv4 transport mode and for
> > IPv6 both transport and tunnel mode.
> >
> > IPv6 tunnel mode is not working due to wrongly assigned fields of
> > security association patterns, as it was IPv4, during creation of
> > inline crypto session.
> >
> > IPv6 and IPv4 transport mode is iterating through security capabilities
> > until it reaches tunnel, which causes session to be created as tunnel,
> > instead of transport. Another issue, is that config file does not
> > provide source and destination ip addresses for transport mode, which
> > are required by NIC to perform inline crypto. It uses default addresses
> > stored in security association (all zeroes), which causes dropped
> > packages.
> >
> > To fix that, reorganization of code in create_session() is needed,
> > to behave appropriately to given protocol (IPv6/IPv4). Change in
> > iteration through security capabilities is also required, to check
> > for expected mode (not only tunnel).
> >
> > For lack of addresses issue, some resolving mechanism is needed.
> > Approach is to store addresses in security association, as it is
> > for tunnel mode. Difference is that they are obtained from sp rules,
> > instead of config file. To do that, sp[4/6]_spi_present() function
> > is used to find addresses based on spi value, and then stored in
> > corresponding sa rule. This approach assumes, that every sp rule
> > for inline crypto have valid addresses, as well as range of addresses
> > is not supported.
> >
> > New flags for ipsec_sa structure are required to distinguish between
> > IPv4 and IPv6 transport modes. Because of that, there is need to
> > change all checks done on these flags, so they work as expected.
> >
> > Fixes: ec17993a145a ("examples/ipsec-secgw: support security offload")
> > Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
> >
> This is a very well written description. Thanks. This helps in review of the patch.
> 
> I have a few small comments, rest all is fine.
> 
> > Signed-off-by: Mariusz Drost <mariuszx.drost@intel.com>
> > ---
> >  examples/ipsec-secgw/esp.c   |  12 +--
> >  examples/ipsec-secgw/ipsec.c |  19 +++--
> >  examples/ipsec-secgw/ipsec.h |  21 +++++-
> >  examples/ipsec-secgw/sa.c    | 142 ++++++++++++++++++++++++++---------
> >  examples/ipsec-secgw/sp4.c   |  24 +++++-
> >  examples/ipsec-secgw/sp6.c   |  42 ++++++++++-
> >  6 files changed, 205 insertions(+), 55 deletions(-)
> >
> > diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
> > index f11d095ba..764e08dcf 100644
> > --- a/examples/ipsec-secgw/esp.c
> > +++ b/examples/ipsec-secgw/esp.c
> > @@ -192,7 +192,7 @@ esp_inbound_post(struct rte_mbuf *m, struct ipsec_sa
> > *sa,
> >  		}
> >  	}
> >
> > -	if (unlikely(sa->flags == TRANSPORT)) {
> > +	if (unlikely(IS_TRANSPORT(sa->flags))) {
> >  		ip = rte_pktmbuf_mtod(m, struct ip *);
> >  		ip4 = (struct ip *)rte_pktmbuf_adj(m,
> >  				sizeof(struct rte_esp_hdr) + sa->iv_len);
> > @@ -233,13 +233,13 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa
> > *sa,
> >
> >  	ip4 = rte_pktmbuf_mtod(m, struct ip *);
> >  	if (likely(ip4->ip_v == IPVERSION)) {
> > -		if (unlikely(sa->flags == TRANSPORT)) {
> > +		if (unlikely(IS_TRANSPORT(sa->flags))) {
> >  			ip_hdr_len = ip4->ip_hl * 4;
> >  			nlp = ip4->ip_p;
> >  		} else
> >  			nlp = IPPROTO_IPIP;
> >  	} else if (ip4->ip_v == IP6_VERSION) {
> > -		if (unlikely(sa->flags == TRANSPORT)) {
> > +		if (unlikely(IS_TRANSPORT(sa->flags))) {
> >  			/* XXX No option headers supported */
> >  			ip_hdr_len = sizeof(struct ip6_hdr);
> >  			ip6 = (struct ip6_hdr *)ip4;
> > @@ -258,13 +258,13 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa
> > *sa,
> >  	pad_len = pad_payload_len + ip_hdr_len - rte_pktmbuf_pkt_len(m);
> >
> >  	RTE_ASSERT(sa->flags == IP4_TUNNEL || sa->flags == IP6_TUNNEL ||
> > -			sa->flags == TRANSPORT);
> > +			IS_TRANSPORT(sa->flags));
> I can see that at multiple places, sa->flags are accessed without your defined
> macros. Could you please update this at all places, so that it will be uniform
> across the application.
> 
> >
> >  	if (likely(sa->flags == IP4_TUNNEL))
> >  		ip_hdr_len = sizeof(struct ip);
> >  	else if (sa->flags == IP6_TUNNEL)
> >  		ip_hdr_len = sizeof(struct ip6_hdr);
> > -	else if (sa->flags != TRANSPORT) {
> > +	else if (!IS_TRANSPORT(sa->flags)) {
> >  		RTE_LOG(ERR, IPSEC_ESP, "Unsupported SA flags: 0x%x\n",
> >  				sa->flags);
> >  		return -EINVAL;
> > @@ -291,7 +291,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
> >  		rte_prefetch0(padding);
> >  	}
> >
> > -	switch (sa->flags) {
> > +	switch (WITHOUT_TRANSPORT_VERSION(sa->flags)) {
> I do not get the intent of this macro " WITHOUT_TRANSPORT_VERSION ". could
> you explain this in comments or some better name of the macro.
> 
> >  	case IP4_TUNNEL:
> >  		ip4 = ip4ip_outbound(m, sizeof(struct rte_esp_hdr) + sa->iv_len,
> >  				&sa->src, &sa->dst);
> 
> 
> Regards,
> Akhil

Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes

[Drost, MariuszX]

Hi,

About your comments:

1) I used macros around sa->flags where it was needed. Not all checks for that set of flags use information if it is transport mode. As for macro WITHOUT_TRANSPORT_VERSION, it was set only for checks that required information from set of flags without taking into account new transport flags -> I can set it in more places (like initialization stage), but I do not see a point of that, besides being uniform.

2) WITHOUT_TRANSPORT_VERSION is a macro which masks sa->flags as they were before change. It cuts newly proposed flags for transport mode, so behavior of switches, where such flags were used before as variable, is unchanged. I will provide a comment to the macro.

I will provide patch as soon as possible (probably tomorrow).

Kind regards,
Mariusz Drost.

-----Original Message-----
From: Akhil Goyal [mailto:akhil.goyal@nxp.com] 
Sent: Tuesday, June 25, 2019 3:15 PM
To: Akhil Goyal <akhil.goyal@nxp.com>; Drost, MariuszX <mariuszx.drost@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>
Cc: dev@dpdk.org
Subject: RE: [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes

Hi Marius,

Could you please send the updated patch soon, so that they can be applied before RC1.

Thanks,
Akhil

> 
> Hi Marius,
> 
> 
> > Application ipsec-secgw is not working for IPv4 transport mode and 
> > for
> > IPv6 both transport and tunnel mode.
> >
> > IPv6 tunnel mode is not working due to wrongly assigned fields of 
> > security association patterns, as it was IPv4, during creation of 
> > inline crypto session.
> >
> > IPv6 and IPv4 transport mode is iterating through security 
> > capabilities until it reaches tunnel, which causes session to be 
> > created as tunnel, instead of transport. Another issue, is that 
> > config file does not provide source and destination ip addresses for 
> > transport mode, which are required by NIC to perform inline crypto. 
> > It uses default addresses stored in security association (all 
> > zeroes), which causes dropped packages.
> >
> > To fix that, reorganization of code in create_session() is needed, 
> > to behave appropriately to given protocol (IPv6/IPv4). Change in 
> > iteration through security capabilities is also required, to check 
> > for expected mode (not only tunnel).
> >
> > For lack of addresses issue, some resolving mechanism is needed.
> > Approach is to store addresses in security association, as it is for 
> > tunnel mode. Difference is that they are obtained from sp rules, 
> > instead of config file. To do that, sp[4/6]_spi_present() function 
> > is used to find addresses based on spi value, and then stored in 
> > corresponding sa rule. This approach assumes, that every sp rule for 
> > inline crypto have valid addresses, as well as range of addresses is 
> > not supported.
> >
> > New flags for ipsec_sa structure are required to distinguish between
> > IPv4 and IPv6 transport modes. Because of that, there is need to 
> > change all checks done on these flags, so they work as expected.
> >
> > Fixes: ec17993a145a ("examples/ipsec-secgw: support security 
> > offload")
> > Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
> >
> This is a very well written description. Thanks. This helps in review of the patch.
> 
> I have a few small comments, rest all is fine.
> 
> > Signed-off-by: Mariusz Drost <mariuszx.drost@intel.com>
> > ---
> >  examples/ipsec-secgw/esp.c   |  12 +--
> >  examples/ipsec-secgw/ipsec.c |  19 +++--  
> > examples/ipsec-secgw/ipsec.h |  21 +++++-
> >  examples/ipsec-secgw/sa.c    | 142 ++++++++++++++++++++++++++---------
> >  examples/ipsec-secgw/sp4.c   |  24 +++++-
> >  examples/ipsec-secgw/sp6.c   |  42 ++++++++++-
> >  6 files changed, 205 insertions(+), 55 deletions(-)
> >
> > diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c 
> > index f11d095ba..764e08dcf 100644
> > --- a/examples/ipsec-secgw/esp.c
> > +++ b/examples/ipsec-secgw/esp.c
> > @@ -192,7 +192,7 @@ esp_inbound_post(struct rte_mbuf *m, struct 
> > ipsec_sa *sa,
> >  		}
> >  	}
> >
> > -	if (unlikely(sa->flags == TRANSPORT)) {
> > +	if (unlikely(IS_TRANSPORT(sa->flags))) {
> >  		ip = rte_pktmbuf_mtod(m, struct ip *);
> >  		ip4 = (struct ip *)rte_pktmbuf_adj(m,
> >  				sizeof(struct rte_esp_hdr) + sa->iv_len); @@ -233,13 +233,13 @@ 
> > esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
> >
> >  	ip4 = rte_pktmbuf_mtod(m, struct ip *);
> >  	if (likely(ip4->ip_v == IPVERSION)) {
> > -		if (unlikely(sa->flags == TRANSPORT)) {
> > +		if (unlikely(IS_TRANSPORT(sa->flags))) {
> >  			ip_hdr_len = ip4->ip_hl * 4;
> >  			nlp = ip4->ip_p;
> >  		} else
> >  			nlp = IPPROTO_IPIP;
> >  	} else if (ip4->ip_v == IP6_VERSION) {
> > -		if (unlikely(sa->flags == TRANSPORT)) {
> > +		if (unlikely(IS_TRANSPORT(sa->flags))) {
> >  			/* XXX No option headers supported */
> >  			ip_hdr_len = sizeof(struct ip6_hdr);
> >  			ip6 = (struct ip6_hdr *)ip4;
> > @@ -258,13 +258,13 @@ esp_outbound(struct rte_mbuf *m, struct 
> > ipsec_sa *sa,
> >  	pad_len = pad_payload_len + ip_hdr_len - rte_pktmbuf_pkt_len(m);
> >
> >  	RTE_ASSERT(sa->flags == IP4_TUNNEL || sa->flags == IP6_TUNNEL ||
> > -			sa->flags == TRANSPORT);
> > +			IS_TRANSPORT(sa->flags));
> I can see that at multiple places, sa->flags are accessed without your 
> defined macros. Could you please update this at all places, so that it 
> will be uniform across the application.
> 
> >
> >  	if (likely(sa->flags == IP4_TUNNEL))
> >  		ip_hdr_len = sizeof(struct ip);
> >  	else if (sa->flags == IP6_TUNNEL)
> >  		ip_hdr_len = sizeof(struct ip6_hdr);
> > -	else if (sa->flags != TRANSPORT) {
> > +	else if (!IS_TRANSPORT(sa->flags)) {
> >  		RTE_LOG(ERR, IPSEC_ESP, "Unsupported SA flags: 0x%x\n",
> >  				sa->flags);
> >  		return -EINVAL;
> > @@ -291,7 +291,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
> >  		rte_prefetch0(padding);
> >  	}
> >
> > -	switch (sa->flags) {
> > +	switch (WITHOUT_TRANSPORT_VERSION(sa->flags)) {
> I do not get the intent of this macro " WITHOUT_TRANSPORT_VERSION ". 
> could you explain this in comments or some better name of the macro.
> 
> >  	case IP4_TUNNEL:
> >  		ip4 = ip4ip_outbound(m, sizeof(struct rte_esp_hdr) + sa->iv_len,
> >  				&sa->src, &sa->dst);
> 
> 
> Regards,
> Akhil

Re: [dpdk-dev] [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec modes

[Akhil Goyal]

Hi Marius,

> 
> Hi,
> 
> About your comments:
> 
> 1) I used macros around sa->flags where it was needed. Not all checks for that
> set of flags use information if it is transport mode. As for macro
> WITHOUT_TRANSPORT_VERSION, it was set only for checks that required
> information from set of flags without taking into account new transport flags ->
> I can set it in more places (like initialization stage), but I do not see a point of
> that, besides being uniform.

I think it would be better if we are adding certain flags to simplify code, we should add
In all the checks.
I can see that in single if else sequence there are 2 different ways to check for the values
Of sa->flags. I think that this can be avoided.


> 
> 2) WITHOUT_TRANSPORT_VERSION is a macro which masks sa->flags as they
> were before change. It cuts newly proposed flags for transport mode, so
> behavior of switches, where such flags were used before as variable, is
> unchanged. I will provide a comment to the macro.
> 
> I will provide patch as soon as possible (probably tomorrow).
> 
> Kind regards,
> Mariusz Drost.
> 
> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Tuesday, June 25, 2019 3:15 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Drost, MariuszX
> <mariuszx.drost@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>; Lu,
> Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH 2/2] examples/ipsec-secgw: fix not working inline ipsec
> modes
> 
> Hi Marius,
> 
> Could you please send the updated patch soon, so that they can be applied
> before RC1.
> 
> Thanks,
> Akhil
> 
> >
> > Hi Marius,
> >
> >
> > > Application ipsec-secgw is not working for IPv4 transport mode and
> > > for
> > > IPv6 both transport and tunnel mode.
> > >
> > > IPv6 tunnel mode is not working due to wrongly assigned fields of
> > > security association patterns, as it was IPv4, during creation of
> > > inline crypto session.
> > >
> > > IPv6 and IPv4 transport mode is iterating through security
> > > capabilities until it reaches tunnel, which causes session to be
> > > created as tunnel, instead of transport. Another issue, is that
> > > config file does not provide source and destination ip addresses for
> > > transport mode, which are required by NIC to perform inline crypto.
> > > It uses default addresses stored in security association (all
> > > zeroes), which causes dropped packages.
> > >
> > > To fix that, reorganization of code in create_session() is needed,
> > > to behave appropriately to given protocol (IPv6/IPv4). Change in
> > > iteration through security capabilities is also required, to check
> > > for expected mode (not only tunnel).
> > >
> > > For lack of addresses issue, some resolving mechanism is needed.
> > > Approach is to store addresses in security association, as it is for
> > > tunnel mode. Difference is that they are obtained from sp rules,
> > > instead of config file. To do that, sp[4/6]_spi_present() function
> > > is used to find addresses based on spi value, and then stored in
> > > corresponding sa rule. This approach assumes, that every sp rule for
> > > inline crypto have valid addresses, as well as range of addresses is
> > > not supported.
> > >
> > > New flags for ipsec_sa structure are required to distinguish between
> > > IPv4 and IPv6 transport modes. Because of that, there is need to
> > > change all checks done on these flags, so they work as expected.
> > >
> > > Fixes: ec17993a145a ("examples/ipsec-secgw: support security
> > > offload")
> > > Fixes: 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
> > >
> > This is a very well written description. Thanks. This helps in review of the patch.
> >
> > I have a few small comments, rest all is fine.
> >
> > > Signed-off-by: Mariusz Drost <mariuszx.drost@intel.com>
> > > ---
> > >  examples/ipsec-secgw/esp.c   |  12 +--
> > >  examples/ipsec-secgw/ipsec.c |  19 +++--
> > > examples/ipsec-secgw/ipsec.h |  21 +++++-
> > >  examples/ipsec-secgw/sa.c    | 142 ++++++++++++++++++++++++++---------
> > >  examples/ipsec-secgw/sp4.c   |  24 +++++-
> > >  examples/ipsec-secgw/sp6.c   |  42 ++++++++++-
> > >  6 files changed, 205 insertions(+), 55 deletions(-)
> > >
> > > diff --git a/examples/ipsec-secgw/esp.c b/examples/ipsec-secgw/esp.c
> > > index f11d095ba..764e08dcf 100644
> > > --- a/examples/ipsec-secgw/esp.c
> > > +++ b/examples/ipsec-secgw/esp.c
> > > @@ -192,7 +192,7 @@ esp_inbound_post(struct rte_mbuf *m, struct
> > > ipsec_sa *sa,
> > >  		}
> > >  	}
> > >
> > > -	if (unlikely(sa->flags == TRANSPORT)) {
> > > +	if (unlikely(IS_TRANSPORT(sa->flags))) {
> > >  		ip = rte_pktmbuf_mtod(m, struct ip *);
> > >  		ip4 = (struct ip *)rte_pktmbuf_adj(m,
> > >  				sizeof(struct rte_esp_hdr) + sa->iv_len); @@ -
> 233,13 +233,13 @@
> > > esp_outbound(struct rte_mbuf *m, struct ipsec_sa *sa,
> > >
> > >  	ip4 = rte_pktmbuf_mtod(m, struct ip *);
> > >  	if (likely(ip4->ip_v == IPVERSION)) {
> > > -		if (unlikely(sa->flags == TRANSPORT)) {
> > > +		if (unlikely(IS_TRANSPORT(sa->flags))) {
> > >  			ip_hdr_len = ip4->ip_hl * 4;
> > >  			nlp = ip4->ip_p;
> > >  		} else
> > >  			nlp = IPPROTO_IPIP;
> > >  	} else if (ip4->ip_v == IP6_VERSION) {
> > > -		if (unlikely(sa->flags == TRANSPORT)) {
> > > +		if (unlikely(IS_TRANSPORT(sa->flags))) {
> > >  			/* XXX No option headers supported */
> > >  			ip_hdr_len = sizeof(struct ip6_hdr);
> > >  			ip6 = (struct ip6_hdr *)ip4;
> > > @@ -258,13 +258,13 @@ esp_outbound(struct rte_mbuf *m, struct
> > > ipsec_sa *sa,
> > >  	pad_len = pad_payload_len + ip_hdr_len - rte_pktmbuf_pkt_len(m);
> > >
> > >  	RTE_ASSERT(sa->flags == IP4_TUNNEL || sa->flags == IP6_TUNNEL ||
> > > -			sa->flags == TRANSPORT);
> > > +			IS_TRANSPORT(sa->flags));
> > I can see that at multiple places, sa->flags are accessed without your
> > defined macros. Could you please update this at all places, so that it
> > will be uniform across the application.
> >
> > >
> > >  	if (likely(sa->flags == IP4_TUNNEL))
> > >  		ip_hdr_len = sizeof(struct ip);
> > >  	else if (sa->flags == IP6_TUNNEL)
> > >  		ip_hdr_len = sizeof(struct ip6_hdr);
> > > -	else if (sa->flags != TRANSPORT) {
> > > +	else if (!IS_TRANSPORT(sa->flags)) {
> > >  		RTE_LOG(ERR, IPSEC_ESP, "Unsupported SA flags: 0x%x\n",
> > >  				sa->flags);
> > >  		return -EINVAL;
> > > @@ -291,7 +291,7 @@ esp_outbound(struct rte_mbuf *m, struct ipsec_sa
> *sa,
> > >  		rte_prefetch0(padding);
> > >  	}
> > >
> > > -	switch (sa->flags) {
> > > +	switch (WITHOUT_TRANSPORT_VERSION(sa->flags)) {
> > I do not get the intent of this macro " WITHOUT_TRANSPORT_VERSION ".
> > could you explain this in comments or some better name of the macro.
> >
> > >  	case IP4_TUNNEL:
> > >  		ip4 = ip4ip_outbound(m, sizeof(struct rte_esp_hdr) + sa->iv_len,
> > >  				&sa->src, &sa->dst);
> >
> >
> > Regards,
> > Akhil