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: Thursday, July 18, 2024 3:32 PM
> To: 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.

> >
> > > 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