From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id A1AFE1B1FC; Tue, 8 Jan 2019 11:39:10 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Jan 2019 02:39:08 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,453,1539673200"; d="scan'208";a="125157113" Received: from irsmsx153.ger.corp.intel.com ([163.33.192.75]) by orsmga001.jf.intel.com with ESMTP; 08 Jan 2019 02:39:08 -0800 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.116]) by IRSMSX153.ger.corp.intel.com ([169.254.9.115]) with mapi id 14.03.0415.000; Tue, 8 Jan 2019 10:39:07 +0000 From: "Ananyev, Konstantin" To: "Hu, Jiayu" , Stephen Hemminger CC: "dev@dpdk.org" , "Bie, Tiwei" , "Richardson, Bruce" , "stable@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] gro: add missing invalid packet checks Thread-Index: AQHUpxi6gYDMrmZp9k2BAzziHTP876Wk6XyAgAAclYCAACVRUA== Date: Tue, 8 Jan 2019 10:39:06 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258010D900817@irsmsx105.ger.corp.intel.com> References: <1546567036-29444-1-git-send-email-jiayu.hu@intel.com> <1546927725-68831-1-git-send-email-jiayu.hu@intel.com> <20190107223151.18b185b7@hermes.lan> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZDAxNWJmZjQtZWQ5Mi00MTkxLWI0NjUtNzFkZTIxYTM5MTAxIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiUHlxdDdSRnNQN0Z0Rk40VDNLQW5pS3N2c2ZtM041TU9iVVdVeHZySW9YQkUrOVVKUXVMZmRBcUE1bnBhcDdCeSJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Jan 2019 10:39:11 -0000 >=20 >=20 > > -----Original Message----- > > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > > Sent: Tuesday, January 8, 2019 2:32 PM > > To: Hu, Jiayu > > Cc: dev@dpdk.org; Bie, Tiwei ; Richardson, Bruce > > ; stable@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH] gro: add missing invalid packet checks > > > > On Tue, 8 Jan 2019 14:08:45 +0800 > > Jiayu Hu wrote: > > > > > + /* > > > + * Don't process the packet whose Ethernet, IPv4 and TCP header > > > + * lengths are invalid. In addition, if the IPv4 header contains > > > + * Options, the packet shouldn't be processed. > > > + */ > > > + if (unlikely(ILLEGAL_ETHER_HDRLEN(pkt->l2_len) || > > > + ILLEGAL_IPV4_HDRLEN(pkt->l3_len) || > > > + ILLEGAL_TCP_HDRLEN(pkt->l4_len))) > > > + return -1; >=20 > In the GRO design, we assume applications give correct > MBUF->l2_len/.. for input packets of GRO. Specifically, GRO > library assumes applications will set values to MBUF->l2_len/... > and guarantee the values are the same as the values in the packet > headers. The reason for this assumption is to process header faster. > This is also why I want to add this assumption in the programmer > guide. >=20 > The above code is to forbid GRO to process invalid packets, which > have invalid packet header lengths, like TCP header length is less than > 20 bytes. >=20 > > > > I like it when code is as picky as possible when doing optimizations be= cause > > it reduces possible security riskg. > > > > To me this looks more confusing and not as careful as doing it like: > > > > if (unlikely(pkt->l2_len !=3D ETHER_HDR_LEN)) > > return -1; > > eth_hdr =3D rte_pktmbuf_mtod(pkt, struct ether_hdr *); > > ipv4_hdr =3D (struct ipv4_hdr *)((char *)eth_hdr + ETHER_HDR_LEN); > > > > if (pkt->l3_len !=3D (ipv4->version_ihl & IPV4_HDR_IHL_MASK) << 4) > > return -1; > > > > if (pkt->l4_len < sizeof(struct tcp_hdr)) > > return -1; > > > > You should also check for TCP options as well. >=20 > There are two ways to get ether, ipv4 and tcp headers: > 1). Use MBUF->l2_len/l3_len...; > 2). Parse packet and ignore MBUF->l2_len/.... >=20 > If we follow the choice 1, we don't need to parse packet and > don't need to check if values of MBUF->l2_len/... are correct, > since we assume applications will set correct values. If we follow > the choice 2, we don't need to care about the values of MBUF->l2_len/... >=20 > I am a little confused about your code, since it parses packet and > checks if the values of MBUF->l2_len/... are correct. If we don't use > MBUF->l2_len/... to get ether/ipv4/tcp headers, why should we check > the values of MBUF->l2_len/...? >=20 Agree that we don't need both. My preference would be to stick with 1). In many cases user would have already determined l2/l3/l4 len by this stage. Konstantin