DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH v1] gro: bug fix in identifying 0 length tcp packets
@ 2022-04-03 11:50 Kumara Parameshwaran
  2022-04-04  6:22 ` Morten Brørup
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Kumara Parameshwaran @ 2022-04-03 11:50 UTC (permalink / raw)
  To: jiayu.hu; +Cc: dev, Kumara Parameshwaran, stable, Kumara Parameshwaran

As the minimum Ethernet frame size is 64 bytes, a 0 length
tcp payload without tcp options would be 54 bytes and hence
there would be padding. So it would be incorrect to use the
packet length to determine the tcp data length.

Fixes: 1e4cf4d6d4fb ("gro: cleanup")
Cc: stable@dpdk.org

Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
v1:
	Do not use packet length to determine the tcp data length as 
	the packet length could have padded bytes. This would lead 
	to addition of 0 length tcp packets into the GRO layer when 
	there ethernet fram is padded.
 lib/gro/gro_tcp4.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c
index 7498c66..45e3f48 100644
--- a/lib/gro/gro_tcp4.c
+++ b/lib/gro/gro_tcp4.c
@@ -198,7 +198,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	struct rte_tcp_hdr *tcp_hdr;
 	uint32_t sent_seq;
 	int32_t tcp_dl;
-	uint16_t ip_id, hdr_len, frag_off;
+	uint16_t ip_id, frag_off;
 	uint8_t is_atomic;
 
 	struct tcp4_flow_key key;
@@ -217,7 +217,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	eth_hdr = rte_pktmbuf_mtod(pkt, struct rte_ether_hdr *);
 	ipv4_hdr = (struct rte_ipv4_hdr *)((char *)eth_hdr + pkt->l2_len);
 	tcp_hdr = (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
-	hdr_len = pkt->l2_len + pkt->l3_len + pkt->l4_len;
 
 	/*
 	 * Don't process the packet which has FIN, SYN, RST, PSH, URG, ECE
@@ -229,7 +228,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	 * Don't process the packet whose payload length is less than or
 	 * equal to 0.
 	 */
-	tcp_dl = pkt->pkt_len - hdr_len;
+	tcp_dl = rte_be_to_cpu_16(ipv4_hdr->total_length) - (pkt->l3_len + pkt->l4_len);
 	if (tcp_dl <= 0)
 		return -1;
 
-- 
2.7.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* RE: [PATCH v1] gro: bug fix in identifying 0 length tcp packets
  2022-04-03 11:50 [PATCH v1] gro: bug fix in identifying 0 length tcp packets Kumara Parameshwaran
@ 2022-04-04  6:22 ` Morten Brørup
  2022-04-22 10:32 ` [PATCH v2] " Kumara Parameshwaran
       [not found] ` <CANxNyatRmEU8mMag=Z=dg-200pBTmfBdP-ML4VEwYmG6X73EpA@mail.gmail.com>
  2 siblings, 0 replies; 4+ messages in thread
From: Morten Brørup @ 2022-04-04  6:22 UTC (permalink / raw)
  To: Kumara Parameshwaran, jiayu.hu; +Cc: dev, stable, Kumara Parameshwaran

> From: Kumara Parameshwaran [mailto:kumaraparamesh92@gmail.com]
> Sent: Sunday, 3 April 2022 13.51
> 
> As the minimum Ethernet frame size is 64 bytes, a 0 length
> tcp payload without tcp options would be 54 bytes and hence
> there would be padding. So it would be incorrect to use the
> packet length to determine the tcp data length.
> 
> Fixes: 1e4cf4d6d4fb ("gro: cleanup")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
> v1:
> 	Do not use packet length to determine the tcp data length as
> 	the packet length could have padded bytes. This would lead
> 	to addition of 0 length tcp packets into the GRO layer when
> 	there ethernet fram is padded.
>  lib/gro/gro_tcp4.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c
> index 7498c66..45e3f48 100644
> --- a/lib/gro/gro_tcp4.c
> +++ b/lib/gro/gro_tcp4.c
> @@ -198,7 +198,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>  	struct rte_tcp_hdr *tcp_hdr;
>  	uint32_t sent_seq;
>  	int32_t tcp_dl;
> -	uint16_t ip_id, hdr_len, frag_off;
> +	uint16_t ip_id, frag_off;
>  	uint8_t is_atomic;
> 
>  	struct tcp4_flow_key key;
> @@ -217,7 +217,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>  	eth_hdr = rte_pktmbuf_mtod(pkt, struct rte_ether_hdr *);
>  	ipv4_hdr = (struct rte_ipv4_hdr *)((char *)eth_hdr + pkt-
> >l2_len);
>  	tcp_hdr = (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
> -	hdr_len = pkt->l2_len + pkt->l3_len + pkt->l4_len;
> 
>  	/*
>  	 * Don't process the packet which has FIN, SYN, RST, PSH, URG,
> ECE
> @@ -229,7 +228,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>  	 * Don't process the packet whose payload length is less than or
>  	 * equal to 0.
>  	 */
> -	tcp_dl = pkt->pkt_len - hdr_len;
> +	tcp_dl = rte_be_to_cpu_16(ipv4_hdr->total_length) - (pkt->l3_len
> + pkt->l4_len);
>  	if (tcp_dl <= 0)
>  		return -1;
> 
> --
> 2.7.4
> 

Please confirm that this does not introduce a buffer overrun regarding malformed packets, e.g. a small packet with ipv4_hdr->total_length set to 65000.

I haven't looked at the patch in context, so my concern may be irrelevant.

-Morten


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH v2] gro: bug fix in identifying 0 length tcp packets
  2022-04-03 11:50 [PATCH v1] gro: bug fix in identifying 0 length tcp packets Kumara Parameshwaran
  2022-04-04  6:22 ` Morten Brørup
@ 2022-04-22 10:32 ` Kumara Parameshwaran
       [not found] ` <CANxNyatRmEU8mMag=Z=dg-200pBTmfBdP-ML4VEwYmG6X73EpA@mail.gmail.com>
  2 siblings, 0 replies; 4+ messages in thread
From: Kumara Parameshwaran @ 2022-04-22 10:32 UTC (permalink / raw)
  To: jiayu.hu; +Cc: dev, Kumara Parameshwaran, stable, Kumara Parameshwaran

From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>

As the minimum Ethernet frame size is 64 bytes, a 0 length
tcp payload without tcp options would be 54 bytes and hence
there would be padding. So it would be incorrect to use the
packet length to determine the tcp data length.

Fixes: 1e4cf4d6d4fb ("gro: cleanup")
Cc: stable@dpdk.org

Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
---
v1:
	Do not use packet length to determine the tcp data length as 
	the packet length could have padded bytes. This would lead 
	to addition of 0 length tcp packets into the GRO layer when 
	there ethernet fram is padded.
v2:
	Since using ip packet length to determine the tcp data length, 
	validate the ip packet length

 lib/gro/gro_tcp4.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c
index 7498c66..30f5922 100644
--- a/lib/gro/gro_tcp4.c
+++ b/lib/gro/gro_tcp4.c
@@ -198,7 +198,8 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	struct rte_tcp_hdr *tcp_hdr;
 	uint32_t sent_seq;
 	int32_t tcp_dl;
-	uint16_t ip_id, hdr_len, frag_off;
+	uint16_t ip_id, frag_off;
+	uint16_t ip_len;
 	uint8_t is_atomic;
 
 	struct tcp4_flow_key key;
@@ -217,7 +218,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	eth_hdr = rte_pktmbuf_mtod(pkt, struct rte_ether_hdr *);
 	ipv4_hdr = (struct rte_ipv4_hdr *)((char *)eth_hdr + pkt->l2_len);
 	tcp_hdr = (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
-	hdr_len = pkt->l2_len + pkt->l3_len + pkt->l4_len;
 
 	/*
 	 * Don't process the packet which has FIN, SYN, RST, PSH, URG, ECE
@@ -229,8 +229,9 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	 * Don't process the packet whose payload length is less than or
 	 * equal to 0.
 	 */
-	tcp_dl = pkt->pkt_len - hdr_len;
-	if (tcp_dl <= 0)
+	ip_len = rte_be_to_cpu_16(ipv4_hdr->total_length);
+	tcp_dl = ip_len - (pkt->l3_len + pkt->l4_len);
+	if (tcp_dl <= 0 || ip_len > pkt->pkt_len)
 		return -1;
 
 	/*
-- 
2.7.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v1] gro: bug fix in identifying 0 length tcp packets
       [not found] ` <CANxNyatRmEU8mMag=Z=dg-200pBTmfBdP-ML4VEwYmG6X73EpA@mail.gmail.com>
@ 2022-07-18  5:06   ` kumaraparameshwaran rathinavel
  0 siblings, 0 replies; 4+ messages in thread
From: kumaraparameshwaran rathinavel @ 2022-07-18  5:06 UTC (permalink / raw)
  To: Hu, Jiayu; +Cc: Thomas Monjalon, mb, dev

[-- Attachment #1: Type: text/plain, Size: 2887 bytes --]

Hi Jiayu,
Can you please  suggest if the patch can be used ? When timestamps are
disabled we would unnecessarily indulge pure TCP ack packets in the GRO
layer. Or can we have a fix where if the TCP timestamp option is not
present in the packet, do not process the packet, return immediately ?

Thanks,
Param

On Mon, Apr 25, 2022 at 11:36 PM kumaraparameshwaran rathinavel <
kumaraparamesh92@gmail.com> wrote:

> Hi,
>
> I would like you to review this patch and let me know what you think of
> it.
>
> Thanks,
> Kumara.
>
> ---------- Forwarded message ---------
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> Date: Sun, Apr 3, 2022 at 5:20 PM
> Subject: [PATCH v1] gro: bug fix in identifying 0 length tcp packets
> To: <jiayu.hu@intel.com>
> Cc: <dev@dpdk.org>, Kumara Parameshwaran <kumaraparamesh92@gmail.com>, <
> stable@dpdk.org>, Kumara Parameshwaran <kparameshwar@vmware.com>
>
>
> As the minimum Ethernet frame size is 64 bytes, a 0 length
> tcp payload without tcp options would be 54 bytes and hence
> there would be padding. So it would be incorrect to use the
> packet length to determine the tcp data length.
>
> Fixes: 1e4cf4d6d4fb ("gro: cleanup")
> Cc: stable@dpdk.org
>
> Signed-off-by: Kumara Parameshwaran <kparameshwar@vmware.com>
> ---
> v1:
>         Do not use packet length to determine the tcp data length as
>         the packet length could have padded bytes. This would lead
>         to addition of 0 length tcp packets into the GRO layer when
>         there ethernet fram is padded.
>  lib/gro/gro_tcp4.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c
> index 7498c66..45e3f48 100644
> --- a/lib/gro/gro_tcp4.c
> +++ b/lib/gro/gro_tcp4.c
> @@ -198,7 +198,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>         struct rte_tcp_hdr *tcp_hdr;
>         uint32_t sent_seq;
>         int32_t tcp_dl;
> -       uint16_t ip_id, hdr_len, frag_off;
> +       uint16_t ip_id, frag_off;
>         uint8_t is_atomic;
>
>         struct tcp4_flow_key key;
> @@ -217,7 +217,6 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>         eth_hdr = rte_pktmbuf_mtod(pkt, struct rte_ether_hdr *);
>         ipv4_hdr = (struct rte_ipv4_hdr *)((char *)eth_hdr + pkt->l2_len);
>         tcp_hdr = (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
> -       hdr_len = pkt->l2_len + pkt->l3_len + pkt->l4_len;
>
>         /*
>          * Don't process the packet which has FIN, SYN, RST, PSH, URG, ECE
> @@ -229,7 +228,7 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>          * Don't process the packet whose payload length is less than or
>          * equal to 0.
>          */
> -       tcp_dl = pkt->pkt_len - hdr_len;
> +       tcp_dl = rte_be_to_cpu_16(ipv4_hdr->total_length) - (pkt->l3_len +
> pkt->l4_len);
>         if (tcp_dl <= 0)
>                 return -1;
>
> --
> 2.7.4
>
>

[-- Attachment #2: Type: text/html, Size: 4309 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-07-18  5:06 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-03 11:50 [PATCH v1] gro: bug fix in identifying 0 length tcp packets Kumara Parameshwaran
2022-04-04  6:22 ` Morten Brørup
2022-04-22 10:32 ` [PATCH v2] " Kumara Parameshwaran
     [not found] ` <CANxNyatRmEU8mMag=Z=dg-200pBTmfBdP-ML4VEwYmG6X73EpA@mail.gmail.com>
2022-07-18  5:06   ` [PATCH v1] " kumaraparameshwaran rathinavel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).