Re: [EXT] [PATCH] ipsec: fix NAT-T length calculation

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

On 07-Jul-23 4:12 AM, Xiao Liang wrote:
> |<-           mb->pkt_len - sqh_len                   ->|
> |<-            sa->hdr_len            ->|
> |<- sa->hdr_l3_off ->|            |<- udph->dgram_len ->|
>
> +--------------------+------------+-----+-----+---------+-----+
> |         ETH        |     IP     | UDP | ESP | payload | sqh |
> +--------------------+------------+-----+-----+---------+-----+
>
> |<- sa->hdr_l3_off ->|<- l3_len ->|
> |<-      prm->tun.hdr_len       ->|
> |<-            sa->hdr_len            ->|

sa->hdr_len and prm->tun.hdr_len don't include L2 length so both should 
start in the diagram at the end of the ETH header.

So the right way to compute datagram length is

dgram_len = mb->pkt_len - sqh_len - sa->hdr_l3_off - sa->hdr_len + 
sizeof(struct rte_udp_hdr)


>
> The figure above shows how
>      udph->dgram_len = mb->pkt_len - sqh_len - sa->hdr_len +
> sizeof(struct rte_udp_hdr);
> and
>      l3_len = prm->tun.hdr_len - sa->hdr_l3_off;
>
> Correct me if anything wrong.
>
> Thanks,
> Xiao Liang
>
>
>
>
> On Thu, Jul 6, 2023 at 6:20 PM Radu Nicolau <radu.nicolau@intel.com> wrote:
>>
>> On 06-Jul-23 10:08 AM, Konstantin Ananyev wrote:
>>> Hi Akhil,
>>>
>>>> Hi Konstantin,
>>>> Can you review this patch?
>>>>
>>>>> UDP header length is included in sa->hdr_len. Take care of that in
>>>>> L3 header and pakcet length calculation.
>>>>>
>>>>> Fixes: 01eef5907fc3 ("ipsec: support NAT-T")
>>>>>
>>>>> Signed-off-by: Xiao Liang <shaw.leon@gmail.com>
>>>>> ---
>>>>>    lib/ipsec/esp_outb.c | 2 +-
>>>>>    lib/ipsec/sa.c       | 2 +-
>>>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/lib/ipsec/esp_outb.c b/lib/ipsec/esp_outb.c
>>>>> index 9cbd9202f6..ec87b1dce2 100644
>>>>> --- a/lib/ipsec/esp_outb.c
>>>>> +++ b/lib/ipsec/esp_outb.c
>>>>> @@ -198,7 +198,7 @@ outb_tun_pkt_prepare(struct rte_ipsec_sa *sa,
>>>>> rte_be64_t sqc,
>>>>>              struct rte_udp_hdr *udph = (struct rte_udp_hdr *)
>>>>>                      (ph + sa->hdr_len - sizeof(struct rte_udp_hdr));
>>>>>              udph->dgram_len = rte_cpu_to_be_16(mb->pkt_len - sqh_len -
>>>>> -                           sa->hdr_l3_off - sa->hdr_len);
>>>>> +                           sa->hdr_len + sizeof(struct rte_udp_hdr));
>>> To be honest, it is not clear to me why we shouldn't take into account sa->hdr_l3_off
>>>    any more.
>>> Probably the author can explain.
>>> Also would like author of  NAT-T support to chime in.
>>> Radu, any comments on that patch?
>> I agree, hdr_l3_off should not be ignored. Also sa->hdr_len already
>> includes the size of UDP header, see line 366 in esp_outb_tun_init in
>> sa.c (or the line above this change, where the udph pointer is computed
>> assuming this)
>>> Thanks
>>> Konstantin
>>>
>>>>>      }
>>>>>
>>>>>      /* update original and new ip header fields */
>>>>> diff --git a/lib/ipsec/sa.c b/lib/ipsec/sa.c
>>>>> index 59a547637d..2297bd6d72 100644
>>>>> --- a/lib/ipsec/sa.c
>>>>> +++ b/lib/ipsec/sa.c
>>>>> @@ -371,7 +371,7 @@ esp_outb_tun_init(struct rte_ipsec_sa *sa, const struct
>>>>> rte_ipsec_sa_prm *prm)
>>>>>
>>>>>      /* update l2_len and l3_len fields for outbound mbuf */
>>>>>      sa->tx_offload.val = rte_mbuf_tx_offload(sa->hdr_l3_off,
>>>>> -           sa->hdr_len - sa->hdr_l3_off, 0, 0, 0, 0, 0);
>>>>> +           prm->tun.hdr_len - sa->hdr_l3_off, 0, 0, 0, 0, 0);
>>>>>
>>>>>      esp_outb_init(sa, sa->hdr_len, prm->ipsec_xform.esn.value);
>>>>>    }
>>>>> --
>>>>> 2.40.0