[dpdk-dev] [PATCH v2 08/10] ipsec: add rte_security cpu_crypto action support

Ananyev, Konstantin konstantin.ananyev at intel.com
Wed Oct 9 01:28:35 CEST 2019


Hi Fan,
Comments for inbound part inline.
As I can see majority of my v1 comments still are not addressed.
Please check.
Konstantin

> 
> This patch updates the ipsec library to handle the newly introduced
> RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO action.
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang at intel.com>
> ---
>  lib/librte_ipsec/crypto.h   |  24 +++
>  lib/librte_ipsec/esp_inb.c  | 200 ++++++++++++++++++++++--
>  lib/librte_ipsec/esp_outb.c | 369 +++++++++++++++++++++++++++++++++++++++++---
>  lib/librte_ipsec/sa.c       |  53 ++++++-
>  lib/librte_ipsec/sa.h       |  29 ++++
>  lib/librte_ipsec/ses.c      |   4 +-
>  6 files changed, 643 insertions(+), 36 deletions(-)
> 
> diff --git a/lib/librte_ipsec/crypto.h b/lib/librte_ipsec/crypto.h
> index f8fbf8d4f..901c8c7de 100644
> --- a/lib/librte_ipsec/crypto.h
> +++ b/lib/librte_ipsec/crypto.h
> @@ -179,4 +179,28 @@ lksd_none_cop_prepare(struct rte_crypto_op *cop,
>  	__rte_crypto_sym_op_attach_sym_session(sop, cs);
>  }
> 
> +typedef void* (*_set_icv_f)(void *val, struct rte_mbuf *ml, uint32_t icv_off);
> +
> +static inline void *
> +set_icv_va_pa(void *val, struct rte_mbuf *ml, uint32_t icv_off)
> +{
> +	union sym_op_data *icv = val;
> +
> +	icv->va = rte_pktmbuf_mtod_offset(ml, void *, icv_off);
> +	icv->pa = rte_pktmbuf_iova_offset(ml, icv_off);
> +
> +	return icv->va;
> +}
> +
> +static inline void *
> +set_icv_va(__rte_unused void *val, __rte_unused struct rte_mbuf *ml,
> +		__rte_unused uint32_t icv_off)
> +{
> +	void **icv_va = val;
> +
> +	*icv_va = rte_pktmbuf_mtod_offset(ml, void *, icv_off);
> +
> +	return *icv_va;
> +}
> +
>  #endif /* _CRYPTO_H_ */
> diff --git a/lib/librte_ipsec/esp_inb.c b/lib/librte_ipsec/esp_inb.c
> index 8e3ecbc64..c4476e819 100644
> --- a/lib/librte_ipsec/esp_inb.c
> +++ b/lib/librte_ipsec/esp_inb.c
> @@ -105,6 +105,78 @@ inb_cop_prepare(struct rte_crypto_op *cop,
>  	}
>  }
> 
> +static inline int
> +inb_cpu_crypto_proc_prepare(const struct rte_ipsec_sa *sa, struct rte_mbuf *mb,
> +	uint32_t pofs, uint32_t plen,
> +	struct rte_security_vec *buf, struct iovec *cur_vec,
> +	void *iv)
> +{
> +	struct rte_mbuf *ms;
> +	struct iovec *vec = cur_vec;
> +	struct aead_gcm_iv *gcm;
> +	struct aesctr_cnt_blk *ctr;
> +	uint64_t *ivp;
> +	uint32_t algo;
> +	uint32_t left;
> +	uint32_t off = 0, n_seg = 0;

Same comment as for v1:
Please separate variable definition and value assignment.
It makes it hard to read, plus we don't do that in the rest of the library,
so better to follow rest of the code style.

> +
> +	ivp = rte_pktmbuf_mtod_offset(mb, uint64_t *,
> +		pofs + sizeof(struct rte_esp_hdr));
> +	algo = sa->algo_type;
> +
> +	switch (algo) {
> +	case ALGO_TYPE_AES_GCM:
> +		gcm = (struct aead_gcm_iv *)iv;
> +		aead_gcm_iv_fill(gcm, ivp[0], sa->salt);
> +		off = sa->ctp.cipher.offset + pofs;
> +		left = plen - sa->ctp.cipher.length;
> +		break;
> +	case ALGO_TYPE_AES_CBC:
> +	case ALGO_TYPE_3DES_CBC:
> +		copy_iv(iv, ivp, sa->iv_len);
> +		off = sa->ctp.auth.offset + pofs;
> +		left = plen - sa->ctp.auth.length;
> +		break;
> +	case ALGO_TYPE_AES_CTR:
> +		copy_iv(iv, ivp, sa->iv_len);
> +		off = sa->ctp.auth.offset + pofs;
> +		left = plen - sa->ctp.auth.length;
> +		ctr = (struct aesctr_cnt_blk *)iv;
> +		aes_ctr_cnt_blk_fill(ctr, ivp[0], sa->salt);
> +		break;
> +	case ALGO_TYPE_NULL:
> +		left = plen - sa->ctp.cipher.length;
> +		break;
> +	default:
> +		return -EINVAL;

How we can endup here?
If we have an unknown algorithm, shouldn't we fail at init stage?

> +	}
> +
> +	ms = mbuf_get_seg_ofs(mb, &off);
> +	if (!ms)
> +		return -1;

Same comment as for v1:
inb_pkt_prepare() should already check that we have a valid packet.
I don't think there is a need to check for any failure here.
Another thing, our esp header will be in the first segment for sure,
so do we need get_seg_ofs() here at all?


> +
> +	while (n_seg < RTE_LIBRTE_IP_FRAG_MAX_FRAG && left && ms) {
> +		uint32_t len = RTE_MIN(left, ms->data_len - off);


Again, same comments as for v1:

- I don't think this is right, we shouldn't impose additional limitations to
the number of segments in the packet.

- Whole construction seems a bit over-complicated here...
Why just not have a separate function that would dill iovec[] from mbuf
And return an error if there is not enough iovec[] entries?
Something like:

static inline int
mbuf_to_iovec(const struct rte_mbuf *mb, uint32_t ofs, uint32_t len, struct iovec vec[], uint32_t num)
{
     uint32_t i;
     if (mb->nb_seg > num)
        return - mb->nb_seg;

    vec[0].iov_base =  rte_pktmbuf_mtod_offset(mb, void *, off);
    vec[0].iov_len = mb->data_len - off;

    for (i = 1, ms = mb->next; mb != NULL; ms = ms->next, i++) {
        vec[i].iov_base = rte_pktmbuf_mtod(ms);
        vec[i].iov_len = ms->data_len;
    }

   vec[i].iov_len -= mb->pkt_len - len;
   return i;
}

Then we can use that function to fill our iovec[] in a loop.

L- ooking at this function, it seems to consist of 2 separate parts:
1. calculates offset and generates iv
2. setup iovec[].
Probably worth to split it into 2 separate functions like that.
Would be much easier to read/understand.

> +
> +		vec->iov_base = rte_pktmbuf_mtod_offset(ms, void *, off);
> +		vec->iov_len = len;
> +
> +		left -= len;
> +		vec++;
> +		n_seg++;
> +		ms = ms->next;
> +		off = 0;
> +	}
> +
> +	if (left)
> +		return -1;
> +
> +	buf->vec = cur_vec;
> +	buf->num = n_seg;
> +
> +	return n_seg;
> +}
> +
>  /*
>   * Helper function for prepare() to deal with situation when
>   * ICV is spread by two segments. Tries to move ICV completely into the
> @@ -139,20 +211,21 @@ move_icv(struct rte_mbuf *ml, uint32_t ofs)
>   */
>  static inline void
>  inb_pkt_xprepare(const struct rte_ipsec_sa *sa, rte_be64_t sqc,
> -	const union sym_op_data *icv)
> +	uint8_t *icv_va, void *aad_buf, uint32_t aad_off)
>  {
>  	struct aead_gcm_aad *aad;
> 
>  	/* insert SQN.hi between ESP trailer and ICV */
>  	if (sa->sqh_len != 0)
> -		insert_sqh(sqn_hi32(sqc), icv->va, sa->icv_len);
> +		insert_sqh(sqn_hi32(sqc), icv_va, sa->icv_len);
> 
>  	/*
>  	 * fill AAD fields, if any (aad fields are placed after icv),
>  	 * right now we support only one AEAD algorithm: AES-GCM.
>  	 */
>  	if (sa->aad_len != 0) {
> -		aad = (struct aead_gcm_aad *)(icv->va + sa->icv_len);
> +		aad = aad_buf ? aad_buf :
> +				(struct aead_gcm_aad *)(icv_va + aad_off);
>  		aead_gcm_aad_fill(aad, sa->spi, sqc, IS_ESN(sa));
>  	}
>  }
> @@ -162,13 +235,15 @@ inb_pkt_xprepare(const struct rte_ipsec_sa *sa, rte_be64_t sqc,
>   */
>  static inline int32_t
>  inb_pkt_prepare(const struct rte_ipsec_sa *sa, const struct replay_sqn *rsn,
> -	struct rte_mbuf *mb, uint32_t hlen, union sym_op_data *icv)
> +	struct rte_mbuf *mb, uint32_t hlen, _set_icv_f set_icv, void *icv_val,
> +	void *aad_buf)

This whole construct with another function pointer , overloaded arguments, etc.,
looks a bit clumsy and overcomplicated.
I think it would be much cleaner and easier to re-arrange the code like that:

1. update inb_pkt_xprepare to take aad buffer pointer and aad len as a parameters:

static inline void
inb_pkt_xprepare(const struct rte_ipsec_sa *sa, rte_be64_t sqc,
        const union sym_op_data *icv, void *aad, uint32_t aad_len)
{
        /* insert SQN.hi between ESP trailer and ICV */
        if (sa->sqh_len != 0)
                insert_sqh(sqn_hi32(sqc), icv->va, sa->icv_len);

        /*
         * fill AAD fields, if any (aad fields are placed after icv),
         * right now we support only one AEAD algorithm: AES-GCM.
         */
        if (aad_len != 0)
                aead_gcm_aad_fill(aad, sa->spi, sqc, IS_ESN(sa));
}

2. split inb_pkt_prepare() into 2 reusable helper functions:

*
 * retrieve and reconstruct SQN, then check it, then
 * convert it back into network byte order.
 */
static inline int
inb_get_sqn(const struct rte_ipsec_sa *sa, const struct replay_sqn *rsn,
        struct rte_mbuf *mb, uint32_t hlen, rte_be64_t *sqc)
{
        int32_t rc;
        uint64_t sqn;
        struct rte_esp_hdr *esph;

        esph = rte_pktmbuf_mtod_offset(mb, struct rte_esp_hdr *, hlen);

        /*
         * retrieve and reconstruct SQN, then check it, then
         * convert it back into network byte order.
         */
        sqn = rte_be_to_cpu_32(esph->seq);
        if (IS_ESN(sa))
                sqn = reconstruct_esn(rsn->sqn, sqn, sa->replay.win_sz);

        rc = esn_inb_check_sqn(rsn, sa, sqn);
        if (rc == 0)
                *sqc = rte_cpu_to_be_64(sqn);

        return rc;
}

static inline int32_t
inb_prepare(const struct rte_ipsec_sa *sa, struct rte_mbuf *mb,
        uint32_t hlen, uint32_t aad_len, union sym_op_data *icv)
{
        uint32_t clen, icv_len, icv_ofs, plen;
        struct rte_mbuf *ml;

        /* start packet manipulation */
        plen = mb->pkt_len;
        plen = plen - hlen;

        /* check that packet has a valid length */
        clen = plen - sa->ctp.cipher.length;
        if ((int32_t)clen < 0 || (clen & (sa->pad_align - 1)) != 0)
                return -EBADMSG;

        /* find ICV location */
        icv_len = sa->icv_len;
        icv_ofs = mb->pkt_len - icv_len;

        ml = mbuf_get_seg_ofs(mb, &icv_ofs);

        /*
         * if ICV is spread by two segments, then try to
         * move ICV completely into the last segment.
         */
         if (ml->data_len < icv_ofs + icv_len) {

                ml = move_icv(ml, icv_ofs);
                if (ml == NULL)
                        return -ENOSPC;

                /* new ICV location */
                icv_ofs = 0;
        }

        icv_ofs += sa->sqh_len;

        /* we have to allocate space for AAD somewhere,
         * right now - just use free trailing space at the last segment.
         * Would probably be more convenient to reserve space for AAD
         * inside rte_crypto_op itself
         * (again for IV space is already reserved inside cop).
         */
        if (aad_len + sa->sqh_len > rte_pktmbuf_tailroom(ml))
                return -ENOSPC;

        icv->va = rte_pktmbuf_mtod_offset(ml, void *, icv_ofs);
        icv->pa = rte_pktmbuf_iova_offset(ml, icv_ofs);

        /*
         * if esn is used then high-order 32 bits are also used in ICV
         * calculation but are not transmitted, update packet length
         * to be consistent with auth data length and offset, this will
         * be subtracted from packet length in post crypto processing
         */
        mb->pkt_len += sa->sqh_len;
        ml->data_len += sa->sqh_len;

        return plen;
}      
   
3. Now inb_pkt_prepare() becomes a simple sequential invocation of these 3 sub-functions
with right parameters:

static inline int32_t
inb_pkt_prepare(const struct rte_ipsec_sa *sa, const struct replay_sqn *rsn,
        struct rte_mbuf *mb, uint32_t hlen, union sym_op_data *icv)
{
        int32_t rc;
        uint64_t sqn;
        
        rc = inb_get_sqn(sa, rsn, mb, hlen, &sqn);
        if (rc != 0)
                return rc;

        rc = inb_prepare(sa, mb, hlen, sa->aad_len, icv);
        if (rc < 0)
                return rc;

        inb_pkt_xprepare(sa, sqn, icv, icv->va + sa->icv_len, sa->aad_len);
        return rc;
}

4. And that would be version of inb_pkt_prepare for cpu_cypto:

static inline int32_t
inb_cpu_prepare(const struct rte_ipsec_sa *sa, const struct replay_sqn *rsn,
        struct rte_mbuf *mb, uint32_t hlen, union sym_op_data *icv, void *aad)
{
        int32_t rc;
        uint64_t sqn;
        
        rc = inb_get_sqn(sa, rsn, mb, hlen, &sqn);
        if (rc != 0)
                return rc;

        rc = inb_prepare(sa, mb, hlen, 0, icv);
        if (rc < 0)
                return rc;

        inb_pkt_xprepare(sa, sqn, icv, aad, sa->aad_len);
        return rc;
}


>  {
>  	int32_t rc;
>  	uint64_t sqn;
>  	uint32_t clen, icv_len, icv_ofs, plen;
>  	struct rte_mbuf *ml;
>  	struct rte_esp_hdr *esph;
> +	void *icv_va;
> 
>  	esph = rte_pktmbuf_mtod_offset(mb, struct rte_esp_hdr *, hlen);
> 
> @@ -226,8 +301,8 @@ inb_pkt_prepare(const struct rte_ipsec_sa *sa, const struct replay_sqn *rsn,
>  	if (sa->aad_len + sa->sqh_len > rte_pktmbuf_tailroom(ml))
>  		return -ENOSPC;
> 
> -	icv->va = rte_pktmbuf_mtod_offset(ml, void *, icv_ofs);
> -	icv->pa = rte_pktmbuf_iova_offset(ml, icv_ofs);
> +	icv_va = set_icv(icv_val, ml, icv_ofs);
> +	inb_pkt_xprepare(sa, sqn, icv_va, aad_buf, sa->icv_len);
> 
>  	/*
>  	 * if esn is used then high-order 32 bits are also used in ICV
> @@ -238,7 +313,6 @@ inb_pkt_prepare(const struct rte_ipsec_sa *sa, const struct replay_sqn *rsn,
>  	mb->pkt_len += sa->sqh_len;
>  	ml->data_len += sa->sqh_len;
> 
> -	inb_pkt_xprepare(sa, sqn, icv);
>  	return plen;
>  }
> 
> @@ -265,7 +339,8 @@ esp_inb_pkt_prepare(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],
>  	for (i = 0; i != num; i++) {
> 
>  		hl = mb[i]->l2_len + mb[i]->l3_len;
> -		rc = inb_pkt_prepare(sa, rsn, mb[i], hl, &icv);
> +		rc = inb_pkt_prepare(sa, rsn, mb[i], hl, set_icv_va_pa,
> +				(void *)&icv, NULL);
>  		if (rc >= 0) {
>  			lksd_none_cop_prepare(cop[k], cs, mb[i]);
>  			inb_cop_prepare(cop[k], sa, mb[i], &icv, hl, rc);
> @@ -512,7 +587,6 @@ tun_process(const struct rte_ipsec_sa *sa, struct rte_mbuf *mb[],
>  	return k;
>  }
> 
> -
>  /*
>   * *process* function for tunnel packets
>   */
> @@ -625,6 +699,114 @@ esp_inb_pkt_process(struct rte_ipsec_sa *sa, struct rte_mbuf *mb[],
>  	return n;
>  }
> 
> +/*
> + * process packets using sync crypto engine
> + */
> +static uint16_t
> +esp_inb_cpu_crypto_pkt_process(const struct rte_ipsec_session *ss,
> +		struct rte_mbuf *mb[], uint16_t num,
> +		esp_inb_process_t process)
> +{
> +	int32_t rc;
> +	uint32_t i, hl, n, p;
> +	struct rte_ipsec_sa *sa;
> +	struct replay_sqn *rsn;
> +	void *icv_va;
> +	uint32_t sqn[num];
> +	uint32_t dr[num];
> +	uint8_t sqh_len;
> +
> +	/* cpu crypto specific variables */
> +	struct rte_security_vec buf[num];
> +	struct iovec vec[RTE_LIBRTE_IP_FRAG_MAX_FRAG * num];

Same comment as for v1:
I don't think this is right, we shouldn't impose additional limitations to
the number of segments in the packet.

> +	uint32_t vec_idx = 0;
> +	uint64_t iv_buf[num][IPSEC_MAX_IV_QWORD];
> +	void *iv[num];
> +	int status[num];
> +	uint8_t *aad_buf[num][sizeof(struct aead_gcm_aad)];
> +	void *aad[num];
> +	void *digest[num];
> +	uint32_t k;
> +
> +	sa = ss->sa;
> +	rsn = rsn_acquire(sa);
> +	sqh_len = sa->sqh_len;
> +
> +	k = 0;
> +	for (i = 0; i != num; i++) {
> +		hl = mb[i]->l2_len + mb[i]->l3_len;
> +		rc = inb_pkt_prepare(sa, rsn, mb[i], hl, set_icv_va,
> +				(void *)&icv_va, (void *)aad_buf[k]);

Better to have separate function similar to inb_pkt_prepare(), see above.

> +		if (rc >= 0) {
> +			iv[k] = (void *)iv_buf[k];
> +			aad[k] = (void *)aad_buf[k];
> +			digest[k] = (void *)icv_va;
> +
> +			rc = inb_cpu_crypto_proc_prepare(sa, mb[i], hl,
> +					rc, &buf[k], &vec[vec_idx], iv[k]);
> +			if (rc < 0) {
> +				dr[i - k] = i;

I think with you current aproach you can't do like that.
As your vec[] still can contain some entries from fail mbuf.
So in theoryyou need to cleanup these entries which is quite complicated,
better to avoid such case at all. 
I think you need to reorder the code, as described above.

> +				continue;
> +			}
> +
> +			vec_idx += rc;
> +			k++;
> +		} else
> +			dr[i - k] = i;
> +	}
> +
> +	/* copy not prepared mbufs beyond good ones */
> +	if (k != num) {
> +		rte_errno = EBADMSG;
> +
> +		if (unlikely(k == 0))
> +			return 0;
> +
> +		move_bad_mbufs(mb, dr, num, num - k);
> +	}
> +
> +	/* process the packets */
> +	n = 0;
> +	rc = rte_security_process_cpu_crypto_bulk(ss->security.ctx,
> +			ss->security.ses, buf, iv, aad, digest, status, k);
> +	/* move failed process packets to dr */

Same comment as for v1:
That just doesn't look right, instead of updating dr[] and move_bad_mbufs(), you need to:
if (rc != 0) {walk through status[] and for failed ones set PKT_RX_SEC_OFFLOAD_FAILED in appropriate mbuf->ol_flags}.
tun_process(), etc. expects PKT_RX_SEC_OFFLOAD_FAILED to be set in mb->ol_flags
for failed packets.


> +	for (i = 0; i < k; i++) {
> +		if (status[i]) {
> +			dr[n++] = i;
> +			rte_errno = EBADMSG;
> +		}
> +	}
> +
> +	/* move bad packets to the back */
> +	if (n)
> +		move_bad_mbufs(mb, dr, k, n);
> +
> +	/* process packets */
> +	p = process(sa, mb, sqn, dr, k - n, sqh_len);
> +



> +	if (p != k - n && p != 0)
> +		move_bad_mbufs(mb, dr, k - n, k - n - p);
> +
> +	if (p != num)
> +		rte_errno = EBADMSG;
> +
> +	return p;
> +}
> +
> +uint16_t
> +esp_inb_tun_cpu_crypto_pkt_process(const struct rte_ipsec_session *ss,
> +		struct rte_mbuf *mb[], uint16_t num)
> +{
> +	return esp_inb_cpu_crypto_pkt_process(ss, mb, num, tun_process);
> +}
> +
> +uint16_t
> +esp_inb_trs_cpu_crypto_pkt_process(const struct rte_ipsec_session *ss,
> +		struct rte_mbuf *mb[], uint16_t num)
> +{
> +	return esp_inb_cpu_crypto_pkt_process(ss, mb, num, trs_process);
> +}
> +


More information about the dev mailing list