diff mbox series

gro: fix gro with ethernet tail padding bytes

Message ID 20220727080036.27263-1-jun.qiu@jaguarmicro.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series gro: fix gro with ethernet tail padding bytes |

Checks

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

Commit Message

Jun Qiu July 27, 2022, 8 a.m. UTC 
  Exclude CRC fields, the minimum Ethernet packet
length is 60 bytes. When the actual packet length
is less than 60 bytes, padding is added to the tail.
When GRO is performed on a packet containing a padding
field, mbuf->pkt_len is the one that contains the
padding field, which leads to the error of thinking
of the padding field as the actual content of the packet.
We need to trim away this extra padding field during
GRO processing.

Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
Cc: stable@dpdk.org

Signed-off-by: Jun Qiu <jun.qiu@jaguarmicro.com>
---
 lib/gro/gro_tcp4.c | 7 ++++++-
 lib/gro/gro_udp4.c | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Hu, Jiayu July 28, 2022, 1:55 a.m. UTC | #1 
Hi Jun,

> -----Original Message-----
> From: Jun Qiu <jun.qiu@jaguarmicro.com>
> Sent: Wednesday, July 27, 2022 4:01 PM
> To: dev@dpdk.org
> Cc: Hu, Jiayu <jiayu.hu@intel.com>; stable@dpdk.org
> Subject: [PATCH] gro: fix gro with ethernet tail padding bytes
> 
> Exclude CRC fields, the minimum Ethernet packet length is 60 bytes. When
> the actual packet length is less than 60 bytes, padding is added to the tail.
> When GRO is performed on a packet containing a padding field, mbuf-
> >pkt_len is the one that contains the padding field, which leads to the error
> of thinking of the padding field as the actual content of the packet.
> We need to trim away this extra padding field during GRO processing.
> 
> Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jun Qiu <jun.qiu@jaguarmicro.com>
> ---
>  lib/gro/gro_tcp4.c | 7 ++++++-
>  lib/gro/gro_udp4.c | 4 ++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c index
> 7849a2bd1d..0110db5748 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, hdr_len, frag_off, ip_tlen;
>  	uint8_t is_atomic;
> 
>  	struct tcp4_flow_key key;
> @@ -233,6 +233,11 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>  	if (tcp_dl <= 0)
>  		return -1;
> 
> +	/* trim the tail padding bytes */
> +	ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
> +	if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
> +		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);

Good catch.

But there is a case needed to consider:
if the packet with padding doesn't merge with any other packets, its
padding bytes are trimmed too. If upper layer functions check the minimal
packet length before check padding bytes in the packet, this change will
cause they treat this packet as an illegal one. How does Linux kernel GRO
handle padding packets?

Another choice is to trim the padding bytes when the packet merges with
another one. In this case, GRO will only trim padding bytes for reassembled
packets. For example, you can trim padding in merge_two_tcp4_packets().

Thanks,
Jiayu

> +
>  	/*
>  	 * Save IPv4 ID for the packet whose DF bit is 0. For the packet
>  	 * whose DF bit is 1, IPv4 ID is ignored.
> diff --git a/lib/gro/gro_udp4.c b/lib/gro/gro_udp4.c index
> dd71135ada..839f9748b7 100644
> --- a/lib/gro/gro_udp4.c
> +++ b/lib/gro/gro_udp4.c
> @@ -231,6 +231,10 @@ gro_udp4_reassemble(struct rte_mbuf *pkt,
>  	if (ip_dl <= pkt->l3_len)
>  		return -1;
> 
> +	/* trim the tail padding bytes */
> +	if (pkt->pkt_len > (uint32_t)(ip_dl + pkt->l2_len))
> +		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_dl - pkt->l2_len);
> +
>  	ip_dl -= pkt->l3_len;
>  	ip_id = rte_be_to_cpu_16(ipv4_hdr->packet_id);
>  	frag_offset = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
> --
> 2.25.1
Jun Qiu July 28, 2022, 11:02 a.m. UTC | #2 
I don't think the stack(software) cares if the length of a packet is less than 60, especially when receiving it.
In the linux kernel, if the packet length does not match the IP total-length, the packet is flushed to the stack instead of GRO. The previous GRO cache packets are also flushed into the stack.

If you trim padding in merge_two_tcp4_packets(), then both "pkt_head" and "pkt_tail" need to decide whether to trim or not, which can be a bit tricky to handle.


-----Original Message-----
From: Hu, Jiayu <jiayu.hu@intel.com> 
Sent: Thursday, July 28, 2022 9:56 AM
To: Jun Qiu <jun.qiu@jaguarmicro.com>; dev@dpdk.org
Cc: stable@dpdk.org
Subject: RE: [PATCH] gro: fix gro with ethernet tail padding bytes

Hi Jun,

> -----Original Message-----
> From: Jun Qiu <jun.qiu@jaguarmicro.com>
> Sent: Wednesday, July 27, 2022 4:01 PM
> To: dev@dpdk.org
> Cc: Hu, Jiayu <jiayu.hu@intel.com>; stable@dpdk.org
> Subject: [PATCH] gro: fix gro with ethernet tail padding bytes
> 
> Exclude CRC fields, the minimum Ethernet packet length is 60 bytes. 
> When the actual packet length is less than 60 bytes, padding is added to the tail.
> When GRO is performed on a packet containing a padding field, mbuf-
> >pkt_len is the one that contains the padding field, which leads to 
> >the error
> of thinking of the padding field as the actual content of the packet.
> We need to trim away this extra padding field during GRO processing.
> 
> Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jun Qiu <jun.qiu@jaguarmicro.com>
> ---
>  lib/gro/gro_tcp4.c | 7 ++++++-
>  lib/gro/gro_udp4.c | 4 ++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c index
> 7849a2bd1d..0110db5748 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, hdr_len, frag_off, ip_tlen;
>  	uint8_t is_atomic;
> 
>  	struct tcp4_flow_key key;
> @@ -233,6 +233,11 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
>  	if (tcp_dl <= 0)
>  		return -1;
> 
> +	/* trim the tail padding bytes */
> +	ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
> +	if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
> +		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);

Good catch.

But there is a case needed to consider:
if the packet with padding doesn't merge with any other packets, its padding bytes are trimmed too. If upper layer functions check the minimal packet length before check padding bytes in the packet, this change will cause they treat this packet as an illegal one. How does Linux kernel GRO handle padding packets?

Another choice is to trim the padding bytes when the packet merges with another one. In this case, GRO will only trim padding bytes for reassembled packets. For example, you can trim padding in merge_two_tcp4_packets().

Thanks,
Jiayu

> +
>  	/*
>  	 * Save IPv4 ID for the packet whose DF bit is 0. For the packet
>  	 * whose DF bit is 1, IPv4 ID is ignored.
> diff --git a/lib/gro/gro_udp4.c b/lib/gro/gro_udp4.c index
> dd71135ada..839f9748b7 100644
> --- a/lib/gro/gro_udp4.c
> +++ b/lib/gro/gro_udp4.c
> @@ -231,6 +231,10 @@ gro_udp4_reassemble(struct rte_mbuf *pkt,
>  	if (ip_dl <= pkt->l3_len)
>  		return -1;
> 
> +	/* trim the tail padding bytes */
> +	if (pkt->pkt_len > (uint32_t)(ip_dl + pkt->l2_len))
> +		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_dl - pkt->l2_len);
> +
>  	ip_dl -= pkt->l3_len;
>  	ip_id = rte_be_to_cpu_16(ipv4_hdr->packet_id);
>  	frag_offset = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
> --
> 2.25.1
Hu, Jiayu Aug. 15, 2022, 1:27 a.m. UTC | #3 
> -----Original Message-----
> From: Jun Qiu <jun.qiu@jaguarmicro.com>
> Sent: Thursday, July 28, 2022 7:02 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH] gro: fix gro with ethernet tail padding bytes
> 
> I don't think the stack(software) cares if the length of a packet is less than 60,
> especially when receiving it.
> In the linux kernel, if the packet length does not match the IP total-length,
> the packet is flushed to the stack instead of GRO. The previous GRO cache
> packets are also flushed into the stack.
> 
> If you trim padding in merge_two_tcp4_packets(), then both "pkt_head"
> and "pkt_tail" need to decide whether to trim or not, which can be a bit tricky
> to handle.

OK. This patch is OK for me.

Acked-by: Jiayu Hu <Jiayu.hu@intel.com>

Thanks,
Jiayu
> 
> 
> -----Original Message-----
> From: Hu, Jiayu <jiayu.hu@intel.com>
> Sent: Thursday, July 28, 2022 9:56 AM
> To: Jun Qiu <jun.qiu@jaguarmicro.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH] gro: fix gro with ethernet tail padding bytes
> 
> Hi Jun,
> 
> > -----Original Message-----
> > From: Jun Qiu <jun.qiu@jaguarmicro.com>
> > Sent: Wednesday, July 27, 2022 4:01 PM
> > To: dev@dpdk.org
> > Cc: Hu, Jiayu <jiayu.hu@intel.com>; stable@dpdk.org
> > Subject: [PATCH] gro: fix gro with ethernet tail padding bytes
> >
> > Exclude CRC fields, the minimum Ethernet packet length is 60 bytes.
> > When the actual packet length is less than 60 bytes, padding is added to the
> tail.
> > When GRO is performed on a packet containing a padding field, mbuf-
> > >pkt_len is the one that contains the padding field, which leads to
> > >the error
> > of thinking of the padding field as the actual content of the packet.
> > We need to trim away this extra padding field during GRO processing.
> >
> > Fixes: 0d2cbe59b719 ("lib/gro: support TCP/IPv4")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jun Qiu <jun.qiu@jaguarmicro.com>
> > ---
> >  lib/gro/gro_tcp4.c | 7 ++++++-
> >  lib/gro/gro_udp4.c | 4 ++++
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c index
> > 7849a2bd1d..0110db5748 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, hdr_len, frag_off, ip_tlen;
> >  	uint8_t is_atomic;
> >
> >  	struct tcp4_flow_key key;
> > @@ -233,6 +233,11 @@ gro_tcp4_reassemble(struct rte_mbuf *pkt,
> >  	if (tcp_dl <= 0)
> >  		return -1;
> >
> > +	/* trim the tail padding bytes */
> > +	ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
> > +	if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
> > +		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
> 
> Good catch.
> 
> But there is a case needed to consider:
> if the packet with padding doesn't merge with any other packets, its padding
> bytes are trimmed too. If upper layer functions check the minimal packet
> length before check padding bytes in the packet, this change will cause they
> treat this packet as an illegal one. How does Linux kernel GRO handle padding
> packets?
> 
> Another choice is to trim the padding bytes when the packet merges with
> another one. In this case, GRO will only trim padding bytes for reassembled
> packets. For example, you can trim padding in merge_two_tcp4_packets().
> 
> Thanks,
> Jiayu
> 
> > +
> >  	/*
> >  	 * Save IPv4 ID for the packet whose DF bit is 0. For the packet
> >  	 * whose DF bit is 1, IPv4 ID is ignored.
> > diff --git a/lib/gro/gro_udp4.c b/lib/gro/gro_udp4.c index
> > dd71135ada..839f9748b7 100644
> > --- a/lib/gro/gro_udp4.c
> > +++ b/lib/gro/gro_udp4.c
> > @@ -231,6 +231,10 @@ gro_udp4_reassemble(struct rte_mbuf *pkt,
> >  	if (ip_dl <= pkt->l3_len)
> >  		return -1;
> >
> > +	/* trim the tail padding bytes */
> > +	if (pkt->pkt_len > (uint32_t)(ip_dl + pkt->l2_len))
> > +		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_dl - pkt->l2_len);
> > +
> >  	ip_dl -= pkt->l3_len;
> >  	ip_id = rte_be_to_cpu_16(ipv4_hdr->packet_id);
> >  	frag_offset = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);
> > --
> > 2.25.1
Thomas Monjalon Oct. 9, 2022, 5:26 p.m. UTC | #4 
15/08/2022 03:27, Hu, Jiayu:
> From: Jun Qiu <jun.qiu@jaguarmicro.com>
> > 
> > I don't think the stack(software) cares if the length of a packet is less than 60,
> > especially when receiving it.
> > In the linux kernel, if the packet length does not match the IP total-length,
> > the packet is flushed to the stack instead of GRO. The previous GRO cache
> > packets are also flushed into the stack.
> > 
> > If you trim padding in merge_two_tcp4_packets(), then both "pkt_head"
> > and "pkt_tail" need to decide whether to trim or not, which can be a bit tricky
> > to handle.
> 
> OK. This patch is OK for me.
> 
> Acked-by: Jiayu Hu <Jiayu.hu@intel.com>

Applied, thanks.
diff mbox series

Patch

diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c
index 7849a2bd1d..0110db5748 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, hdr_len, frag_off, ip_tlen;
 	uint8_t is_atomic;
 
 	struct tcp4_flow_key key;
@@ -233,6 +233,11 @@  gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	if (tcp_dl <= 0)
 		return -1;
 
+	/* trim the tail padding bytes */
+	ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
+	if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
+		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
+
 	/*
 	 * Save IPv4 ID for the packet whose DF bit is 0. For the packet
 	 * whose DF bit is 1, IPv4 ID is ignored.
diff --git a/lib/gro/gro_udp4.c b/lib/gro/gro_udp4.c
index dd71135ada..839f9748b7 100644
--- a/lib/gro/gro_udp4.c
+++ b/lib/gro/gro_udp4.c
@@ -231,6 +231,10 @@  gro_udp4_reassemble(struct rte_mbuf *pkt,
 	if (ip_dl <= pkt->l3_len)
 		return -1;
 
+	/* trim the tail padding bytes */
+	if (pkt->pkt_len > (uint32_t)(ip_dl + pkt->l2_len))
+		rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_dl - pkt->l2_len);
+
 	ip_dl -= pkt->l3_len;
 	ip_id = rte_be_to_cpu_16(ipv4_hdr->packet_id);
 	frag_offset = rte_be_to_cpu_16(ipv4_hdr->fragment_offset);