[dpdk-dev] [PATCH 6/9] ipsec: implement SA data-path API

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Tuesday, November 20, 2018 9:44 AM
> To: Zhang, Qi Z <qi.z.zhang at intel.com>; dev at dpdk.org
> Cc: Awal, Mohammad Abdul <mohammad.abdul.awal at intel.com>
> Subject: Re: [dpdk-dev] [PATCH 6/9] ipsec: implement SA data-path API
> 
> 
> Hi Qi,
> 
> >
> > Hi Konstantin and Awal:
> >
> > 	I have couple questions for this patch.
> > 	please forgive me if they are obvious, since I don't have much insight on IPsec, but I may work on related stuff in future :)
> >
> > > +static inline int32_t
> > > +esp_outb_tun_pkt_prepare(struct rte_ipsec_sa *sa, rte_be64_t sqc,
> > > +	const uint64_t ivp[IPSEC_MAX_IV_QWORD], struct rte_mbuf *mb,
> > > +	union sym_op_data *icv)
> > > +{
> > > +	uint32_t clen, hlen, pdlen, pdofs, tlen;
> > > +	struct rte_mbuf *ml;
> > > +	struct esp_hdr *esph;
> > > +	struct esp_tail *espt;
> > > +	char *ph, *pt;
> > > +	uint64_t *iv;
> > > +
> > > +	/* calculate extra header space required */
> > > +	hlen = sa->hdr_len + sa->iv_len + sizeof(*esph);
> > > +
> > > +	/* number of bytes to encrypt */
> > > +	clen = mb->pkt_len + sizeof(*espt);
> > > +	clen = RTE_ALIGN_CEIL(clen, sa->pad_align);
> > > +
> > > +	/* pad length + esp tail */
> > > +	pdlen = clen - mb->pkt_len;
> > > +	tlen = pdlen + sa->icv_len;
> > > +
> > > +	/* do append and prepend */
> > > +	ml = rte_pktmbuf_lastseg(mb);
> > > +	if (tlen + sa->sqh_len + sa->aad_len > rte_pktmbuf_tailroom(ml))
> > > +		return -ENOSPC;
> > > +
> > > +	/* prepend header */
> > > +	ph = rte_pktmbuf_prepend(mb, hlen);
> > > +	if (ph == NULL)
> > > +		return -ENOSPC;
> > > +
> > > +	/* append tail */
> > > +	pdofs = ml->data_len;
> > > +	ml->data_len += tlen;
> > > +	mb->pkt_len += tlen;
> > > +	pt = rte_pktmbuf_mtod_offset(ml, typeof(pt), pdofs);
> > > +
> > > +	/* update pkt l2/l3 len */
> > > +	mb->l2_len = sa->hdr_l3_off;
> > > +	mb->l3_len = sa->hdr_len - sa->hdr_l3_off;
> > > +
> > > +	/* copy tunnel pkt header */
> > > +	rte_memcpy(ph, sa->hdr, sa->hdr_len);
> >
> > I didn't get this, my understand is:
> >
> > for tunnel mode if an original packet is
> >
> > 	Eth + IP + UDP/TCP + data,
> 
> It  is assumed that input mbuf doesn't contain L2 header already (only L3/L4/...)
> That's why we don't shift L2 header.
> Probably have to put that into public API comments.

After another thought - probably it is better to support the case when L2 is not stripped too here.
After all we do support it for other modes (inbound tunnel/transport, outbound transport).
Will try to add it into v2.
Konstantin

> 
> >
> > after encap, it should become
> >
> > 	Eth + encap header (IP or IP + UDP) + ESP Header + IP + UDP/TCP + Data + ESP Tailer...
> >
> > So after rte_pktmbuf_prepend shouldn't we do below
> >
> > 1) shift L2 HEAD (Eth) ahead
> > 2) copy encap header and ESP header to the hole.
> > ?
> >
> > But now we just copy the sa->hdr on the pre-pend space directly? What is the sa->hdr supposed to be?
> 
> Optional L2 header and new L3 header.
> 
> > but no matter what is it, we encap
> > everything before the packet?
> > BTW, is UDP encapsulation also be considered here?
> 
> Right now - no.
> Might be later, if there would be a request for it.
> 
> >, I didn't figure out how a IP + UDP header should be configured with sa->hdr , sa-
> > >hdr_l3_off, sa->hdr_len for this case
> >
> > > +static inline int
> > > +esp_inb_tun_single_pkt_process(struct rte_ipsec_sa *sa, struct rte_mbuf
> > > *mb,
> > > +	uint32_t *sqn)
> > > +{
> > > +	uint32_t hlen, icv_len, tlen;
> > > +	struct esp_hdr *esph;
> > > +	struct esp_tail *espt;
> > > +	struct rte_mbuf *ml;
> > > +	char *pd;
> > > +
> > > +	if (mb->ol_flags & PKT_RX_SEC_OFFLOAD_FAILED)
> > > +		return -EBADMSG;
> > > +
> > > +	icv_len = sa->icv_len;
> > > +
> > > +	ml = rte_pktmbuf_lastseg(mb);
> > > +	espt = rte_pktmbuf_mtod_offset(ml, struct esp_tail *,
> > > +		ml->data_len - icv_len - sizeof(*espt));
> >
> > What kind of mechanism is to guarantee that last segment will always cover the esp tail?( data_len >= icv_len + sizeof (*espt))
> > Is that possible the esp tail be split into multi-segment for jumbo frames caes?
> 
> It is possible, though right now we don't support such cases.
> Right now it is up to the caller to make sure last segment contains espt+icv.
> Plan to add proper multi-seg support later (most likely 19.05).
> Konstantin