Re: [dpdk-dev] [PATCH v6 07/10] ipsec: add support for NAT-T

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> 
> Add support for the IPsec NAT-Traversal use case for Tunnel mode
> packets.
> 
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> Signed-off-by: Abhijit Sinha <abhijit.sinha@intel.com>
> Signed-off-by: Daniel Martin Buckley <daniel.m.buckley@intel.com>
> Acked-by: Fan Zhang <roy.fan.zhang@intel.com>
> ---
>  lib/ipsec/iph.h          | 17 +++++++++++++++++
>  lib/ipsec/rte_ipsec_sa.h |  8 +++++++-
>  lib/ipsec/sa.c           | 13 ++++++++++++-
>  lib/ipsec/sa.h           |  4 ++++
>  4 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/ipsec/iph.h b/lib/ipsec/iph.h
> index 2d223199ac..c5c213a2b4 100644
> --- a/lib/ipsec/iph.h
> +++ b/lib/ipsec/iph.h
> @@ -251,6 +251,7 @@ update_tun_outb_l3hdr(const struct rte_ipsec_sa *sa, void *outh,
>  {
>  	struct rte_ipv4_hdr *v4h;
>  	struct rte_ipv6_hdr *v6h;
> +	struct rte_udp_hdr *udph;
>  	uint8_t is_outh_ipv4;
> 
>  	if (sa->type & RTE_IPSEC_SATP_MODE_TUNLV4) {
> @@ -258,11 +259,27 @@ update_tun_outb_l3hdr(const struct rte_ipsec_sa *sa, void *outh,
>  		v4h = outh;
>  		v4h->packet_id = pid;
>  		v4h->total_length = rte_cpu_to_be_16(plen - l2len);
> +
> +		if (sa->type & RTE_IPSEC_SATP_NATT_ENABLE) {
> +			udph = (struct rte_udp_hdr *)(v4h + 1);
> +			udph->dst_port = sa->natt.dport;
> +			udph->src_port = sa->natt.sport;
> +			udph->dgram_len = rte_cpu_to_be_16(plen - l2len -
> +				(sizeof(*v4h) + sizeof(*udph)));
> +		}
>  	} else {
>  		is_outh_ipv4 = 0;
>  		v6h = outh;
>  		v6h->payload_len = rte_cpu_to_be_16(plen - l2len -
>  				sizeof(*v6h));
> +
> +		if (sa->type & RTE_IPSEC_SATP_NATT_ENABLE) {
> +			udph = (struct rte_udp_hdr *)(v6h + 1);

Why you presume there would be always ipv6 with no options?
Shouldn't we use hdr_l3_len provided by user?
Another thing - I am not sure we need 'natt' field in rte_ipsec_sa at all.
UDP header (sport, dport) is consitant and could be part of header template
provided by user at sa initialization time.

> +			udph->dst_port = sa->natt.dport;
> +			udph->src_port = sa->natt.sport;
> +			udph->dgram_len = rte_cpu_to_be_16(plen - l2len -
> +				(sizeof(*v6h) + sizeof(*udph)));

Whose responsibility will be to update cksum field?

> +		}
>  	}
> 
>  	if (sa->type & TUN_HDR_MSK)
> diff --git a/lib/ipsec/rte_ipsec_sa.h b/lib/ipsec/rte_ipsec_sa.h
> index cf51ad8338..40d1e70d45 100644
> --- a/lib/ipsec/rte_ipsec_sa.h
> +++ b/lib/ipsec/rte_ipsec_sa.h
> @@ -76,6 +76,7 @@ struct rte_ipsec_sa_prm {
>   * - inbound/outbound
>   * - mode (TRANSPORT/TUNNEL)
>   * - for TUNNEL outer IP version (IPv4/IPv6)
> + * - NAT-T UDP encapsulated (TUNNEL mode only)
>   * - are SA SQN operations 'atomic'
>   * - ESN enabled/disabled
>   * ...
> @@ -86,7 +87,8 @@ enum {
>  	RTE_SATP_LOG2_PROTO,
>  	RTE_SATP_LOG2_DIR,
>  	RTE_SATP_LOG2_MODE,
> -	RTE_SATP_LOG2_SQN = RTE_SATP_LOG2_MODE + 2,
> +	RTE_SATP_LOG2_NATT = RTE_SATP_LOG2_MODE + 2,

Why to insert it in the middle?
Why not to add to the end, as usually people do for new options?

> +	RTE_SATP_LOG2_SQN,
>  	RTE_SATP_LOG2_ESN,
>  	RTE_SATP_LOG2_ECN,
>  	RTE_SATP_LOG2_DSCP
> @@ -109,6 +111,10 @@ enum {
>  #define RTE_IPSEC_SATP_MODE_TUNLV4	(1ULL << RTE_SATP_LOG2_MODE)
>  #define RTE_IPSEC_SATP_MODE_TUNLV6	(2ULL << RTE_SATP_LOG2_MODE)
> 
> +#define RTE_IPSEC_SATP_NATT_MASK	(1ULL << RTE_SATP_LOG2_NATT)
> +#define RTE_IPSEC_SATP_NATT_DISABLE	(0ULL << RTE_SATP_LOG2_NATT)
> +#define RTE_IPSEC_SATP_NATT_ENABLE	(1ULL << RTE_SATP_LOG2_NATT)
> +
>  #define RTE_IPSEC_SATP_SQN_MASK		(1ULL << RTE_SATP_LOG2_SQN)
>  #define RTE_IPSEC_SATP_SQN_RAW		(0ULL << RTE_SATP_LOG2_SQN)
>  #define RTE_IPSEC_SATP_SQN_ATOM		(1ULL << RTE_SATP_LOG2_SQN)
> diff --git a/lib/ipsec/sa.c b/lib/ipsec/sa.c
> index 2ecbbce0a4..8e369e4618 100644
> --- a/lib/ipsec/sa.c
> +++ b/lib/ipsec/sa.c
> @@ -217,6 +217,10 @@ fill_sa_type(const struct rte_ipsec_sa_prm *prm, uint64_t *type)
>  	} else
>  		return -EINVAL;
> 
> +	/* check for UDP encapsulation flag */
> +	if (prm->ipsec_xform.options.udp_encap == 1)
> +		tp |= RTE_IPSEC_SATP_NATT_ENABLE;
> +
>  	/* check for ESN flag */
>  	if (prm->ipsec_xform.options.esn == 0)
>  		tp |= RTE_IPSEC_SATP_ESN_DISABLE;
> @@ -372,7 +376,8 @@ esp_sa_init(struct rte_ipsec_sa *sa, const struct rte_ipsec_sa_prm *prm,
>  	const struct crypto_xform *cxf)
>  {
>  	static const uint64_t msk = RTE_IPSEC_SATP_DIR_MASK |
> -				RTE_IPSEC_SATP_MODE_MASK;
> +				RTE_IPSEC_SATP_MODE_MASK |
> +				RTE_IPSEC_SATP_NATT_MASK;
> 
>  	if (prm->ipsec_xform.options.ecn)
>  		sa->tos_mask |= RTE_IPV4_HDR_ECN_MASK;
> @@ -475,10 +480,16 @@ esp_sa_init(struct rte_ipsec_sa *sa, const struct rte_ipsec_sa_prm *prm,
>  	case (RTE_IPSEC_SATP_DIR_IB | RTE_IPSEC_SATP_MODE_TRANS):
>  		esp_inb_init(sa);
>  		break;
> +	case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TUNLV4 |
> +			RTE_IPSEC_SATP_NATT_ENABLE):
> +	case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TUNLV6 |
> +			RTE_IPSEC_SATP_NATT_ENABLE):
>  	case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TUNLV4):
>  	case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TUNLV6):
>  		esp_outb_tun_init(sa, prm);
>  		break;
> +	case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TRANS |
> +			RTE_IPSEC_SATP_NATT_ENABLE):
>  	case (RTE_IPSEC_SATP_DIR_OB | RTE_IPSEC_SATP_MODE_TRANS):
>  		esp_outb_init(sa, 0);
>  		break;
> diff --git a/lib/ipsec/sa.h b/lib/ipsec/sa.h
> index 5e237f3525..3f38921eb3 100644
> --- a/lib/ipsec/sa.h
> +++ b/lib/ipsec/sa.h
> @@ -101,6 +101,10 @@ struct rte_ipsec_sa {
>  		uint64_t msk;
>  		uint64_t val;
>  	} tx_offload;
> +	struct {
> +		uint16_t sport;
> +		uint16_t dport;
> +	} natt;
>  	uint32_t salt;
>  	uint8_t algo_type;
>  	uint8_t proto;    /* next proto */
> --
> 2.25.1