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

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

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