Re: [dpdk-dev] [PATCH v4 06/10] ipsec: implement SA data-path API

[Thomas Monjalon <thomas@monjalon.net>]

21/12/2018 15:27, Ananyev, Konstantin:
> 
> > >>> + */
> > >>> +
> > >>> +/*
> > >>> + * Move preceding (L3) headers down to remove ESP header and IV.
> > >>> + */
> > >> why cant we use rte_mbuf APIs to append/prepend/trim/adjust lengths.
> > > We do use rte_mbuf append/trim, etc. adjust mbuf's data_ofs and data_len.
> > > But apart from that for transport mode we have to move actual packet headers.
> > > Let say for inbound we have to get rid of ESP header (which is after IP header),
> > > but preserve IP header, so we moving L2/L3 headers down, overwriting ESP header.
> > ok got your point
> > >> I believe these adjustments are happening in the mbuf itself.
> > >> Moreover these APIs are not specific to esp headers.
> > > I didn't get your last sentence: that function is used to remove esp header
> > > (see above) - that's why I named it that way.
> > These can be used to remove any header and not specifically esp. So this
> > API could be generic in rte_mbuf.
> 
> That function has nothing to do with mbuf in general.
> It just copies bytes between overlapping in certain way buffers
> (src.start < dst.start < src.end < dst.end).
> Right now it is very primitive - copies on byte at a time in
> descending order.
> Wrote it just to avoid using memmove(). 
> I don't think there is any point to have such dummy function in the lib/eal.
> 
> > >
> > >>> +static inline void
> > >>> +remove_esph(char *np, char *op, uint32_t hlen)
> > >>> +{
> > >>> +	uint32_t i;
> > >>> +
> > >>> +	for (i = hlen; i-- != 0; np[i] = op[i])
> > >>> +		;
> > >>> +}
> > >>> +
> > >>> +/*
> > >
> > >>> +
> > >>> +/* update original and new ip header fields for tunnel case */
> > >>> +static inline void
> > >>> +update_tun_l3hdr(const struct rte_ipsec_sa *sa, void *p, uint32_t plen,
> > >>> +		uint32_t l2len, rte_be16_t pid)
> > >>> +{
> > >>> +	struct ipv4_hdr *v4h;
> > >>> +	struct ipv6_hdr *v6h;
> > >>> +
> > >>> +	if (sa->type & RTE_IPSEC_SATP_MODE_TUNLV4) {
> > >>> +		v4h = p;
> > >>> +		v4h->packet_id = pid;
> > >>> +		v4h->total_length = rte_cpu_to_be_16(plen - l2len);
> > >> where are we updating the rest of the fields, like ttl, checksum, ip
> > >> addresses, etc
> > > TTL, ip addresses and other fileds supposed to be setuped by user
> > > and provided via rte_ipsec_sa_init():
> > > struct rte_ipsec_sa_prm.tun.hdr  should contain prepared template
> > > for L3(and L2 if user wants to) header.
> > > Checksum calculation is not done inside the lib right now -
> > > it is a user responsibility to caclucate/set it after librte_ipsec
> > > finishes processing the packet.
> > I believe static fields are updated during sa init but some fields like
> > ttl and checksum,
> > can be updated in the library itself which is updated for every packet.
> > (https://tools.ietf.org/html/rfc1624)
> 
> About checksum - there is no point to calculate cksum it in the lib,
> as user may choose to use HW chksum offload.
> All other libraries ip_frag, GSO, etc. leave it to the user,
> I don't see why ipsec should be different here.
> About TTL and other fields - I suppose you refer to:
> https://tools.ietf.org/html/rfc4301#section-5.1.2
> Header Construction for Tunnel Mode
> right?
> Surely that has to be supported, one way or the other,
> but we don't plan to implement it in 19.02.
> Current plan to add it in 19.05, if time permits.
> 
> > >
> > >>> +	} else {
> > >>> +		v6h = p;
> > >>> +		v6h->payload_len = rte_cpu_to_be_16(plen - l2len -
> > >>> +				sizeof(*v6h));
> > >>> +	}
> > >>> +}
> > >>> +
> > >>> +#endif /* _IPH_H_ */
> > >>> diff --git a/lib/librte_ipsec/ipsec_sqn.h b/lib/librte_ipsec/ipsec_sqn.h
> > >>> index 1935f6e30..6e18c34eb 100644
> > >>> --- a/lib/librte_ipsec/ipsec_sqn.h
> > >>> +++ b/lib/librte_ipsec/ipsec_sqn.h
> > >>> @@ -15,6 +15,45 @@
> > >>>
> > >>>    #define IS_ESN(sa)	((sa)->sqn_mask == UINT64_MAX)
> > >>>
> > >>> +/*
> > >>> + * gets SQN.hi32 bits, SQN supposed to be in network byte order.
> > >>> + */
> > >>> +static inline rte_be32_t
> > >>> +sqn_hi32(rte_be64_t sqn)
> > >>> +{
> > >>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > >>> +	return (sqn >> 32);
> > >>> +#else
> > >>> +	return sqn;
> > >>> +#endif
> > >>> +}
> > >>> +
> > >>> +/*
> > >>> + * gets SQN.low32 bits, SQN supposed to be in network byte order.
> > >>> + */
> > >>> +static inline rte_be32_t
> > >>> +sqn_low32(rte_be64_t sqn)
> > >>> +{
> > >>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > >>> +	return sqn;
> > >>> +#else
> > >>> +	return (sqn >> 32);
> > >>> +#endif
> > >>> +}
> > >>> +
> > >>> +/*
> > >>> + * gets SQN.low16 bits, SQN supposed to be in network byte order.
> > >>> + */
> > >>> +static inline rte_be16_t
> > >>> +sqn_low16(rte_be64_t sqn)
> > >>> +{
> > >>> +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > >>> +	return sqn;
> > >>> +#else
> > >>> +	return (sqn >> 48);
> > >>> +#endif
> > >>> +}
> > >>> +
> > >> shouldn't we move these seq number APIs in rte_esp.h and make them generic
> > > It could be done, but who will use them except librte_ipsec?
> > Whoever uses rte_esp.h and not use ipsec lib. The intent of rte_esp.h is
> > just for that only, otherwise we don't need rte_esp.h, we can have the
> > content of rte_esp.h in ipsec itself.
> 
> Again these functions are used just inside the lib to help avoid
> extra byteswapping during crypto-data/packet header constructions.
> I don't see how they will be useful in general. 
> Sure, if there will be demand from users in future - we can move them,
> but right now I don't think that would happen. 

I am not an expert of IPsec, but in general it is better to offer modular
code, so we can use very basic code and allow implementing an alternative
for higher level.
That's why I would be in favor to keep protocol definitions and checksum
in rte_net, as it is done for TCP.
About how much modular we want to be, it is a difficult question,
matter of tradeoff.