diff mbox series

gro : fix pkt length when extra bytes are padded to ethernet frame

Message ID 20221010175109.44282-1-kumaraparmesh92@gmail.com (mailing list archive)
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers
Series gro : fix pkt length when extra bytes are padded to ethernet frame |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation warning apply issues
ci/iol-testing warning apply patch failure

Commit Message

Kumara Parameshwaran Oct. 10, 2022, 5:51 p.m. UTC 
  From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>

When GRO packets are merged the packet length is used while
merging the adjacent packets. If the padded bytes are accounted
we would end up acking unsent TCP segments.

Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
v1:
	If there is padding to the ethernet frame cases where timestamp is disabled
	the packet length should be validated with the total ip length as packet length 
	is used in the GRO merging logic

---
 lib/gro/gro_tcp4.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

David Marchand Oct. 10, 2022, 7:32 p.m. UTC | #1 
On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
<kumaraparamesh92@gmail.com> wrote:
>
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
>
> When GRO packets are merged the packet length is used while
> merging the adjacent packets. If the padded bytes are accounted
> we would end up acking unsent TCP segments.
>
> Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> v1:
>         If there is padding to the ethernet frame cases where timestamp is disabled
>         the packet length should be validated with the total ip length as packet length
>         is used in the GRO merging logic

Please, can you test with current main branch?
We recently merged a fix:
https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403
Kumara Parameshwaran Oct. 11, 2022, 5:47 a.m. UTC | #2 
On Tue, Oct 11, 2022 at 1:03 AM David Marchand <david.marchand@redhat.com>
wrote:

> On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
> <kumaraparamesh92@gmail.com> wrote:
> >
> > From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> >
> > When GRO packets are merged the packet length is used while
> > merging the adjacent packets. If the padded bytes are accounted
> > we would end up acking unsent TCP segments.
> >
> > Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> > v1:
> >         If there is padding to the ethernet frame cases where timestamp
> is disabled
> >         the packet length should be validated with the total ip length
> as packet length
> >         is used in the GRO merging logic
>
> Please, can you test with current main branch?
> We recently merged a fix:
>
> https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403
>
> Thanks, David. I did look at the patch. This would fix the problem, but
>> lets say for a case of an ACK packet where TCP data length would be zero
>> and if the packet contains the padded bytes, this check should be done
>> before the follwing check and not after this. @Hu, Jiayu
>> <jiayu.hu@intel.com>  @Jun Qiu <jun.qiu@jaguarmicro.com>  please let me
>> know your thoughts.
>>
> /*
>> * Don't process the packet whose payload length is less than or
>> * equal to 0.
>> */
>> tcp_dl = pkt->pkt_len - hdr_len;
>> if (tcp_dl <= 0)
>> return -1;
>>
> --
> David Marchand
>
>
Jun Qiu Oct. 12, 2022, 1:34 a.m. UTC | #3 
Yes,  It's better to do it before the tcp_dl check.


  1.  /* trim the tail padding bytes */
  2.  ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
  3.  if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
  4.      rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
  5.
  6.  /*
  7.   * Don't process the packet whose payload length is less than or
  8.   * equal to 0.
  9.   */
  10. tcp_dl = pkt->pkt_len - hdr_len;
  11. if (tcp_dl <= 0)
  12.     return -1;
From: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>
Sent: Tuesday, October 11, 2022 1:48 PM
To: dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Hu, Jiayu <jiayu.hu@intel.com>; Jun Qiu <jun.qiu@jaguarmicro.com>
Subject: Fwd: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame



On Tue, Oct 11, 2022 at 1:03 AM David Marchand <david.marchand@redhat.com<mailto:david.marchand@redhat.com>> wrote:
On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
<kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>> wrote:
>
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
>
> When GRO packets are merged the packet length is used while
> merging the adjacent packets. If the padded bytes are accounted
> we would end up acking unsent TCP segments.
>
> Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
> v1:
>         If there is padding to the ethernet frame cases where timestamp is disabled
>         the packet length should be validated with the total ip length as packet length
>         is used in the GRO merging logic

Please, can you test with current main branch?
We recently merged a fix:
https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403
Thanks, David. I did look at the patch. This would fix the problem, but lets say for a case of an ACK packet where TCP data length would be zero and if the packet contains the padded bytes, this check should be done before the follwing check and not after this. @Hu, Jiayu<mailto:jiayu.hu@intel.com>  @Jun Qiu<mailto:jun.qiu@jaguarmicro.com>  please let me know your thoughts.
/*
* Don't process the packet whose payload length is less than or
* equal to 0.
*/
tcp_dl = pkt->pkt_len - hdr_len;
if (tcp_dl <= 0)
return -1;
--
David Marchand
Kumara Parameshwaran Oct. 12, 2022, 7:48 a.m. UTC | #4 
Shall I raise a PR with the change ?


On Wed, Oct 12, 2022 at 7:04 AM Jun Qiu <jun.qiu@jaguarmicro.com> wrote:

> Yes,  It's better to do it before the tcp_dl check.
>
>
>
>    1. /* trim the tail padding bytes */
>    2. ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
>    3. *if* (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
>    4.     rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
>    5.
>    6. /*
>    7.  * Don't process the packet whose payload length is less than or
>    8.  * equal to 0.
>    9.  */
>    10. tcp_dl = pkt->pkt_len - hdr_len;
>    11. *if* (tcp_dl <= 0)
>    12.     *return* -1;
>
> *From:* kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>
> *Sent:* Tuesday, October 11, 2022 1:48 PM
> *To:* dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Jun Qiu <jun.qiu@jaguarmicro.com>
> *Subject:* Fwd: [PATCH] gro : fix pkt length when extra bytes are padded
> to ethernet frame
>
>
>
>
>
>
>
> On Tue, Oct 11, 2022 at 1:03 AM David Marchand <david.marchand@redhat.com>
> wrote:
>
> On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
> <kumaraparamesh92@gmail.com> wrote:
> >
> > From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> >
> > When GRO packets are merged the packet length is used while
> > merging the adjacent packets. If the padded bytes are accounted
> > we would end up acking unsent TCP segments.
> >
> > Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> > v1:
> >         If there is padding to the ethernet frame cases where timestamp
> is disabled
> >         the packet length should be validated with the total ip length
> as packet length
> >         is used in the GRO merging logic
>
> Please, can you test with current main branch?
> We recently merged a fix:
>
> https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403
>
> Thanks, David. I did look at the patch. This would fix the problem, but
> lets say for a case of an ACK packet where TCP data length would be zero
> and if the packet contains the padded bytes, this check should be done
> before the follwing check and not after this. @Hu, Jiayu
> <jiayu.hu@intel.com>  @Jun Qiu <jun.qiu@jaguarmicro.com>  please let me
> know your thoughts.
>
> /*
> * Don't process the packet whose payload length is less than or
> * equal to 0.
> */
> tcp_dl = pkt->pkt_len - hdr_len;
> if (tcp_dl <= 0)
> return -1;
>
> --
> David Marchand
>
>
David Marchand Oct. 12, 2022, 7:50 a.m. UTC | #5 
On Wed, Oct 12, 2022 at 9:48 AM kumaraparameshwaran rathinavel
<kumaraparamesh92@gmail.com> wrote:
>
> Shall I raise a PR with the change ?

Let's give some time to Jiayu and Jun to reply.
Thanks.
Hu, Jiayu Oct. 16, 2022, 7:01 a.m. UTC | #6 
Hi Kumara,

Agree. We need to trim padding bytes first, then do the length check.

Thanks,
Jiayu

From: Jun Qiu <jun.qiu@jaguarmicro.com>
Sent: Wednesday, October 12, 2022 9:35 AM
To: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>; dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Hu, Jiayu <jiayu.hu@intel.com>
Subject: RE: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame

Yes,  It's better to do it before the tcp_dl check.


  1.  /* trim the tail padding bytes */
  2.  ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
  3.  if (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
  4.      rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
  5.
  6.  /*
  7.   * Don't process the packet whose payload length is less than or
  8.   * equal to 0.
  9.   */
  10. tcp_dl = pkt->pkt_len - hdr_len;
  11. if (tcp_dl <= 0)
  12.     return -1;
From: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
Sent: Tuesday, October 11, 2022 1:48 PM
To: dev@dpdk.org<mailto:dev@dpdk.org>; David Marchand <david.marchand@redhat.com<mailto:david.marchand@redhat.com>>; Hu, Jiayu <jiayu.hu@intel.com<mailto:jiayu.hu@intel.com>>; Jun Qiu <jun.qiu@jaguarmicro.com<mailto:jun.qiu@jaguarmicro.com>>
Subject: Fwd: [PATCH] gro : fix pkt length when extra bytes are padded to ethernet frame



On Tue, Oct 11, 2022 at 1:03 AM David Marchand <david.marchand@redhat.com<mailto:david.marchand@redhat.com>> wrote:
On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
<kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>> wrote:
>
> From: Kumara Parameshwaran <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
>
> When GRO packets are merged the packet length is used while
> merging the adjacent packets. If the padded bytes are accounted
> we would end up acking unsent TCP segments.
>
> Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com<mailto:kumaraparamesh92@gmail.com>>
> v1:
>         If there is padding to the ethernet frame cases where timestamp is disabled
>         the packet length should be validated with the total ip length as packet length
>         is used in the GRO merging logic

Please, can you test with current main branch?
We recently merged a fix:
https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403
Thanks, David. I did look at the patch. This would fix the problem, but lets say for a case of an ACK packet where TCP data length would be zero and if the packet contains the padded bytes, this check should be done before the follwing check and not after this. @Hu, Jiayu<mailto:jiayu.hu@intel.com>  @Jun Qiu<mailto:jun.qiu@jaguarmicro.com>  please let me know your thoughts.
/*
* Don't process the packet whose payload length is less than or
* equal to 0.
*/
tcp_dl = pkt->pkt_len - hdr_len;
if (tcp_dl <= 0)
return -1;
--
David Marchand
Kumara Parameshwaran Oct. 17, 2022, 1:27 p.m. UTC | #7 
Hi Jiayu,

Please find the patch
http://patches.dpdk.org/project/dpdk/patch/20221016144305.19249-1-kumaraparmesh92@gmail.com/

I did reply with the above message ID but for some reason a new patch
series was created. Not sure what was the mistake that I made. Should the
commit message be the same for the series ?

Thanks,
Kumara.

On Sun, Oct 16, 2022 at 12:31 PM Hu, Jiayu <jiayu.hu@intel.com> wrote:

> Hi Kumara,
>
>
>
> Agree. We need to trim padding bytes first, then do the length check.
>
>
>
> Thanks,
>
> Jiayu
>
>
>
> *From:* Jun Qiu <jun.qiu@jaguarmicro.com>
> *Sent:* Wednesday, October 12, 2022 9:35 AM
> *To:* kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>;
> dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Hu, Jiayu <
> jiayu.hu@intel.com>
> *Subject:* RE: [PATCH] gro : fix pkt length when extra bytes are padded
> to ethernet frame
>
>
>
> Yes,  It's better to do it before the tcp_dl check.
>
>
>
>    1. /* trim the tail padding bytes */
>    2. ip_tlen = rte_be_to_cpu_16(ipv4_hdr->total_length);
>    3. *if* (pkt->pkt_len > (uint32_t)(ip_tlen + pkt->l2_len))
>    4.     rte_pktmbuf_trim(pkt, pkt->pkt_len - ip_tlen - pkt->l2_len);
>    5.
>    6. /*
>    7.  * Don't process the packet whose payload length is less than or
>    8.  * equal to 0.
>    9.  */
>    10. tcp_dl = pkt->pkt_len - hdr_len;
>    11. *if* (tcp_dl <= 0)
>    12.     *return* -1;
>
> *From:* kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>
> *Sent:* Tuesday, October 11, 2022 1:48 PM
> *To:* dev@dpdk.org; David Marchand <david.marchand@redhat.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Jun Qiu <jun.qiu@jaguarmicro.com>
> *Subject:* Fwd: [PATCH] gro : fix pkt length when extra bytes are padded
> to ethernet frame
>
>
>
>
>
>
>
> On Tue, Oct 11, 2022 at 1:03 AM David Marchand <david.marchand@redhat.com>
> wrote:
>
> On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
> <kumaraparamesh92@gmail.com> wrote:
> >
> > From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> >
> > When GRO packets are merged the packet length is used while
> > merging the adjacent packets. If the padded bytes are accounted
> > we would end up acking unsent TCP segments.
> >
> > Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> > v1:
> >         If there is padding to the ethernet frame cases where timestamp
> is disabled
> >         the packet length should be validated with the total ip length
> as packet length
> >         is used in the GRO merging logic
>
> Please, can you test with current main branch?
> We recently merged a fix:
>
> https://git.dpdk.org/dpdk/commit/?id=b8a55871d5af6f5b8694b1cb5eacbc629734e403
>
> Thanks, David. I did look at the patch. This would fix the problem, but
> lets say for a case of an ACK packet where TCP data length would be zero
> and if the packet contains the padded bytes, this check should be done
> before the follwing check and not after this. @Hu, Jiayu
> <jiayu.hu@intel.com>  @Jun Qiu <jun.qiu@jaguarmicro.com>  please let me
> know your thoughts.
>
> /*
> * Don't process the packet whose payload length is less than or
> * equal to 0.
> */
> tcp_dl = pkt->pkt_len - hdr_len;
> if (tcp_dl <= 0)
> return -1;
>
> --
> David Marchand
>
>
Stephen Hemminger Oct. 31, 2023, 6:49 p.m. UTC | #8 
On Mon, 17 Oct 2022 18:57:44 +0530
kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com> wrote:

> From: kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>
> To: "Hu, Jiayu" <jiayu.hu@intel.com>
> Cc: Jun Qiu <jun.qiu@jaguarmicro.com>, "dev@dpdk.org" <dev@dpdk.org>,   David Marchand <david.marchand@redhat.com>
> Subject: Re: [PATCH] gro : fix pkt length when extra bytes are padded to  ethernet frame
> Date: Mon, 17 Oct 2022 18:57:44 +0530
> 
> Hi Jiayu,
> 
> Please find the patch
> http://patches.dpdk.org/project/dpdk/patch/20221016144305.19249-1-kumaraparmesh92@gmail.com/
> 
> I did reply with the above message ID but for some reason a new patch
> series was created. Not sure what was the mistake that I made. Should the
> commit message be the same for the series ?
> 
> Thanks,
> Kumara.

I don't see a conclusive reply in this mail thread.
And the patch doesn't apply to current main.
Therefore if the problem still exists, please rebase and resubmit.
diff mbox series

Patch

diff --git a/lib/gro/gro_tcp4.c b/lib/gro/gro_tcp4.c
index 7498c66141..ac6439bb4e 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_len;
 	uint8_t is_atomic;
 
 	struct tcp4_flow_key key;
@@ -219,6 +219,12 @@  gro_tcp4_reassemble(struct rte_mbuf *pkt,
 	tcp_hdr = (struct rte_tcp_hdr *)((char *)ipv4_hdr + pkt->l3_len);
 	hdr_len = pkt->l2_len + pkt->l3_len + pkt->l4_len;
 
+	ip_len = rte_be_to_cpu_16(ipv4_hdr->total_length);
+	/* To handle padding cases in cases like where timestamps are disabled */
+	if (unlikely(pkt->pkt_len > (uint32_t)(ip_len + pkt->l2_len))) {
+		pkt->pkt_len = ip_len + pkt->l2_len;
+	}
+
 	/*
 	 * Don't process the packet which has FIN, SYN, RST, PSH, URG, ECE
 	 * or CWR set.