Re: [dpdk-dev] [PATCH v6 07/10] ipsec: add support for NAT-T

["Nicolau, Radu" <radu.nicolau@intel.com>]

On 9/23/2021 5:43 PM, Ananyev, Konstantin wrote:
>
>> Add support for the IPsec NAT-Traversal use case for Tunnel mode
>> packets.
>>
>> 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/iph.h          | 17 +++++++++++++++++
>>   lib/ipsec/rte_ipsec_sa.h |  8 +++++++-
>>   lib/ipsec/sa.c           | 13 ++++++++++++-
>>   lib/ipsec/sa.h           |  4 ++++
>>   4 files changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/ipsec/iph.h b/lib/ipsec/iph.h
>> index 2d223199ac..c5c213a2b4 100644
>> --- a/lib/ipsec/iph.h
>> +++ b/lib/ipsec/iph.h
>> @@ -251,6 +251,7 @@ update_tun_outb_l3hdr(const struct rte_ipsec_sa *sa, void *outh,
>>   {
>>   	struct rte_ipv4_hdr *v4h;
>>   	struct rte_ipv6_hdr *v6h;
>> +	struct rte_udp_hdr *udph;
>>   	uint8_t is_outh_ipv4;
>>
>>   	if (sa->type & RTE_IPSEC_SATP_MODE_TUNLV4) {
>> @@ -258,11 +259,27 @@ update_tun_outb_l3hdr(const struct rte_ipsec_sa *sa, void *outh,
>>   		v4h = outh;
>>   		v4h->packet_id = pid;
>>   		v4h->total_length = rte_cpu_to_be_16(plen - l2len);
>> +
>> +		if (sa->type & RTE_IPSEC_SATP_NATT_ENABLE) {
>> +			udph = (struct rte_udp_hdr *)(v4h + 1);
>> +			udph->dst_port = sa->natt.dport;
>> +			udph->src_port = sa->natt.sport;
>> +			udph->dgram_len = rte_cpu_to_be_16(plen - l2len -
>> +				(sizeof(*v4h) + sizeof(*udph)));
>> +		}
>>   	} else {
>>   		is_outh_ipv4 = 0;
>>   		v6h = outh;
>>   		v6h->payload_len = rte_cpu_to_be_16(plen - l2len -
>>   				sizeof(*v6h));
>> +
>> +		if (sa->type & RTE_IPSEC_SATP_NATT_ENABLE) {
>> +			udph = (struct rte_udp_hdr *)(v6h + 1);
> Why you presume there would be always ipv6 with no options?
> Shouldn't we use hdr_l3_len provided by user?

Yes, I will use hdr_l3_len.

> Another thing - I am not sure we need 'natt' field in rte_ipsec_sa at all.
> UDP header (sport, dport) is consitant and could be part of header template
> provided by user at sa initialization time.

The rte_security_ipsec_sa_options::udp_encap flag assumes that the UDP 
encapsulation i.e. adding the header is not the responsibility of the 
user, so we can append it (transparently to the user) to the header 
template but the user should not do it. Will this work?


>
>> +			udph->dst_port = sa->natt.dport;
>> +			udph->src_port = sa->natt.sport;
>> +			udph->dgram_len = rte_cpu_to_be_16(plen - l2len -
>> +				(sizeof(*v6h) + sizeof(*udph)));
> Whose responsibility will be to update cksum field?
According to the RFC it should be zero and the rx side must not 
check/use it. I will set it as zero