DPDK patches and discussions
 help / color / mirror / Atom feed
From: Gagandeep Singh <G.Singh@nxp.com>
To: Konstantin Ananyev <konstantin.ananyev@huawei.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>,
	Sean Morrissey <sean.morrissey@intel.com>
Cc: "stable@dpdk.org" <stable@dpdk.org>
Subject: RE: [PATCH 3/3] examples/l3fwd: fix maximum acceptable port ID in routes
Date: Mon, 22 Jul 2024 04:27:41 +0000	[thread overview]
Message-ID: <AS8SPR01MB0024B39AC860F5F9F0F403C2E1A82@AS8SPR01MB0024.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <AS8SPR01MB00247F538839FCDC58BE4B6AE1A82@AS8SPR01MB0024.eurprd04.prod.outlook.com>

Hi,

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

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


  reply	other threads:[~2024-07-22  4:28 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-07-15 10:14 [PATCH 1/3] examples/l3fwd: support single route file Gagandeep Singh
2024-07-15 10:14 ` [PATCH 2/3] examples/l3fwd: fix return value on rules add Gagandeep Singh
2024-07-16  6:55   ` Hemant Agrawal
2024-07-15 10:14 ` [PATCH 3/3] examples/l3fwd: fix maximum acceptable port ID in routes Gagandeep Singh
2024-07-17 10:17   ` Konstantin Ananyev
2024-07-18  6:30     ` Gagandeep Singh
2024-07-18 10:01       ` Konstantin Ananyev
2024-07-22  3:28         ` Gagandeep Singh
2024-07-22  4:27           ` Gagandeep Singh [this message]
2024-07-23 16:22             ` Konstantin Ananyev
2024-07-24  8:02               ` Konstantin Ananyev
2024-08-02 10:13                 ` Gagandeep Singh
2024-08-06  3:41 ` [v2 0/3] L3fwd changes Gagandeep Singh
2024-08-06  3:41   ` [v2 1/3] examples/l3fwd: support single route file Gagandeep Singh
2024-08-06  3:41   ` [v2 2/3] examples/l3fwd: fix return value on rules add Gagandeep Singh
2024-08-06  3:41   ` [v2 3/3] examples/l3fwd: enhance valid ports checking Gagandeep Singh
2024-11-13 19:39     ` Stephen Hemminger
2024-11-19 17:13       ` Thomas Monjalon
2024-11-20  4:07         ` Gagandeep Singh
2024-11-20  4:05   ` [v3 0/3] L3fwd changes Gagandeep Singh
2024-11-20  4:05     ` [v3 1/3] examples/l3fwd: support single route file Gagandeep Singh
2024-11-20  4:05     ` [v3 2/3] examples/l3fwd: fix return value on rules add Gagandeep Singh
2024-11-20  4:05     ` [v3 3/3] examples/l3fwd: enhance valid ports checking Gagandeep Singh
2024-11-20 11:10       ` Konstantin Ananyev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AS8SPR01MB0024B39AC860F5F9F0F403C2E1A82@AS8SPR01MB0024.eurprd04.prod.outlook.com \
    --to=g.singh@nxp.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@huawei.com \
    --cc=konstantin.v.ananyev@yandex.ru \
    --cc=sean.morrissey@intel.com \
    --cc=stable@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).