From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 1AA3D5B38; Tue, 15 Jan 2019 03:49:02 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Jan 2019 18:49:02 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,480,1539673200"; d="scan'208";a="108279942" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga006.jf.intel.com with ESMTP; 14 Jan 2019 18:49:02 -0800 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 14 Jan 2019 18:49:01 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.408.0; Mon, 14 Jan 2019 18:49:01 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.63]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.160]) with mapi id 14.03.0415.000; Tue, 15 Jan 2019 10:48:58 +0800 From: "Hu, Jiayu" To: Stephen Hemminger CC: "dev@dpdk.org" , "Ananyev, Konstantin" , "thomas@monjalon.net" , "stable@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks Thread-Index: AQHUqPYPT3KiVpCI1kS+zBXANWFesaWvA2AAgACIi8A= Date: Tue, 15 Jan 2019 02:48:58 +0000 Message-ID: References: <1546927725-68831-1-git-send-email-jiayu.hu@intel.com> <1547132768-2384-1-git-send-email-jiayu.hu@intel.com> <20190114170028.68bdd4d7@hermes.lan> In-Reply-To: <20190114170028.68bdd4d7@hermes.lan> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiODYyYmU4Y2MtYmY5Yy00NzUwLTg1NGItNmMzZTczZjA1MzY4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiTldMQkI2aEt4VkJOeDZqR2IydDFnTTBzR3BFdmhXUEdRbEtRS0lVSFhcL2RNZDNFNThsYUhTcytyMytKUmtKaUsifQ== x-ctpclassification: CTP_NT x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2] 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, 15 Jan 2019 02:49:03 -0000 > -----Original Message----- > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Tuesday, January 15, 2019 9:00 AM > To: Hu, Jiayu > Cc: dev@dpdk.org; Ananyev, Konstantin ; > thomas@monjalon.net; stable@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2] gro: add missing invalid packet checks >=20 > On Thu, 10 Jan 2019 23:06:08 +0800 > Jiayu Hu wrote: >=20 > > + > > +#define ILLEGAL_ETHER_HDRLEN(len) ((len) !=3D ETHER_HDR_LEN) > > +#define ILLEGAL_ETHER_VXLAN_HDRLEN(len) \ > > + ((len) !=3D (ETHER_VXLAN_HLEN + ETHER_HDR_LEN)) > > +#define ILLEGAL_IPV4_HDRLEN(len) ((len) !=3D sizeof(struct ipv4_hdr)) > > +#define ILLEGAL_TCP_HDRLEN(len) \ > > + (((len) < sizeof(struct tcp_hdr)) || ((len) > TCP_MAX_HLEN)) > > + >=20 > Why not inline (which keeps type checking) instead of macro. > Results in same code. You mean the inline functions like the following two? static inline int check_invalid_hdr _tcp4(mbuf) { if (unlikely(mbuf->l2_len !=3D ETHER_HDR_LEN || mbuf->l3_len !=3D sizeof(struct ipv4_hdr) || mbuf->l4_len < sizeof(struct tcp_hdr) || mbuf->l4_len > 60)) return 1; return 0; } static inline int check_invalid_hdr_vxlan_tcp4(mbuf) { if (unlikely(mbuf->outer_l2_len !=3D ETHER_HDR_LEN || mbuf->outer_l3_len !=3D sizeof(struct ipv4_hdr) || mbuf->l2_len !=3D (ETHER_VXLAN_HLEN + ETHER_HDR_LEN) || mbuf->l3_len !=3D sizeof(struct ipv4_hdr) || mbuf->l4_len < sizeof(struct tcp_hdr) || mbuf->l4_len > 60)) return 1; return 0; } Or you mean every header length check should be one inline function, like: check_invalid_ipv4_hdr(), check_invalid_vxlan_hdr() etc. ? What is the main benefit of inline function for length check here? >=20 > Also, prefer "invalid" instead "ILLEGAL" . There is no government inforci= ng > a rule on packet headers. Thanks. I will change the name. >=20 > Also, what about ipv4 options, or TCP options? Usually, IPv4 header doesn't have Options. Therefore, the implementations of TCP and VxLAN GRO don't consider IPv4 Option fields when merge packets. For TCP Options, GRO checks Option fields when merges packets. So in the above code, the valid TCP header length is 20~60, and valid IPv4 header length is 20. >=20 > And even VXLAN header check should be more rigorous. What about not > allowing > fragments in IP header for example. Hmm, this is an assumption that all input packets are not IP fragments, whi= ch is stated in programmer guide. But maybe too many assumptions make it hard for users to use GRO. I will add this check. BTW, if the received packets from PMD are IP fragments, their MBUF->packet_= type will include L4 protocols? >=20 > If you are going to do enforcement, be as strict as you can.