RE: [PATCH 3/3] examples/l3fwd: fix maximum acceptable port ID in routes

[Gagandeep Singh <G.Singh@nxp.com>]

Hi,

> -----Original Message-----
> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Sent: Wednesday, July 24, 2024 1:32 PM
> To: Konstantin Ananyev <konstantin.ananyev@huawei.com>; Gagandeep
> Singh <G.Singh@nxp.com>; dev@dpdk.org; Konstantin Ananyev
> <konstantin.v.ananyev@yandex.ru>; Sean Morrissey
> <sean.morrissey@intel.com>
> Cc: stable@dpdk.org
> Subject: RE: [PATCH 3/3] examples/l3fwd: fix maximum acceptable port ID
> in routes
> 
> 
> 
> > > > > > > > Application is accepting routes for port ID up to
> > > > > > > > UINT8_MAX for 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 this route rule will be skipped:
> > > > > > >
> > > > > > >         /* populate the LPM table */
> > > > > > >         for (i = 0; i < route_num_v4; i++) {
> > > > > > >                 struct in_addr in;
> > > > > > >
> > > > > > >                 /* skip unused ports */
> > > > > > >                 if ((1 << route_base_v4[i].if_out &
> > > > > > >                                 enabled_port_mask) == 0)
> > > > > > >                         continue;
> > > > > > >
> > > > > > > Same for EM.
> > > > > > I am trying to update the check for MAX if_out value in rules
> > > > > > config 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
> > > > > > port ID while parsing rule file becomes more valid as this
> > > > > > check can pass 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_lpm().
> > > > > Something like:
> > > > >
> > > > > if (route_base_v4[i].if_out >= sizeof(enabled_port_mask) * CHAR_BIT
> ||
> > > > >    ((1 << route_base_v4[i].if_out & enabled_port_mask) == 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_config_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 with -p
> option.
> >
> > 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.
Sorry for late reply, first option looks ok to me. I can add a user given option in
Setup functions to decide skip or continue. 
In V2, also will try to create a common function for this check for all the setup
Functions.

> 
> >
> > > >
> > > > > >
> > > > > > > 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 port ID
> is 31+.
> > > > > > In setup_lpm, skipping the rules for the ports which are not
> > > > > > enabled and not giving error, I guess probably because of ease of
> use.
> > > > > > e.g. user has only single ipv4_routes config file with route
> > > > > > rules 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
> > > > > > each 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
> > > > > with
> > > > > 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 does.
> > > > > > >
> > > > > > > > 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 <g.singh@nxp.com>
> > > > > > > > ---
> > > > > > > >  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..65c71cd1ba
> > > > > > > > 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_MAX, 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_rule *v)
> > > > > > > >  	/* protocol. */
> > > > > > > >  	GET_CB_FIELD(in[CB_FLD_PROTO], v->v4_key.proto, 0,
> UINT8_MAX, 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 = 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 = 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
> > >