diff mbox series

ipsec: fix NAT-T length calculation

Message ID 20230418084613.52740-1-shaw.leon@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series ipsec: fix NAT-T length calculation |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-unit-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/intel-Functional success Functional PASS

Commit Message

Xiao Liang April 18, 2023, 8:46 a.m. UTC 
  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(-)

Comments

Akhil Goyal July 5, 2023, 1:49 p.m. UTC | #1 
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));
>  	}
> 
>  	/* 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
Konstantin Ananyev July 6, 2023, 9:08 a.m. UTC | #2 
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?
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
Radu Nicolau July 6, 2023, 10:20 a.m. UTC | #3 
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
Xiao Liang July 7, 2023, 2:06 a.m. UTC | #4 
Well, let me explain.

> >>> 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)

A few lines above, there is:

    ph = rte_pktmbuf_prepend(mb, hlen - l2len);

The L2 header is already pulled from the packet, then the packet
length has nothing to do with hdr_l3_off from this point, so use encap
header length instead.
You are right sa->hdr_len already includes the size of UDP header, and
that's why it should be added back here.

> >>>     }
> >>>
> >>>     /* 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);
> >>>   }

Again sa->hdr_len includes UDP header, which is not part of L3, so use
the original prm->tun.hdr_len.

Thanks,
Xiao Liang
Xiao Liang July 7, 2023, 3:12 a.m. UTC | #5 
|<-           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            ->|

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
Radu Nicolau July 7, 2023, 8:59 a.m. UTC | #6 
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
Xiao Liang July 7, 2023, 12:51 p.m. UTC | #7 
> 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)
>

|<-           mb->pkt_len - sqh_len                   ->|
|<- sa->hdr_l3_off ->|<- sa->hdr_len ->|
                                  |<- udph->dgram_len ->|

+--------------------+------------+-----+-----+---------+-----+
|         ETH        |     IP     | UDP | ESP | payload | sqh |
+--------------------+------------+-----+-----+---------+-----+

|<- sa->hdr_l3_off ->|<- l3_len ->|
                     |<- sa->hdr_len  ->|

If hdr_len doesn't include L2 length, I would agree that

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

But then what's the point of
    sa->hdr_len - sa->hdr_l3_off
in lib/ipsec/sa.c?
Xiao Liang July 7, 2023, 1:17 p.m. UTC | #8 
The context

    hlen = sa->hdr_len + sa->iv_len + sizeof(*esph);
    ...
    ph = rte_pktmbuf_prepend(mb, hlen - l2len);
    ...
    update_tun_outb_l3hdr(sa, ph + sa->hdr_l3_off, ph + hlen,
        mb->pkt_len - sqh_len, sa->hdr_l3_off, sqn_low16(sqc));

assumes L2 header length included in sa->hdr_len.

Even if hdr_len doesn't include L2, then mb->pkt_len won't either, so
UDP datagram length should still be
    mb->pkt_len - sqh_len - sa->hdr_len + sizeof(struct rte_udp_hdr);

On Fri, Jul 7, 2023 at 8:51 PM Xiao Liang <shaw.leon@gmail.com> wrote:
>
> > 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)
> >
>
> |<-           mb->pkt_len - sqh_len                   ->|
> |<- sa->hdr_l3_off ->|<- sa->hdr_len ->|
>                                   |<- udph->dgram_len ->|
>
> +--------------------+------------+-----+-----+---------+-----+
> |         ETH        |     IP     | UDP | ESP | payload | sqh |
> +--------------------+------------+-----+-----+---------+-----+
>
> |<- sa->hdr_l3_off ->|<- l3_len ->|
>                      |<- sa->hdr_len  ->|
>
> If hdr_len doesn't include L2 length, I would agree that
>
>     dgram_len = mb->pkt_len - sqh_len - sa->hdr_l3_off - sa->hdr_len +
> sizeof(struct rte_udp_hdr)
>
> But then what's the point of
>     sa->hdr_len - sa->hdr_l3_off
> in lib/ipsec/sa.c?
Radu Nicolau July 7, 2023, 1:26 p.m. UTC | #9 
On 07-Jul-23 1:51 PM, Xiao Liang wrote:
>> 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)
>>
> |<-           mb->pkt_len - sqh_len                   ->|
> |<- sa->hdr_l3_off ->|<- sa->hdr_len ->|
>                                    |<- udph->dgram_len ->|
>
> +--------------------+------------+-----+-----+---------+-----+
> |         ETH        |     IP     | UDP | ESP | payload | sqh |
> +--------------------+------------+-----+-----+---------+-----+
>
> |<- sa->hdr_l3_off ->|<- l3_len ->|
>                       |<- sa->hdr_len  ->|
>
> If hdr_len doesn't include L2 length, I would agree that
>
>      dgram_len = mb->pkt_len - sqh_len - sa->hdr_l3_off - sa->hdr_len +
> sizeof(struct rte_udp_hdr)
>
> But then what's the point of
>      sa->hdr_len - sa->hdr_l3_off
> in lib/ipsec/sa.c?

I will defer to Konstantin for a definite answer, that is if sa->hdr_len 
is supposed to include l2 length / offset or not. If it does, then the 
change that triggered this discussion is correct and we don't need to 
account for hdr_l3_off there.
Konstantin Ananyev July 10, 2023, 9:20 a.m. UTC | #10 
18/04/2023 09:46, Xiao Liang пишет:
> 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));
>   	}
>   
>   	/* 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);
>   }

Acked-by: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>


Thanks for explanation and for the fix.
One thing that still bothers me with UDP encap support:
we still don't have a test-case for it in examples/ipsec-secgw/test.
Konstantin Ananyev July 10, 2023, 9:24 a.m. UTC | #11 
07/07/2023 14:26, Radu Nicolau пишет:
> 
> On 07-Jul-23 1:51 PM, Xiao Liang wrote:
>>> 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)
>>>
>> |<-           mb->pkt_len - sqh_len                   ->|
>> |<- sa->hdr_l3_off ->|<- sa->hdr_len ->|
>>                                    |<- udph->dgram_len ->|
>>
>> +--------------------+------------+-----+-----+---------+-----+
>> |         ETH        |     IP     | UDP | ESP | payload | sqh |
>> +--------------------+------------+-----+-----+---------+-----+
>>
>> |<- sa->hdr_l3_off ->|<- l3_len ->|
>>                       |<- sa->hdr_len  ->|
>>
>> If hdr_len doesn't include L2 length, I would agree that
>>
>>      dgram_len = mb->pkt_len - sqh_len - sa->hdr_l3_off - sa->hdr_len +
>> sizeof(struct rte_udp_hdr)
>>
>> But then what's the point of
>>      sa->hdr_len - sa->hdr_l3_off
>> in lib/ipsec/sa.c?
> 
> I will defer to Konstantin for a definite answer, that is if sa->hdr_len 
> is supposed to include l2 length / offset or not. If it does, then the 
> change that triggered this discussion is correct and we don't need to 
> account for hdr_l3_off there.
> 


Ok, have to revive my memories here.
So, actually hdr_len stands for all tunell headers users want to add.
It consits of optional l2_len (could be zero) plus outer ip len, plus
in that case udp hdr len.
hdr_l3_off is an offset withih hdr_len where outer ip header starts.
So yep, initial patch looks ok to me, I just acked it.
Sorry for being a bit sloppy at reviewing it.
Radu Nicolau July 10, 2023, 9:38 a.m. UTC | #12 
On 10-Jul-23 10:24 AM, Konstantin Ananyev wrote:
> 07/07/2023 14:26, Radu Nicolau пишет:
>>
>> On 07-Jul-23 1:51 PM, Xiao Liang wrote:
>>>> 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)
>>>>
>>> |<-           mb->pkt_len - sqh_len ->|
>>> |<- sa->hdr_l3_off ->|<- sa->hdr_len ->|
>>>                                    |<- udph->dgram_len ->|
>>>
>>> +--------------------+------------+-----+-----+---------+-----+
>>> |         ETH        |     IP     | UDP | ESP | payload | sqh |
>>> +--------------------+------------+-----+-----+---------+-----+
>>>
>>> |<- sa->hdr_l3_off ->|<- l3_len ->|
>>>                       |<- sa->hdr_len  ->|
>>>
>>> If hdr_len doesn't include L2 length, I would agree that
>>>
>>>      dgram_len = mb->pkt_len - sqh_len - sa->hdr_l3_off - sa->hdr_len +
>>> sizeof(struct rte_udp_hdr)
>>>
>>> But then what's the point of
>>>      sa->hdr_len - sa->hdr_l3_off
>>> in lib/ipsec/sa.c?
>>
>> I will defer to Konstantin for a definite answer, that is if 
>> sa->hdr_len is supposed to include l2 length / offset or not. If it 
>> does, then the change that triggered this discussion is correct and 
>> we don't need to account for hdr_l3_off there.
>>
>
>
> Ok, have to revive my memories here.
> So, actually hdr_len stands for all tunell headers users want to add.
> It consits of optional l2_len (could be zero) plus outer ip len, plus
> in that case udp hdr len.
> hdr_l3_off is an offset withih hdr_len where outer ip header starts.
> So yep, initial patch looks ok to me, I just acked it.
> Sorry for being a bit sloppy at reviewing it.


This fix should also be CC'ed to stable@dpdk.org

Acked-by: Radu Nicolau <radu.nicolau@intel.com>
diff mbox series

Patch

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));
 	}
 
 	/* 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);
 }