From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id AC01145698; Wed, 24 Jul 2024 10:12:05 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BC201427CC; Wed, 24 Jul 2024 10:12:04 +0200 (CEST) Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by mails.dpdk.org (Postfix) with ESMTP id 1A69841153; Wed, 24 Jul 2024 10:02:06 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.18.186.31]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4WTRHr5tGkz6K8mZ; Wed, 24 Jul 2024 15:59:36 +0800 (CST) Received: from frapeml500005.china.huawei.com (unknown [7.182.85.13]) by mail.maildlp.com (Postfix) with ESMTPS id 1F434140B2A; Wed, 24 Jul 2024 16:02:01 +0800 (CST) Received: from frapeml500007.china.huawei.com (7.182.85.172) by frapeml500005.china.huawei.com (7.182.85.13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Wed, 24 Jul 2024 10:02:00 +0200 Received: from frapeml500007.china.huawei.com ([7.182.85.172]) by frapeml500007.china.huawei.com ([7.182.85.172]) with mapi id 15.01.2507.039; Wed, 24 Jul 2024 10:02:00 +0200 From: Konstantin Ananyev To: Konstantin Ananyev , Gagandeep Singh , "dev@dpdk.org" , Konstantin Ananyev , Sean Morrissey CC: "stable@dpdk.org" Subject: RE: [PATCH 3/3] examples/l3fwd: fix maximum acceptable port ID in routes Thread-Topic: [PATCH 3/3] examples/l3fwd: fix maximum acceptable port ID in routes Thread-Index: AQHa1qAIJ72CVVlLaEGsAap4GVnOybH6t9YAgAFKzOCAAEMlgIAF2LrQgAARmwCAAlj0gIABCGRA Date: Wed, 24 Jul 2024 08:02:00 +0000 Message-ID: <5f6a307304e8472e9379922ae8c427ea@huawei.com> References: <20240715101458.645014-1-g.singh@nxp.com> <20240715101458.645014-3-g.singh@nxp.com> <370cf325ad02427cbca5a37756da4c35@huawei.com> <2d2ec732b68b4faba1af4777bd486893@huawei.com> In-Reply-To: <2d2ec732b68b4faba1af4777bd486893@huawei.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.206.138.42] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > > > > > > > Application is accepting routes for port ID up to UINT8_MAX f= or > > > > > > > LPM amd EM routes on parsing the given rule file, but only up= to > > > > > > > 32 ports can be enabled as per the variable enabled_port_mask > > > > > > > which is defined as uint32_t. > > > > > > > > > > > > > > This patch restricts the rules parsing code to accept routes = for > > > > > > > port ID up to 31 only to avoid any unnecessary maintenance of > > > > > > > rules which will never be used. > > > > > > > > > > > > If we want to add this extra check, probably better to do it in= setup_lpm(). > > > > > > Where we already check that port is enabled, and If not, then t= his > > > > > > route rule will be skipped: > > > > > > > > > > > > /* populate the LPM table */ > > > > > > for (i =3D 0; i < route_num_v4; i++) { > > > > > > struct in_addr in; > > > > > > > > > > > > /* skip unused ports */ > > > > > > if ((1 << route_base_v4[i].if_out & > > > > > > enabled_port_mask) =3D=3D 0) > > > > > > continue; > > > > > > > > > > > > Same for EM. > > > > > I am trying to update the check for MAX if_out value in rules con= fig > > > > > file parsing > > > > which will be before setup_lpm(). > > > > > The reason is, restricting and adding only those rules which can = be > > > > > used by the application while populating the route_base_v4/v6 at > > > > > first step and avoid unnecessary memory allocation for local > > > > > variables to store more > > > > not required rules. > > > > > > > > Hmm... but why it is a problem? > > > Not really a problem, Just trying to optimize wherever it Is possible= . > > > > > > > > > > > > > > > > > > ((1 << route_base_v4[i].if_out & > > > > > > enabled_port_mask) > > > > > By looking into this check, it seems restriction to maximum 31 po= rt > > > > > ID while parsing rule file becomes more valid as this check can p= ass > > > > > due to overflow in case value of route_base_v4[i].if_out Is 31+. > > > > > > > > Agree, I think we need both, and it probably need to be in setup_lp= m(). > > > > Something like: > > > > > > > > if (route_base_v4[i].if_out >=3D sizeof(enabled_port_mask) * CHAR_B= IT || > > > > ((1 << route_base_v4[i].if_out & enabled_port_mask) =3D=3D 0) { > > > > /* print some error message here*/ > > > > rte_exiit(...); /* or return an error */ } > > > > > > > Yes, I can change it to this. > > > > I re-checked the code, IMO we should restrict the rules in " read_confi= g_files" > > May be we can move this check to read_config_files. > > As having this check in the setup can result in rte_exit() call when no= user rule file > > Is given and application is using the default rules. In that case route= _base_v4 will > > Have 16 rules for 16 ports (default rules). > > So this check will fails always unless user enable all the 16 ports wit= h -p option. >=20 > Ah yes, you are right. > That's why probably right now we probably just do 'continue;' here... > Yeh, probably the easiest way is to put this check before setup_lpm() - > in parsing code, or straight after that. > Can I ask you for one more thing: can we add a new function that would > do this check and use it everywhere (lpm/em/acl). As alternative thought - we might add to setup_lpm() an extra parameter to indicate what do we want to do on rule with invalid/disabled port - just= skip it or fail. Another alternative - remove default route ability at all, though that one is a change in behavior and probably there would be some complaints. =20 >=20 > > > > > > > > > > > > > > Another question here - why we just silently skip the rule with= invalid port? > > > > > In read_config_files_lpm() we are calling the rte_exit in case po= rt ID is 31+. > > > > > In setup_lpm, skipping the rules for the ports which are not enab= led > > > > > and not giving error, I guess probably because of ease of use. > > > > > e.g. user has only single ipv4_routes config file with route rule= s > > > > > for port ID 0,1,2,3,4 and want to use same file for multiple test > > > > > cases like 1. when only port 0 enabled 2. when only port 0 and 1 > > > > > enabled and so on. > > > > > In this case, user can avoid to have separate route files for eac= h of the test > > > case. > > > > > > > > The problem as I see it - we are not consistent here. > > > > In some cases we just silently skip rules with invalid (or disabled= ) > > > > port numbers, in other cases we generate an error and exit. > > > > For me it would be better, if we follow one simple policy (abort wi= th > > > > error) here for all cases. > > > Ok, I will add the rte_exit if route port is invalid or not enabled. > > > With this change onwards It will be assumed user will add only those = routes With > > > port IDs which are valid and enabled in the application. > > > > > > > > > > > > > > > > > > Probably need to fail with error... that what ACL code-path doe= s. > > > > > > > > > > > > > Fixes: e7e6dd643092 ("examples/l3fwd: support config file for > > > > > > > EM") > > > > > > > Fixes: 52def963fc1c ("examples/l3fwd: support config file for > > > > > > > LPM/FIB") > > > > > > > Cc: sean.morrissey@intel.com > > > > > > > Cc: stable@dpdk.org > > > > > > > > > > > > > > Signed-off-by: Gagandeep Singh > > > > > > > --- > > > > > > > examples/l3fwd/em_route_parse.c | 6 ++++-- > > > > > > > examples/l3fwd/lpm_route_parse.c | 6 ++++-- > > > > > > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > > > > > > > > > > > diff --git a/examples/l3fwd/em_route_parse.c > > > > > > > b/examples/l3fwd/em_route_parse.c index 8b534de5f1..65c71cd1b= a > > > > > > > 100644 > > > > > > > --- a/examples/l3fwd/em_route_parse.c > > > > > > > +++ b/examples/l3fwd/em_route_parse.c > > > > > > > @@ -65,7 +65,8 @@ em_parse_v6_rule(char *str, struct em_rule = *v) > > > > > > > /* protocol. */ > > > > > > > GET_CB_FIELD(in[CB_FLD_PROTO], v->v6_key.proto, 0, UINT8_MA= X, 0); > > > > > > > /* out interface. */ > > > > > > > - GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, UINT8_MAX, 0)= ; > > > > > > > + GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, > > > > > > > + (sizeof(enabled_port_mask) * CHAR_BIT) - 1, 0); > > > > > > > > > > > > > > return 0; > > > > > > > } > > > > > > > @@ -102,7 +103,8 @@ em_parse_v4_rule(char *str, struct em_rul= e *v) > > > > > > > /* protocol. */ > > > > > > > GET_CB_FIELD(in[CB_FLD_PROTO], v->v4_key.proto, 0, UINT8_MA= X, 0); > > > > > > > /* out interface. */ > > > > > > > - GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, UINT8_MAX, 0)= ; > > > > > > > + GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, > > > > > > > + (sizeof(enabled_port_mask) * CHAR_BIT) - 1, 0); > > > > > > > > > > > > > > return 0; > > > > > > > } > > > > > > > diff --git a/examples/l3fwd/lpm_route_parse.c > > > > > > > b/examples/l3fwd/lpm_route_parse.c > > > > > > > index f27b66e838..357c12d9fe 100644 > > > > > > > --- a/examples/l3fwd/lpm_route_parse.c > > > > > > > +++ b/examples/l3fwd/lpm_route_parse.c > > > > > > > @@ -110,7 +110,8 @@ lpm_parse_v6_rule(char *str, struct > > > > > > > lpm_route_rule > > > > > > > *v) > > > > > > > > > > > > > > rc =3D lpm_parse_v6_net(in[CB_FLD_DST_ADDR], v->ip_32, > > > > > > > &v->depth); > > > > > > > > > > > > > > - GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, UINT8_MAX, 0)= ; > > > > > > > + GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, > > > > > > > + (sizeof(enabled_port_mask) * CHAR_BIT) - 1, 0); > > > > > > > > > > > > > > return rc; > > > > > > > } > > > > > > > @@ -132,7 +133,8 @@ lpm_parse_v4_rule(char *str, struct > > > > > > > lpm_route_rule > > > > > > > *v) > > > > > > > > > > > > > > rc =3D parse_ipv4_addr_mask(in[CB_FLD_DST_ADDR], &v->ip, > > > > > > > &v->depth); > > > > > > > > > > > > > > - GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, UINT8_MAX, 0)= ; > > > > > > > + GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, > > > > > > > + (sizeof(enabled_port_mask) * CHAR_BIT) - 1, 0); > > > > > > > > > > > > > > return rc; > > > > > > > } > > > > > > > -- > > > > > > > 2.25.1 > > > > > > > > > > Gagan > >