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

["Hu, Jiayu" <jiayu.hu@intel.com>]

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