RE: [PATCH] gro: fix gro with ethernet tail padding bytes

[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.


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