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

[Hu, Jiayu]

Hi Kumara,

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

Thanks,
Jiayu

From: Jun Qiu <jun.qiu at jaguarmicro.com>
Sent: Wednesday, October 12, 2022 9:35 AM
To: kumaraparameshwaran rathinavel <kumaraparamesh92 at gmail.com>; dev at dpdk.org; David Marchand <david.marchand at redhat.com>; Hu, Jiayu <jiayu.hu at 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 at gmail.com<mailto:kumaraparamesh92 at gmail.com>>
Sent: Tuesday, October 11, 2022 1:48 PM
To: dev at dpdk.org<mailto:dev at dpdk.org>; David Marchand <david.marchand at redhat.com<mailto:david.marchand at redhat.com>>; Hu, Jiayu <jiayu.hu at intel.com<mailto:jiayu.hu at intel.com>>; Jun Qiu <jun.qiu at jaguarmicro.com<mailto:jun.qiu at 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 at redhat.com<mailto:david.marchand at redhat.com>> wrote:
On Mon, Oct 10, 2022 at 7:51 PM Kumara Parameshwaran
<kumaraparamesh92 at gmail.com<mailto:kumaraparamesh92 at gmail.com>> wrote:
>
> From: Kumara Parameshwaran <kumaraparamesh92 at gmail.com<mailto:kumaraparamesh92 at 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 at gmail.com<mailto:kumaraparamesh92 at 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 at intel.com>  @Jun Qiu<mailto:jun.qiu at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mails.dpdk.org/archives/dev/attachments/20221016/2fec4d43/attachment-0001.htm>