DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Hu, Jiayu" <jiayu.hu@intel.com>
To: Jun Qiu <jun.qiu@jaguarmicro.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "stable@dpdk.org" <stable@dpdk.org>
Subject: RE: [PATCH] gro: fix gro with ethernet tail padding bytes
Date: Mon, 15 Aug 2022 01:27:25 +0000	[thread overview]
Message-ID: <f70c7290d48643bb84620a11e6cbaf61@intel.com> (raw)
In-Reply-To: <KL1PR0601MB45305A12C1DE471DAB47619EF7969@KL1PR0601MB4530.apcprd06.prod.outlook.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


  reply	other threads:[~2022-08-15  1:27 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-27  8:00 Jun Qiu
2022-07-28  1:55 ` Hu, Jiayu
2022-07-28 11:02   ` Jun Qiu
2022-08-15  1:27     ` Hu, Jiayu [this message]
2022-10-09 17:26       ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f70c7290d48643bb84620a11e6cbaf61@intel.com \
    --to=jiayu.hu@intel.com \
    --cc=dev@dpdk.org \
    --cc=jun.qiu@jaguarmicro.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).