Re: [dpdk-dev] [PATCH v6 09/10] ipsec: add support for initial SQN value

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

> Update IPsec library to support initial SQN value.
> 
> 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/esp_outb.c | 19 ++++++++++++-------
>  lib/ipsec/sa.c       | 29 ++++++++++++++++++++++-------
>  2 files changed, 34 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/ipsec/esp_outb.c b/lib/ipsec/esp_outb.c
> index 2c02c3bb12..8a6d09558f 100644
> --- a/lib/ipsec/esp_outb.c
> +++ b/lib/ipsec/esp_outb.c
> @@ -661,7 +661,7 @@ esp_outb_sqh_process(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],
>   */
>  static inline void
>  inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss,
> -	struct rte_mbuf *mb[], uint16_t num)
> +	struct rte_mbuf *mb[], uint16_t num, uint64_t *sqn)
>  {
>  	uint32_t i, ol_flags, bytes = 0;
> 
> @@ -672,7 +672,7 @@ inline_outb_mbuf_prepare(const struct rte_ipsec_session *ss,
>  		bytes += mb[i]->data_len;
>  		if (ol_flags != 0)
>  			rte_security_set_pkt_metadata(ss->security.ctx,
> -				ss->security.ses, mb[i], NULL);
> +				ss->security.ses, mb[i], sqn);


rte_security_set_pkt_metadata() doc says that param is device specific parameter...
Could you probably explain what is the intention here:
Why we need to set pointer to sqn value as device specific parameter?
What PMD expects to do here?
What will happen if PMD expects that parameter to be something else
(not a pointer to sqn value)?

>  	}
>  	ss->sa->statistics.count += num;
>  	ss->sa->statistics.bytes += bytes - (ss->sa->hdr_len * num);
> @@ -764,7 +764,10 @@ inline_outb_tun_pkt_process(const struct rte_ipsec_session *ss,
>  	if (k != num && k != 0)
>  		move_bad_mbufs(mb, dr, num, num - k);
> 
> -	inline_outb_mbuf_prepare(ss, mb, k);
> +	if (sa->sqn_mask > UINT32_MAX)

Here and in other places:
there is a special macro: IS_ESN(sa) for that.

> +		inline_outb_mbuf_prepare(ss, mb, k, &sqn);
> +	else
> +		inline_outb_mbuf_prepare(ss, mb, k, NULL);

Ok, so why we need to pass sqn to metadata only for ESN case?
Is that because inside ESP header we store only lower 32-bits of SQN value?
But, as I remember SQN.hi  are still stored inside the packet, just in different place
(between ESP trailer and ICV).

>  	return k;
>  }
> 
> @@ -799,8 +802,7 @@ inline_outb_trs_pkt_process(const struct rte_ipsec_session *ss,
>  	if (nb_sqn_alloc != nb_sqn)
>  		rte_errno = EOVERFLOW;
> 
> -	k = 0;
> -	for (i = 0; i != num; i++) {
> +	for (i = 0, k = 0; i != num; i++) {

No reason for change.

> 
>  		sqc = rte_cpu_to_be_64(sqn + i);
>  		gen_iv(iv, sqc);
> @@ -828,7 +830,10 @@ inline_outb_trs_pkt_process(const struct rte_ipsec_session *ss,
>  	if (k != num && k != 0)
>  		move_bad_mbufs(mb, dr, num, num - k);
> 
> -	inline_outb_mbuf_prepare(ss, mb, k);
> +	if (sa->sqn_mask > UINT32_MAX)
> +		inline_outb_mbuf_prepare(ss, mb, k, &sqn);
> +	else
> +		inline_outb_mbuf_prepare(ss, mb, k, NULL);
>  	return k;
>  }
> 
> @@ -840,6 +845,6 @@ uint16_t
>  inline_proto_outb_pkt_process(const struct rte_ipsec_session *ss,
>  	struct rte_mbuf *mb[], uint16_t num)
>  {
> -	inline_outb_mbuf_prepare(ss, mb, num);
> +	inline_outb_mbuf_prepare(ss, mb, num, NULL);
>  	return num;
>  }
> diff --git a/lib/ipsec/sa.c b/lib/ipsec/sa.c
> index 5b55bbc098..d94684cf96 100644
> --- a/lib/ipsec/sa.c
> +++ b/lib/ipsec/sa.c
> @@ -294,11 +294,11 @@ esp_inb_tun_init(struct rte_ipsec_sa *sa, const struct rte_ipsec_sa_prm *prm)
>   * Init ESP outbound specific things.
>   */
>  static void
> -esp_outb_init(struct rte_ipsec_sa *sa, uint32_t hlen)
> +esp_outb_init(struct rte_ipsec_sa *sa, uint32_t hlen, uint64_t sqn)
>  {
>  	uint8_t algo_type;
> 
> -	sa->sqn.outb = 1;
> +	sa->sqn.outb = sqn;
> 
>  	algo_type = sa->algo_type;
> 
> @@ -356,6 +356,8 @@ esp_outb_init(struct rte_ipsec_sa *sa, uint32_t hlen)
>  static void
>  esp_outb_tun_init(struct rte_ipsec_sa *sa, const struct rte_ipsec_sa_prm *prm)
>  {
> +	uint64_t sqn = prm->ipsec_xform.esn.value > 0 ?
> +			prm->ipsec_xform.esn.value : 1;

No need to do same thing twice - esp_outb_tun_init can take sqn value as a parameter.

>  	sa->proto = prm->tun.next_proto;
>  	sa->hdr_len = prm->tun.hdr_len;
>  	sa->hdr_l3_off = prm->tun.hdr_l3_off;
> @@ -366,7 +368,7 @@ esp_outb_tun_init(struct rte_ipsec_sa *sa, const struct rte_ipsec_sa_prm *prm)
> 
>  	memcpy(sa->hdr, prm->tun.hdr, sa->hdr_len);
> 
> -	esp_outb_init(sa, sa->hdr_len);
> +	esp_outb_init(sa, sa->hdr_len, sqn);
>  }
> 
>  /*
> @@ -376,6 +378,8 @@ static int
>  esp_sa_init(struct rte_ipsec_sa *sa, const struct rte_ipsec_sa_prm *prm,
>  	const struct crypto_xform *cxf)
>  {
> +	uint64_t sqn = prm->ipsec_xform.esn.value > 0 ?
> +			prm->ipsec_xform.esn.value : 1;

Here and everywhere:
Please try to keep variable definition and its value assignement at different statements.
Will keep coding-style similar across the file and is easier to follow (at least to me).

>  	static const uint64_t msk = RTE_IPSEC_SATP_DIR_MASK |
>  				RTE_IPSEC_SATP_MODE_MASK |
>  				RTE_IPSEC_SATP_NATT_MASK;
> @@ -492,7 +496,7 @@ esp_sa_init(struct rte_ipsec_sa *sa, const struct rte_ipsec_sa_prm *prm,
>  	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);
> +		esp_outb_init(sa, 0, sqn);
>  		break;
>  	}
> 
> @@ -503,15 +507,19 @@ esp_sa_init(struct rte_ipsec_sa *sa, const struct rte_ipsec_sa_prm *prm,
>   * helper function, init SA replay structure.
>   */
>  static void
> -fill_sa_replay(struct rte_ipsec_sa *sa, uint32_t wnd_sz, uint32_t nb_bucket)
> +fill_sa_replay(struct rte_ipsec_sa *sa,
> +		uint32_t wnd_sz, uint32_t nb_bucket, uint64_t sqn)
>  {
>  	sa->replay.win_sz = wnd_sz;
>  	sa->replay.nb_bucket = nb_bucket;
>  	sa->replay.bucket_index_mask = nb_bucket - 1;
>  	sa->sqn.inb.rsn[0] = (struct replay_sqn *)(sa + 1);
> -	if ((sa->type & RTE_IPSEC_SATP_SQN_MASK) == RTE_IPSEC_SATP_SQN_ATOM)
> +	sa->sqn.inb.rsn[0]->sqn = sqn;
> +	if ((sa->type & RTE_IPSEC_SATP_SQN_MASK) == RTE_IPSEC_SATP_SQN_ATOM) {
>  		sa->sqn.inb.rsn[1] = (struct replay_sqn *)
>  			((uintptr_t)sa->sqn.inb.rsn[0] + rsn_size(nb_bucket));
> +		sa->sqn.inb.rsn[1]->sqn = sqn;
> +	}
>  }
> 
>  int
> @@ -830,13 +838,20 @@ rte_ipsec_sa_init(struct rte_ipsec_sa *sa, const struct rte_ipsec_sa_prm *prm,
>  	sa->sqn_mask = (prm->ipsec_xform.options.esn == 0) ?
>  		UINT32_MAX : UINT64_MAX;
> 
> +	/* if we are starting from a non-zero sn value */
> +	if (prm->ipsec_xform.esn.value > 0) {
> +		if (prm->ipsec_xform.direction ==
> +				RTE_SECURITY_IPSEC_SA_DIR_EGRESS)
> +			sa->sqn.outb = prm->ipsec_xform.esn.value;

Hmm... you do set sa->sqn.outb inside esp_outb_init().
Why do you need to duplicate it here?

> +	}
> +
>  	rc = esp_sa_init(sa, prm, &cxf);
>  	if (rc != 0)
>  		rte_ipsec_sa_fini(sa);
> 
>  	/* fill replay window related fields */
>  	if (nb != 0)
> -		fill_sa_replay(sa, wsz, nb);
> +		fill_sa_replay(sa, wsz, nb, prm->ipsec_xform.esn.value);
> 
>  	return sz;
>  }
> --
> 2.25.1