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: Thu, 18 Jul 2024 06:30:36 +0000	[thread overview]
Message-ID: <PAXPR04MB82056D2FF527BFA057220C4CE1AC2@PAXPR04MB8205.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <370cf325ad02427cbca5a37756da4c35@huawei.com>

Hi,

> -----Original Message-----
> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Sent: Wednesday, July 17, 2024 3:48 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.

> ((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+.

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

> 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-18  6:30 UTC|newest]

Thread overview: 16+ 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 [this message]
2024-07-18 10:01       ` Konstantin Ananyev
2024-07-22  3:28         ` Gagandeep Singh
2024-07-22  4:27           ` Gagandeep Singh
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

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=PAXPR04MB82056D2FF527BFA057220C4CE1AC2@PAXPR04MB8205.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).