Re: [dpdk-dev] [PATCH 2/3] ipsec: fix transport mode for ipv6 with extensions

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Marcin,

> 
> Reconstructing IPv6 header after encryption or decryption requires
> updating 'next header' value in the preceding protocol header, which
> is determined by parsing IPv6 header and iteratively looking for
> next IPv6 header extension.
> 
> It is required that 'l3_len' in the mbuf metadata contains a total
> length of the IPv6 header with header extensions up to ESP header.
> 
> Signed-off-by: Marcin Smoczynski <marcinx.smoczynski@intel.com>
> ---
>  lib/Makefile           |  3 ++-
>  lib/librte_ipsec/iph.h | 55 ++++++++++++++++++++++++++++++++++++------
>  2 files changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/Makefile b/lib/Makefile
> index 791e0d991..3ad579f68 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -108,7 +108,8 @@ DEPDIRS-librte_gso += librte_mempool
>  DIRS-$(CONFIG_RTE_LIBRTE_BPF) += librte_bpf
>  DEPDIRS-librte_bpf := librte_eal librte_mempool librte_mbuf librte_ethdev
>  DIRS-$(CONFIG_RTE_LIBRTE_IPSEC) += librte_ipsec
> -DEPDIRS-librte_ipsec := librte_eal librte_mbuf librte_cryptodev librte_security
> +DEPDIRS-librte_ipsec := librte_eal librte_mbuf librte_cryptodev librte_security \
> +			librte_net

A nit.
Please update the comment in lib/meson.build file for the dependencies. Currently it is only for crypto and security.

>  DIRS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += librte_telemetry
>  DEPDIRS-librte_telemetry := librte_eal librte_metrics librte_ethdev
>  DIRS-$(CONFIG_RTE_LIBRTE_RCU) += librte_rcu
> diff --git a/lib/librte_ipsec/iph.h b/lib/librte_ipsec/iph.h
> index 58930cf18..082e4e73e 100644
> --- a/lib/librte_ipsec/iph.h
> +++ b/lib/librte_ipsec/iph.h
> @@ -5,6 +5,8 @@
>  #ifndef _IPH_H_
>  #define _IPH_H_
> 
> +#include <rte_ip.h>
> +
>  /**
>   * @file iph.h
>   * Contains functions/structures/macros to manipulate IPv4/IPv6 headers
> @@ -40,24 +42,61 @@ static inline int
>  update_trs_l3hdr(const struct rte_ipsec_sa *sa, void *p, uint32_t plen,
>  		uint32_t l2len, uint32_t l3len, uint8_t proto)
>  {
> -	struct ipv4_hdr *v4h;
> -	struct ipv6_hdr *v6h;
>  	int32_t rc;
> 
> +	/* IPv4 */
>  	if ((sa->type & RTE_IPSEC_SATP_IPV_MASK) == RTE_IPSEC_SATP_IPV4) {
> +		struct ipv4_hdr *v4h;
> +
>  		v4h = p;
>  		rc = v4h->next_proto_id;
>  		v4h->next_proto_id = proto;
>  		v4h->total_length = rte_cpu_to_be_16(plen - l2len);
> -	} else if (l3len == sizeof(*v6h)) {
> +	/* IPv6 */
> +	} else {
> +		struct ipv6_hdr *v6h;
> +		uint8_t *next_proto_off;
> +
>  		v6h = p;
> -		rc = v6h->proto;
> -		v6h->proto = proto;
> +
> +		/* basic IPv6 header with no extensions */
> +		if (l3len == sizeof(struct ipv6_hdr))
> +			next_proto_off = &v6h->proto;

Is this next_proto_off a pointer to an offset or the value of the next_proto. So IMO the name should be next_proto or it should be p_nh
> +
> +		/* IPv6 with extensions */
> +		else {
> +			size_t ext_len;
> +			int nh;
> +			uint8_t *pd, *plimit;
> +
> +			/* locate last extension within l3len bytes */
> +			pd = (uint8_t *)p;
> +			plimit = pd + l3len;
> +			ext_len = sizeof(struct ipv6_hdr);
> +			nh = v6h->proto;
> +			while (pd + ext_len < plimit) {
> +				pd += ext_len;
> +				nh = rte_ipv6_get_next_ext(pd, nh, &ext_len);
> +				if (unlikely(nh < 0))
> +					return -EINVAL;
> +			}
> +
> +			/* invalid l3len - extension exceeds header length */
> +			if (unlikely(pd + ext_len != plimit))
> +				return -EINVAL;
> +
> +			/* save last extension offset */
> +			next_proto_off = pd;
> +		}
> +
> +		/* update header type; return original value */
> +		rc = *next_proto_off;
> +		*next_proto_off = proto;
> +
> +		/* fix packet length */
>  		v6h->payload_len = rte_cpu_to_be_16(plen - l2len -
>  				sizeof(*v6h));
> -	/* need to add support for IPv6 with options */
> -	} else
> -		rc = -ENOTSUP;
> +	}
> 
>  	return rc;
>  }
> --
> 2.21.0.windows.1