[PATCH] ipsec: fix NAT-T length calculation
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
---
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
Re: [EXT] [PATCH] ipsec: fix NAT-T length calculation
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
Re: [EXT] [PATCH] ipsec: fix NAT-T length calculation
|<- 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 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
> >>> ---
> >>> 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
Re: [EXT] [PATCH] ipsec: fix NAT-T length calculation
> 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?
Re: [EXT] [PATCH] ipsec: fix NAT-T length calculation
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 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?
[PATCH v2] ipsec: fix NAT-T header length calculation
UDP header and L2 header (if any) length is included in sa->hdr_len.
Take care of that in L3 header and pakcet length caculation.
Fixes: 01eef5907fc3 ("ipsec: support NAT-T")
Cc: sta...@dpdk.org
Signed-off-by: Xiao Liang
Acked-by: Konstantin Ananyev
Acked-by: Radu Nicolau
---
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.41.0
[PATCH v2] ipsec: fix NAT-T header length calculation
UDP header and L2 header (if any) length is included in sa->hdr_len.
Take care of that in L3 header and pakcet length calculation.
Fixes: 01eef5907fc3 ("ipsec: support NAT-T")
Cc: sta...@dpdk.org
Signed-off-by: Xiao Liang
Acked-by: Konstantin Ananyev
Acked-by: Radu Nicolau
---
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.41.0
[dpdk-dev] [PATCH] kni:fix build failure as "ndo_change_mtu_rh74" not found in RHEL8
'ndo_change_mtu_rh74' was changed to 'ndo_change_mtu' in RHEL8. Build error log: /home/dpdk-18.11/kernel/linux/kni/compat.h:107:24: error: ‘const struct net_device_ops’ has no member named ‘ndo_change_mtu_rh74’; did you mean ‘ndo_change_mtu’? #define ndo_change_mtu ndo_change_mtu_rh74 ^~~ Signed-off-by: Xiao Liang --- kernel/linux/kni/compat.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/linux/kni/compat.h b/kernel/linux/kni/compat.h index 5aadebbcd..bc81d0c8d 100644 --- a/kernel/linux/kni/compat.h +++ b/kernel/linux/kni/compat.h @@ -103,7 +103,8 @@ #endif #if (defined(RHEL_RELEASE_CODE) && \ - (RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7, 5))) + (RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7, 5)) && \ + (RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8, 0))) #define ndo_change_mtu ndo_change_mtu_rh74 #endif -- 2.17.2
[dpdk-dev] [PATCH] kni:fix build failure as "ndo_change_mtu_rh74" not found in RHEL8
'ndo_change_mtu_rh74' was changed to 'ndo_change_mtu' in RHEL8. Build error log: /home/dpdk-18.11/kernel/linux/kni/compat.h:107:24: error: ‘const struct net_device_ops’ has no member named ‘ndo_change_mtu_rh74’; did you mean ‘ndo_change_mtu’? #define ndo_change_mtu ndo_change_mtu_rh74 ^~~ Signed-off-by: Xiao Liang --- kernel/linux/kni/compat.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/linux/kni/compat.h b/kernel/linux/kni/compat.h index 5aadebbcd..bc81d0c8d 100644 --- a/kernel/linux/kni/compat.h +++ b/kernel/linux/kni/compat.h @@ -103,7 +103,8 @@ #endif #if (defined(RHEL_RELEASE_CODE) && \ - (RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7, 5))) + (RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7, 5)) && \ + (RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8, 0))) #define ndo_change_mtu ndo_change_mtu_rh74 #endif -- 2.17.2
[dpdk-dev] [PATCH v2] kni:fix build failure as "ndo_change_mtu_rh74" not found in RHEL8
'ndo_change_mtu_rh74' was changed to 'ndo_change_mtu' in RHEL8. Build error log: /home/dpdk-18.11/kernel/linux/kni/compat.h:107:24: error: ‘const struct net_device_ops’ has no member named ‘ndo_change_mtu_rh74’; did you mean ‘ndo_change_mtu’? #define ndo_change_mtu ndo_change_mtu_rh74 ^~~ Signed-off-by: Xiao Liang --- kernel/linux/kni/compat.h | 3 ++- kernel/linux/kni/ethtool/igb/kcompat.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/kernel/linux/kni/compat.h b/kernel/linux/kni/compat.h index 5aadebbcd..bc81d0c8d 100644 --- a/kernel/linux/kni/compat.h +++ b/kernel/linux/kni/compat.h @@ -103,7 +103,8 @@ #endif #if (defined(RHEL_RELEASE_CODE) && \ - (RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7, 5))) + (RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7, 5)) && \ + (RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8, 0))) #define ndo_change_mtu ndo_change_mtu_rh74 #endif diff --git a/kernel/linux/kni/ethtool/igb/kcompat.h b/kernel/linux/kni/ethtool/igb/kcompat.h index ae1b53093..2681be684 100644 --- a/kernel/linux/kni/ethtool/igb/kcompat.h +++ b/kernel/linux/kni/ethtool/igb/kcompat.h @@ -3930,7 +3930,8 @@ skb_set_hash(struct sk_buff *skb, __u32 hash, __always_unused int type) #endif #if (defined(RHEL_RELEASE_CODE) && \ - (RHEL_RELEASE_VERSION(7, 5) <= RHEL_RELEASE_CODE)) + (RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7, 5)) && \ + (RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8, 0))) #define ndo_change_mtu ndo_change_mtu_rh74 #endif -- 2.17.2
[dpdk-dev] [PATCH v2] kni:fix build failure as "ndo_change_mtu_rh74" not found in RHEL8
'ndo_change_mtu_rh74' was changed to 'ndo_change_mtu' in RHEL8. Build error log: /home/dpdk-18.11/kernel/linux/kni/compat.h:107:24: error: ‘const struct net_device_ops’ has no member named ‘ndo_change_mtu_rh74’; did you mean ‘ndo_change_mtu’? #define ndo_change_mtu ndo_change_mtu_rh74 ^~~ Signed-off-by: Xiao Liang --- kernel/linux/kni/compat.h | 3 ++- kernel/linux/kni/ethtool/igb/kcompat.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/kernel/linux/kni/compat.h b/kernel/linux/kni/compat.h index 5aadebbcd..bc81d0c8d 100644 --- a/kernel/linux/kni/compat.h +++ b/kernel/linux/kni/compat.h @@ -103,7 +103,8 @@ #endif #if (defined(RHEL_RELEASE_CODE) && \ - (RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7, 5))) + (RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7, 5)) && \ + (RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8, 0))) #define ndo_change_mtu ndo_change_mtu_rh74 #endif diff --git a/kernel/linux/kni/ethtool/igb/kcompat.h b/kernel/linux/kni/ethtool/igb/kcompat.h index ae1b53093..2681be684 100644 --- a/kernel/linux/kni/ethtool/igb/kcompat.h +++ b/kernel/linux/kni/ethtool/igb/kcompat.h @@ -3930,7 +3930,8 @@ skb_set_hash(struct sk_buff *skb, __u32 hash, __always_unused int type) #endif #if (defined(RHEL_RELEASE_CODE) && \ - (RHEL_RELEASE_VERSION(7, 5) <= RHEL_RELEASE_CODE)) + (RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7, 5)) && \ + (RHEL_RELEASE_CODE < RHEL_RELEASE_VERSION(8, 0))) #define ndo_change_mtu ndo_change_mtu_rh74 #endif -- 2.17.2
