[PATCH 1/3] examples/l3fwd: support single route file

[PATCH 1/3] examples/l3fwd: support single route file

[Gagandeep Singh]

IPv6 rules file needs to be specified together with IPv4
rules file to configure user given rules. But if user want to
give only IPv4 or only IPv6 rules, application returns error:
"Missing 1 or more rule files"

With this patch application can accept only IPv4,
only IPv6 or both IP rules.

Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
---
 examples/l3fwd/em_route_parse.c  | 18 ++++++++++--------
 examples/l3fwd/lpm_route_parse.c | 17 ++++++++++-------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/examples/l3fwd/em_route_parse.c b/examples/l3fwd/em_route_parse.c
index 6c16832e94..da23356dd6 100644
--- a/examples/l3fwd/em_route_parse.c
+++ b/examples/l3fwd/em_route_parse.c
@@ -249,8 +249,7 @@ void
 read_config_files_em(void)
 {
 	/* ipv4 check */
-	if (parm_config.rule_ipv4_name != NULL &&
-			parm_config.rule_ipv6_name != NULL) {
+	if (parm_config.rule_ipv4_name != NULL) {
 		/* ipv4 check */
 		route_num_v4 = em_add_rules(parm_config.rule_ipv4_name,
 					&em_route_base_v4, &em_parse_v4_rule);
@@ -258,7 +257,14 @@ read_config_files_em(void)
 			em_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add EM IPv4 rules\n");
 		}
-
+	} else {
+		RTE_LOG(INFO, L3FWD, "Missing IPv4 rule file, using default instead\n");
+		if (em_add_default_v4_rules() < 0) {
+			em_free_routes();
+			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
+		}
+	}
+	if (parm_config.rule_ipv6_name != NULL) {
 		/* ipv6 check */
 		route_num_v6 = em_add_rules(parm_config.rule_ipv6_name,
 					&em_route_base_v6, &em_parse_v6_rule);
@@ -267,11 +273,7 @@ read_config_files_em(void)
 			rte_exit(EXIT_FAILURE, "Failed to add EM IPv6 rules\n");
 		}
 	} else {
-		RTE_LOG(INFO, L3FWD, "Missing 1 or more rule files, using default instead\n");
-		if (em_add_default_v4_rules() < 0) {
-			em_free_routes();
-			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
-		}
+		RTE_LOG(INFO, L3FWD, "Missing IPv6 rule file, using default instead\n");
 		if (em_add_default_v6_rules() < 0) {
 			em_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add default IPv6 rules\n");
diff --git a/examples/l3fwd/lpm_route_parse.c b/examples/l3fwd/lpm_route_parse.c
index f2028d79e1..f7d44aa2cd 100644
--- a/examples/l3fwd/lpm_route_parse.c
+++ b/examples/l3fwd/lpm_route_parse.c
@@ -271,8 +271,7 @@ lpm_free_routes(void)
 void
 read_config_files_lpm(void)
 {
-	if (parm_config.rule_ipv4_name != NULL &&
-			parm_config.rule_ipv6_name != NULL) {
+	if (parm_config.rule_ipv4_name != NULL) {
 		/* ipv4 check */
 		route_num_v4 = lpm_add_rules(parm_config.rule_ipv4_name,
 					&route_base_v4, &lpm_parse_v4_rule);
@@ -280,7 +279,15 @@ read_config_files_lpm(void)
 			lpm_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add IPv4 rules\n");
 		}
+	} else {
+		RTE_LOG(INFO, L3FWD, "Missing IPv4 rule file, using default instead\n");
+		if (lpm_add_default_v4_rules() < 0) {
+			lpm_free_routes();
+			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
+		}
+	}
 
+	if (parm_config.rule_ipv6_name != NULL) {
 		/* ipv6 check */
 		route_num_v6 = lpm_add_rules(parm_config.rule_ipv6_name,
 					&route_base_v6, &lpm_parse_v6_rule);
@@ -289,11 +296,7 @@ read_config_files_lpm(void)
 			rte_exit(EXIT_FAILURE, "Failed to add IPv6 rules\n");
 		}
 	} else {
-		RTE_LOG(INFO, L3FWD, "Missing 1 or more rule files, using default instead\n");
-		if (lpm_add_default_v4_rules() < 0) {
-			lpm_free_routes();
-			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
-		}
+		RTE_LOG(INFO, L3FWD, "Missing IPv6 rule file, using default instead\n");
 		if (lpm_add_default_v6_rules() < 0) {
 			lpm_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add default IPv6 rules\n");
-- 
2.25.1

[PATCH 2/3] examples/l3fwd: fix return value on rules add

[Gagandeep Singh]

fix return value on adding the EM or LPM rules.

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  | 11 ++++++-----
 examples/l3fwd/lpm_route_parse.c | 11 ++++++-----
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/examples/l3fwd/em_route_parse.c b/examples/l3fwd/em_route_parse.c
index da23356dd6..8b534de5f1 100644
--- a/examples/l3fwd/em_route_parse.c
+++ b/examples/l3fwd/em_route_parse.c
@@ -119,7 +119,7 @@ em_add_rules(const char *rule_path,
 	char buff[LINE_MAX];
 	FILE *fh;
 	unsigned int i = 0, rule_size = sizeof(*next);
-	int val;
+	int val, rc;
 
 	*proute_base = NULL;
 	fh = fopen(rule_path, "rb");
@@ -172,13 +172,14 @@ em_add_rules(const char *rule_path,
 			return -EINVAL;
 		}
 
-		if (parser(buff + 1, next) != 0) {
+		rc = parser(buff + 1, next);
+		if (rc != 0) {
 			RTE_LOG(ERR, L3FWD,
-				"%s Line %u: parse rules error\n",
-				rule_path, i);
+				"%s Line %u: parse rules error code = %d\n",
+				rule_path, i, rc);
 			fclose(fh);
 			free(route_rules);
-			return -EINVAL;
+			return rc;
 		}
 
 		route_cnt++;
diff --git a/examples/l3fwd/lpm_route_parse.c b/examples/l3fwd/lpm_route_parse.c
index f7d44aa2cd..f27b66e838 100644
--- a/examples/l3fwd/lpm_route_parse.c
+++ b/examples/l3fwd/lpm_route_parse.c
@@ -184,7 +184,7 @@ lpm_add_rules(const char *rule_path,
 	char buff[LINE_MAX];
 	FILE *fh;
 	unsigned int i = 0, rule_size = sizeof(*next);
-	int val;
+	int val, rc;
 
 	*proute_base = NULL;
 	fh = fopen(rule_path, "rb");
@@ -237,13 +237,14 @@ lpm_add_rules(const char *rule_path,
 			return -EINVAL;
 		}
 
-		if (parser(buff + 1, next) != 0) {
+		rc = parser(buff + 1, next);
+		if (rc != 0) {
 			RTE_LOG(ERR, L3FWD,
-				"%s Line %u: parse rules error\n",
-				rule_path, i);
+				"%s Line %u: parse rules error code = %d\n",
+				rule_path, i, rc);
 			fclose(fh);
 			free(route_rules);
-			return -EINVAL;
+			return rc;
 		}
 
 		route_cnt++;
-- 
2.25.1

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

[Gagandeep Singh]

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.

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

Re: [PATCH 2/3] examples/l3fwd: fix return value on rules add

[Hemant Agrawal]

On 15-07-2024 15:44, Gagandeep Singh wrote:
> fix return value on adding the EM or LPM rules.
>
> 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>


Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>

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

[Konstantin Ananyev]

> 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.
Another question here - why we just silently skip the rule with invalid port?
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

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

[Gagandeep Singh]

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

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

[Konstantin Ananyev]

> > > 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?
 
> 
> > ((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 */
} 
   
> 
> > 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.
 
> 
> > 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

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

[Gagandeep Singh]

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

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

[Gagandeep Singh]

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

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

[Konstantin Ananyev]

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

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

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

[Konstantin Ananyev]

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

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

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

[Gagandeep Singh]

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

[v2 0/3] L3fwd changes

[Gagandeep Singh]

v2 changes:
*  Handled a comment to enhance the invalid port ID logic
   and added a user option to decide exit or silently skip
   in case invalid port in the rules list.


Gagandeep Singh (3):
  examples/l3fwd: support single route file
  examples/l3fwd: fix return value on rules add
  examples/l3fwd: enhance valid ports checking

 examples/l3fwd/em_route_parse.c  | 33 ++++++++++--------
 examples/l3fwd/l3fwd.h           | 16 +++++++++
 examples/l3fwd/l3fwd_em.c        | 22 ++++++++----
 examples/l3fwd/l3fwd_fib.c       | 26 +++++++++-----
 examples/l3fwd/l3fwd_lpm.c       | 26 +++++++++-----
 examples/l3fwd/l3fwd_route.h     |  2 ++
 examples/l3fwd/lpm_route_parse.c | 28 ++++++++-------
 examples/l3fwd/main.c            | 60 ++++++++++++++++++++++++++------
 8 files changed, 154 insertions(+), 59 deletions(-)

-- 
2.25.1

[v2 1/3] examples/l3fwd: support single route file

[Gagandeep Singh]

IPv6 rules file needs to be specified together with IPv4
rules file to configure user given rules. But if user want to
give only IPv4 or only IPv6 rules, application returns error:
"Missing 1 or more rule files"

With this patch application can accept only IPv4,
only IPv6 or both IP rules.

Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
---
 examples/l3fwd/em_route_parse.c  | 18 ++++++++++--------
 examples/l3fwd/lpm_route_parse.c | 17 ++++++++++-------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/examples/l3fwd/em_route_parse.c b/examples/l3fwd/em_route_parse.c
index 6c16832e94..da23356dd6 100644
--- a/examples/l3fwd/em_route_parse.c
+++ b/examples/l3fwd/em_route_parse.c
@@ -249,8 +249,7 @@ void
 read_config_files_em(void)
 {
 	/* ipv4 check */
-	if (parm_config.rule_ipv4_name != NULL &&
-			parm_config.rule_ipv6_name != NULL) {
+	if (parm_config.rule_ipv4_name != NULL) {
 		/* ipv4 check */
 		route_num_v4 = em_add_rules(parm_config.rule_ipv4_name,
 					&em_route_base_v4, &em_parse_v4_rule);
@@ -258,7 +257,14 @@ read_config_files_em(void)
 			em_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add EM IPv4 rules\n");
 		}
-
+	} else {
+		RTE_LOG(INFO, L3FWD, "Missing IPv4 rule file, using default instead\n");
+		if (em_add_default_v4_rules() < 0) {
+			em_free_routes();
+			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
+		}
+	}
+	if (parm_config.rule_ipv6_name != NULL) {
 		/* ipv6 check */
 		route_num_v6 = em_add_rules(parm_config.rule_ipv6_name,
 					&em_route_base_v6, &em_parse_v6_rule);
@@ -267,11 +273,7 @@ read_config_files_em(void)
 			rte_exit(EXIT_FAILURE, "Failed to add EM IPv6 rules\n");
 		}
 	} else {
-		RTE_LOG(INFO, L3FWD, "Missing 1 or more rule files, using default instead\n");
-		if (em_add_default_v4_rules() < 0) {
-			em_free_routes();
-			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
-		}
+		RTE_LOG(INFO, L3FWD, "Missing IPv6 rule file, using default instead\n");
 		if (em_add_default_v6_rules() < 0) {
 			em_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add default IPv6 rules\n");
diff --git a/examples/l3fwd/lpm_route_parse.c b/examples/l3fwd/lpm_route_parse.c
index f2028d79e1..f7d44aa2cd 100644
--- a/examples/l3fwd/lpm_route_parse.c
+++ b/examples/l3fwd/lpm_route_parse.c
@@ -271,8 +271,7 @@ lpm_free_routes(void)
 void
 read_config_files_lpm(void)
 {
-	if (parm_config.rule_ipv4_name != NULL &&
-			parm_config.rule_ipv6_name != NULL) {
+	if (parm_config.rule_ipv4_name != NULL) {
 		/* ipv4 check */
 		route_num_v4 = lpm_add_rules(parm_config.rule_ipv4_name,
 					&route_base_v4, &lpm_parse_v4_rule);
@@ -280,7 +279,15 @@ read_config_files_lpm(void)
 			lpm_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add IPv4 rules\n");
 		}
+	} else {
+		RTE_LOG(INFO, L3FWD, "Missing IPv4 rule file, using default instead\n");
+		if (lpm_add_default_v4_rules() < 0) {
+			lpm_free_routes();
+			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
+		}
+	}
 
+	if (parm_config.rule_ipv6_name != NULL) {
 		/* ipv6 check */
 		route_num_v6 = lpm_add_rules(parm_config.rule_ipv6_name,
 					&route_base_v6, &lpm_parse_v6_rule);
@@ -289,11 +296,7 @@ read_config_files_lpm(void)
 			rte_exit(EXIT_FAILURE, "Failed to add IPv6 rules\n");
 		}
 	} else {
-		RTE_LOG(INFO, L3FWD, "Missing 1 or more rule files, using default instead\n");
-		if (lpm_add_default_v4_rules() < 0) {
-			lpm_free_routes();
-			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
-		}
+		RTE_LOG(INFO, L3FWD, "Missing IPv6 rule file, using default instead\n");
 		if (lpm_add_default_v6_rules() < 0) {
 			lpm_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add default IPv6 rules\n");
-- 
2.25.1

[v2 2/3] examples/l3fwd: fix return value on rules add

[Gagandeep Singh]

fix return value on adding the EM or LPM rules.

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  | 11 ++++++-----
 examples/l3fwd/lpm_route_parse.c | 11 ++++++-----
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/examples/l3fwd/em_route_parse.c b/examples/l3fwd/em_route_parse.c
index da23356dd6..8b534de5f1 100644
--- a/examples/l3fwd/em_route_parse.c
+++ b/examples/l3fwd/em_route_parse.c
@@ -119,7 +119,7 @@ em_add_rules(const char *rule_path,
 	char buff[LINE_MAX];
 	FILE *fh;
 	unsigned int i = 0, rule_size = sizeof(*next);
-	int val;
+	int val, rc;
 
 	*proute_base = NULL;
 	fh = fopen(rule_path, "rb");
@@ -172,13 +172,14 @@ em_add_rules(const char *rule_path,
 			return -EINVAL;
 		}
 
-		if (parser(buff + 1, next) != 0) {
+		rc = parser(buff + 1, next);
+		if (rc != 0) {
 			RTE_LOG(ERR, L3FWD,
-				"%s Line %u: parse rules error\n",
-				rule_path, i);
+				"%s Line %u: parse rules error code = %d\n",
+				rule_path, i, rc);
 			fclose(fh);
 			free(route_rules);
-			return -EINVAL;
+			return rc;
 		}
 
 		route_cnt++;
diff --git a/examples/l3fwd/lpm_route_parse.c b/examples/l3fwd/lpm_route_parse.c
index f7d44aa2cd..f27b66e838 100644
--- a/examples/l3fwd/lpm_route_parse.c
+++ b/examples/l3fwd/lpm_route_parse.c
@@ -184,7 +184,7 @@ lpm_add_rules(const char *rule_path,
 	char buff[LINE_MAX];
 	FILE *fh;
 	unsigned int i = 0, rule_size = sizeof(*next);
-	int val;
+	int val, rc;
 
 	*proute_base = NULL;
 	fh = fopen(rule_path, "rb");
@@ -237,13 +237,14 @@ lpm_add_rules(const char *rule_path,
 			return -EINVAL;
 		}
 
-		if (parser(buff + 1, next) != 0) {
+		rc = parser(buff + 1, next);
+		if (rc != 0) {
 			RTE_LOG(ERR, L3FWD,
-				"%s Line %u: parse rules error\n",
-				rule_path, i);
+				"%s Line %u: parse rules error code = %d\n",
+				rule_path, i, rc);
 			fclose(fh);
 			free(route_rules);
-			return -EINVAL;
+			return rc;
 		}
 
 		route_cnt++;
-- 
2.25.1

[v2 3/3] examples/l3fwd: enhance valid ports checking

[Gagandeep Singh]

The current port ID validation logic in the routes add code has
two issues:

 - It can pass if port ID in route is 31+.
 - It silently skips rules with disabled or invalid
   port IDs

This patch is:
 - Improving the enabled port IDs check logic.
 - Introducing a user option, "exit_on_failure", to control
   the behavior when attempting to add rules for disabled or
   invalid port IDs (either exit or skip)
 - Creating a port ID validation function for use across
   various setup functions

Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
---
 examples/l3fwd/em_route_parse.c |  4 +--
 examples/l3fwd/l3fwd.h          | 16 +++++++++
 examples/l3fwd/l3fwd_em.c       | 22 ++++++++----
 examples/l3fwd/l3fwd_fib.c      | 26 +++++++++-----
 examples/l3fwd/l3fwd_lpm.c      | 26 +++++++++-----
 examples/l3fwd/l3fwd_route.h    |  2 ++
 examples/l3fwd/main.c           | 60 +++++++++++++++++++++++++++------
 7 files changed, 122 insertions(+), 34 deletions(-)

diff --git a/examples/l3fwd/em_route_parse.c b/examples/l3fwd/em_route_parse.c
index 8b534de5f1..b80442d7b8 100644
--- a/examples/l3fwd/em_route_parse.c
+++ b/examples/l3fwd/em_route_parse.c
@@ -10,8 +10,8 @@
 #include "l3fwd.h"
 #include "l3fwd_route.h"
 
-static struct em_rule *em_route_base_v4;
-static struct em_rule *em_route_base_v6;
+struct em_rule *em_route_base_v4;
+struct em_rule *em_route_base_v6;
 
 enum {
 	CB_FLD_DST_ADDR,
diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
index 93ce652d02..d6510448a5 100644
--- a/examples/l3fwd/l3fwd.h
+++ b/examples/l3fwd/l3fwd.h
@@ -56,6 +56,15 @@
 #define L3FWD_HASH_ENTRIES		(1024*1024*1)
 #endif
 
+/* Select Longest-Prefix, Exact match, Forwarding Information Base or Access Control. */
+enum L3FWD_LOOKUP_MODE {
+	L3FWD_LOOKUP_DEFAULT,
+	L3FWD_LOOKUP_LPM,
+	L3FWD_LOOKUP_EM,
+	L3FWD_LOOKUP_FIB,
+	L3FWD_LOOKUP_ACL
+};
+
 struct parm_cfg {
 	const char *rule_ipv4_name;
 	const char *rule_ipv6_name;
@@ -101,6 +110,9 @@ extern struct rte_ether_addr ports_eth_addr[RTE_MAX_ETHPORTS];
 /* mask of enabled ports */
 extern uint32_t enabled_port_mask;
 
+/* Skip or exit on invalid route */
+extern bool exit_on_failure;
+
 /* Used only in exact match mode. */
 extern int ipv6; /**< ipv6 is false by default. */
 extern uint32_t hash_entry_number;
@@ -218,6 +230,10 @@ init_mem(uint16_t portid, unsigned int nb_mbuf);
 int config_port_max_pkt_len(struct rte_eth_conf *conf,
 			    struct rte_eth_dev_info *dev_info);
 
+int
+l3fwd_validate_routes_port(enum L3FWD_LOOKUP_MODE mode, uint32_t i,
+			   bool is_ipv4);
+
 /* Function pointers for ACL, LPM, EM or FIB functionality. */
 void
 setup_acl(const int socketid);
diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c
index 31a7e05e39..1a1b48635b 100644
--- a/examples/l3fwd/l3fwd_em.c
+++ b/examples/l3fwd/l3fwd_em.c
@@ -384,9 +384,14 @@ populate_ipv4_flow_into_table(const struct rte_hash *h)
 		struct in_addr src;
 		struct in_addr dst;
 
-		if ((1 << em_route_base_v4[i].if_out &
-				enabled_port_mask) == 0)
-			continue;
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_EM, i, true);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX: %d: Port ID %d in IPv4 rule is not"
+					 " enabled\n", i, em_route_base_v4[i].if_out);
+			else
+				continue;
+		}
 
 		entry = &em_route_base_v4[i];
 		convert_ipv4_5tuple(&(entry->v4_key), &newkey);
@@ -436,9 +441,14 @@ populate_ipv6_flow_into_table(const struct rte_hash *h)
 		struct em_rule *entry;
 		union ipv6_5tuple_host newkey;
 
-		if ((1 << em_route_base_v6[i].if_out &
-				enabled_port_mask) == 0)
-			continue;
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_EM, i, false);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX %d: Port ID %d given in IPv6 rule is not"
+					 " enabled\n", i, em_route_base_v6[i].if_out);
+			else
+				continue;
+		}
 
 		entry = &em_route_base_v6[i];
 		convert_ipv6_5tuple(&(entry->v6_key), &newkey);
diff --git a/examples/l3fwd/l3fwd_fib.c b/examples/l3fwd/l3fwd_fib.c
index f38b19af3f..9e987e2031 100644
--- a/examples/l3fwd/l3fwd_fib.c
+++ b/examples/l3fwd/l3fwd_fib.c
@@ -670,10 +670,15 @@ setup_fib(const int socketid)
 	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;
+		/* Check for valid ports */
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_FIB, i, true);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX %d: Port ID %d in IPv4 rule is not"
+					 " enabled\n", i, route_base_v4[i].if_out);
+			else
+				continue;
+		}
 
 		rte_eth_dev_info_get(route_base_v4[i].if_out,
 				     &dev_info);
@@ -724,10 +729,15 @@ setup_fib(const int socketid)
 	/* Populate the fib IPv6 table. */
 	for (i = 0; i < route_num_v6; i++) {
 
-		/* Skip unused ports. */
-		if ((1 << route_base_v6[i].if_out &
-				enabled_port_mask) == 0)
-			continue;
+		/* Check for valid ports. */
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_FIB, i, false);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX %d: Port ID %d given in IPv6 rule is not"
+					" enabled\n", i, route_base_v6[i].if_out);
+			else
+				continue;
+		}
 
 		rte_eth_dev_info_get(route_base_v6[i].if_out,
 				     &dev_info);
diff --git a/examples/l3fwd/l3fwd_lpm.c b/examples/l3fwd/l3fwd_lpm.c
index e8fd95aae9..3193e60dc7 100644
--- a/examples/l3fwd/l3fwd_lpm.c
+++ b/examples/l3fwd/l3fwd_lpm.c
@@ -583,10 +583,15 @@ setup_lpm(const int socketid)
 	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;
+		/* Check for valid ports */
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_LPM, i, true);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX: %d: Port ID %d in IPv4 rule is not"
+				 " enabled\n", i, route_base_v4[i].if_out);
+			else
+				continue;
+		}
 
 		rte_eth_dev_info_get(route_base_v4[i].if_out,
 				     &dev_info);
@@ -627,10 +632,15 @@ setup_lpm(const int socketid)
 	/* populate the LPM table */
 	for (i = 0; i < route_num_v6; i++) {
 
-		/* skip unused ports */
-		if ((1 << route_base_v6[i].if_out &
-				enabled_port_mask) == 0)
-			continue;
+		/* Check for valid ports */
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_LPM, i, false);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX %d Port ID %d given in IPv6 rule is not"
+					" enabled\n", i, route_base_v6[i].if_out);
+			else
+				continue;
+		}
 
 		rte_eth_dev_info_get(route_base_v6[i].if_out,
 				     &dev_info);
diff --git a/examples/l3fwd/l3fwd_route.h b/examples/l3fwd/l3fwd_route.h
index 467c4d2859..681d10c6d3 100644
--- a/examples/l3fwd/l3fwd_route.h
+++ b/examples/l3fwd/l3fwd_route.h
@@ -82,6 +82,8 @@ struct em_rule {
 
 extern struct lpm_route_rule *route_base_v4;
 extern struct lpm_route_rule *route_base_v6;
+extern struct em_rule *em_route_base_v4;
+extern struct em_rule *em_route_base_v6;
 extern int route_num_v4;
 extern int route_num_v6;
 
diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 01b763e5ba..3cfe21da90 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -59,14 +59,6 @@ uint16_t nb_txd = TX_DESC_DEFAULT;
 /**< Ports set in promiscuous mode off by default. */
 static int promiscuous_on;
 
-/* Select Longest-Prefix, Exact match, Forwarding Information Base or Access Control. */
-enum L3FWD_LOOKUP_MODE {
-	L3FWD_LOOKUP_DEFAULT,
-	L3FWD_LOOKUP_LPM,
-	L3FWD_LOOKUP_EM,
-	L3FWD_LOOKUP_FIB,
-	L3FWD_LOOKUP_ACL
-};
 static enum L3FWD_LOOKUP_MODE lookup_mode;
 
 /* Global variables. */
@@ -88,7 +80,8 @@ xmm_t val_eth[RTE_MAX_ETHPORTS];
 
 /* mask of enabled ports */
 uint32_t enabled_port_mask;
-
+bool exit_on_failure; /**< Skip the route rule with invalid or */
+			      /**< disabled port ID */
 /* Used only in exact match mode. */
 int ipv6; /**< ipv6 is false by default. */
 
@@ -267,6 +260,43 @@ l3fwd_set_alg(const char *optarg)
 	parm_config.alg = parse_acl_alg(optarg);
 }
 
+/* This function will work only after read_config_files step */
+int
+l3fwd_validate_routes_port(enum L3FWD_LOOKUP_MODE mode, uint32_t i,
+			   bool is_ipv4)
+{
+	uint32_t max_port_count = (sizeof(enabled_port_mask) * CHAR_BIT);
+
+	if (mode == L3FWD_LOOKUP_LPM ||
+			mode == L3FWD_LOOKUP_FIB) {
+		if (is_ipv4) {
+			if (route_base_v4[i].if_out >= max_port_count ||
+				((1 << route_base_v4[i].if_out &
+				  enabled_port_mask) == 0))
+				return -1;
+		} else {
+			if (route_base_v6[i].if_out >= max_port_count ||
+				((1 << route_base_v6[i].if_out &
+				  enabled_port_mask) == 0))
+				return -1;
+		}
+	} else if (mode == L3FWD_LOOKUP_EM) {
+		if (is_ipv4) {
+			if (em_route_base_v4[i].if_out >= max_port_count ||
+				((1 << em_route_base_v4[i].if_out &
+				  enabled_port_mask) == 0))
+				return -1;
+		} else {
+			if (em_route_base_v6[i].if_out >= max_port_count ||
+				((1 << em_route_base_v6[i].if_out &
+				  enabled_port_mask) == 0))
+				return -1;
+		}
+	}
+
+	return 0;
+}
+
 /*
  * Setup lookup methods for forwarding.
  * Currently exact-match, longest-prefix-match and forwarding information
@@ -402,6 +432,7 @@ print_usage(const char *prgname)
 		" [--parse-ptype]"
 		" [--per-port-pool]"
 		" [--mode]"
+		" [--exit-on-failure]"
 #ifdef RTE_LIB_EVENTDEV
 		" [--eventq-sched]"
 		" [--event-vector [--event-vector-size SIZE] [--event-vector-tmo NS]]"
@@ -428,6 +459,8 @@ print_usage(const char *prgname)
 		"  --per-port-pool: Use separate buffer pool per port\n"
 		"  --mode: Packet transfer mode for I/O, poll or eventdev\n"
 		"          Default mode = poll\n"
+		"  --exit-on-failure: Exit on invalid/disabled port ID given in route rule\n"
+		"          Default action is skip the rule\n"
 #ifdef RTE_LIB_EVENTDEV
 		"  --eventq-sched: Event queue synchronization method\n"
 		"                  ordered, atomic or parallel.\n"
@@ -689,6 +722,7 @@ static const char short_options[] =
 #define CMD_LINE_OPT_RELAX_RX_OFFLOAD "relax-rx-offload"
 #define CMD_LINE_OPT_PER_PORT_POOL "per-port-pool"
 #define CMD_LINE_OPT_MODE "mode"
+#define CMD_LINE_OPT_EXIT_ON_FAILURE "exit-on-failure"
 #define CMD_LINE_OPT_EVENTQ_SYNC "eventq-sched"
 #define CMD_LINE_OPT_EVENT_ETH_RX_QUEUES "event-eth-rxqs"
 #define CMD_LINE_OPT_LOOKUP "lookup"
@@ -726,7 +760,8 @@ enum {
 	CMD_LINE_OPT_LOOKUP_NUM,
 	CMD_LINE_OPT_ENABLE_VECTOR_NUM,
 	CMD_LINE_OPT_VECTOR_SIZE_NUM,
-	CMD_LINE_OPT_VECTOR_TMO_NS_NUM
+	CMD_LINE_OPT_VECTOR_TMO_NS_NUM,
+	CMD_LINE_OPT_EXIT_ON_FAILURE_NUM
 };
 
 static const struct option lgopts[] = {
@@ -743,6 +778,7 @@ static const struct option lgopts[] = {
 	{CMD_LINE_OPT_DISABLE_RSS, 0, 0, CMD_LINE_OPT_DISABLE_RSS_NUM},
 	{CMD_LINE_OPT_PER_PORT_POOL, 0, 0, CMD_LINE_OPT_PARSE_PER_PORT_POOL},
 	{CMD_LINE_OPT_MODE, 1, 0, CMD_LINE_OPT_MODE_NUM},
+	{CMD_LINE_OPT_EXIT_ON_FAILURE, 0, 0, CMD_LINE_OPT_EXIT_ON_FAILURE_NUM},
 	{CMD_LINE_OPT_EVENTQ_SYNC, 1, 0, CMD_LINE_OPT_EVENTQ_SYNC_NUM},
 	{CMD_LINE_OPT_EVENT_ETH_RX_QUEUES, 1, 0,
 					CMD_LINE_OPT_EVENT_ETH_RX_QUEUES_NUM},
@@ -885,6 +921,10 @@ parse_args(int argc, char **argv)
 			parse_mode(optarg);
 			break;
 
+		case CMD_LINE_OPT_EXIT_ON_FAILURE_NUM:
+			exit_on_failure = true;
+			break;
+
 #ifdef RTE_LIB_EVENTDEV
 		case CMD_LINE_OPT_EVENTQ_SYNC_NUM:
 			parse_eventq_sched(optarg);
-- 
2.25.1

Re: [v2 3/3] examples/l3fwd: enhance valid ports checking

[Stephen Hemminger]

On Tue,  6 Aug 2024 09:11:20 +0530
Gagandeep Singh <g.singh@nxp.com> wrote:

> The current port ID validation logic in the routes add code has
> two issues:
> 
>  - It can pass if port ID in route is 31+.
>  - It silently skips rules with disabled or invalid
>    port IDs
> 
> This patch is:
>  - Improving the enabled port IDs check logic.
>  - Introducing a user option, "exit_on_failure", to control
>    the behavior when attempting to add rules for disabled or
>    invalid port IDs (either exit or skip)
>  - Creating a port ID validation function for use across
>    various setup functions
> 
> Signed-off-by: Gagandeep Singh <g.singh@nxp.com>

Patch looks fine, but other changes in the intervening time have
caused conflicts with this patch. Needs to be rebased.

Re: [v2 3/3] examples/l3fwd: enhance valid ports checking

[Thomas Monjalon]

13/11/2024 20:39, Stephen Hemminger:
> On Tue,  6 Aug 2024 09:11:20 +0530
> Gagandeep Singh <g.singh@nxp.com> wrote:
> 
> > The current port ID validation logic in the routes add code has
> > two issues:
> > 
> >  - It can pass if port ID in route is 31+.
> >  - It silently skips rules with disabled or invalid
> >    port IDs
> > 
> > This patch is:
> >  - Improving the enabled port IDs check logic.
> >  - Introducing a user option, "exit_on_failure", to control
> >    the behavior when attempting to add rules for disabled or
> >    invalid port IDs (either exit or skip)
> >  - Creating a port ID validation function for use across
> >    various setup functions
> > 
> > Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> 
> Patch looks fine, but other changes in the intervening time have
> caused conflicts with this patch. Needs to be rebased.

Waiting for a rebase please?

[v3 0/3] L3fwd changes

[Gagandeep Singh]

v3 changes:
*  rebased the series to latest commit.

v2 changes:
*  Handled a comment to enhance the invalid port ID logic
   and added a user option to decide exit or silently skip
   in case invalid port in the rules list.

Gagandeep Singh (3):
  examples/l3fwd: support single route file
  examples/l3fwd: fix return value on rules add
  examples/l3fwd: enhance valid ports checking

 examples/l3fwd/em_route_parse.c  | 33 +++++++++---------
 examples/l3fwd/l3fwd.h           | 16 +++++++++
 examples/l3fwd/l3fwd_em.c        | 22 ++++++++----
 examples/l3fwd/l3fwd_fib.c       | 26 +++++++++-----
 examples/l3fwd/l3fwd_lpm.c       | 26 +++++++++-----
 examples/l3fwd/l3fwd_route.h     |  2 ++
 examples/l3fwd/lpm_route_parse.c | 28 ++++++++-------
 examples/l3fwd/main.c            | 58 +++++++++++++++++++++++++++-----
 8 files changed, 153 insertions(+), 58 deletions(-)

-- 
2.25.1

[v3 1/3] examples/l3fwd: support single route file

[Gagandeep Singh]

IPv6 rules file needs to be specified together with IPv4
rules file to configure user given rules. But if user want to
give only IPv4 or only IPv6 rules, application returns error:
"Missing 1 or more rule files"

With this patch application can accept only IPv4,
only IPv6 or both IP rules.

Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
---
 examples/l3fwd/em_route_parse.c  | 18 ++++++++++--------
 examples/l3fwd/lpm_route_parse.c | 17 ++++++++++-------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/examples/l3fwd/em_route_parse.c b/examples/l3fwd/em_route_parse.c
index 6c16832e94..da23356dd6 100644
--- a/examples/l3fwd/em_route_parse.c
+++ b/examples/l3fwd/em_route_parse.c
@@ -249,8 +249,7 @@ void
 read_config_files_em(void)
 {
 	/* ipv4 check */
-	if (parm_config.rule_ipv4_name != NULL &&
-			parm_config.rule_ipv6_name != NULL) {
+	if (parm_config.rule_ipv4_name != NULL) {
 		/* ipv4 check */
 		route_num_v4 = em_add_rules(parm_config.rule_ipv4_name,
 					&em_route_base_v4, &em_parse_v4_rule);
@@ -258,7 +257,14 @@ read_config_files_em(void)
 			em_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add EM IPv4 rules\n");
 		}
-
+	} else {
+		RTE_LOG(INFO, L3FWD, "Missing IPv4 rule file, using default instead\n");
+		if (em_add_default_v4_rules() < 0) {
+			em_free_routes();
+			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
+		}
+	}
+	if (parm_config.rule_ipv6_name != NULL) {
 		/* ipv6 check */
 		route_num_v6 = em_add_rules(parm_config.rule_ipv6_name,
 					&em_route_base_v6, &em_parse_v6_rule);
@@ -267,11 +273,7 @@ read_config_files_em(void)
 			rte_exit(EXIT_FAILURE, "Failed to add EM IPv6 rules\n");
 		}
 	} else {
-		RTE_LOG(INFO, L3FWD, "Missing 1 or more rule files, using default instead\n");
-		if (em_add_default_v4_rules() < 0) {
-			em_free_routes();
-			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
-		}
+		RTE_LOG(INFO, L3FWD, "Missing IPv6 rule file, using default instead\n");
 		if (em_add_default_v6_rules() < 0) {
 			em_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add default IPv6 rules\n");
diff --git a/examples/l3fwd/lpm_route_parse.c b/examples/l3fwd/lpm_route_parse.c
index 9c179dc065..2df1b3e925 100644
--- a/examples/l3fwd/lpm_route_parse.c
+++ b/examples/l3fwd/lpm_route_parse.c
@@ -270,8 +270,7 @@ lpm_free_routes(void)
 void
 read_config_files_lpm(void)
 {
-	if (parm_config.rule_ipv4_name != NULL &&
-			parm_config.rule_ipv6_name != NULL) {
+	if (parm_config.rule_ipv4_name != NULL) {
 		/* ipv4 check */
 		route_num_v4 = lpm_add_rules(parm_config.rule_ipv4_name,
 					&route_base_v4, &lpm_parse_v4_rule);
@@ -279,7 +278,15 @@ read_config_files_lpm(void)
 			lpm_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add IPv4 rules\n");
 		}
+	} else {
+		RTE_LOG(INFO, L3FWD, "Missing IPv4 rule file, using default instead\n");
+		if (lpm_add_default_v4_rules() < 0) {
+			lpm_free_routes();
+			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
+		}
+	}
 
+	if (parm_config.rule_ipv6_name != NULL) {
 		/* ipv6 check */
 		route_num_v6 = lpm_add_rules(parm_config.rule_ipv6_name,
 					&route_base_v6, &lpm_parse_v6_rule);
@@ -288,11 +295,7 @@ read_config_files_lpm(void)
 			rte_exit(EXIT_FAILURE, "Failed to add IPv6 rules\n");
 		}
 	} else {
-		RTE_LOG(INFO, L3FWD, "Missing 1 or more rule files, using default instead\n");
-		if (lpm_add_default_v4_rules() < 0) {
-			lpm_free_routes();
-			rte_exit(EXIT_FAILURE, "Failed to add default IPv4 rules\n");
-		}
+		RTE_LOG(INFO, L3FWD, "Missing IPv6 rule file, using default instead\n");
 		if (lpm_add_default_v6_rules() < 0) {
 			lpm_free_routes();
 			rte_exit(EXIT_FAILURE, "Failed to add default IPv6 rules\n");
-- 
2.25.1

[v3 2/3] examples/l3fwd: fix return value on rules add

[Gagandeep Singh]

fix return value on adding the EM or LPM rules.

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  | 11 ++++++-----
 examples/l3fwd/lpm_route_parse.c | 11 ++++++-----
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/examples/l3fwd/em_route_parse.c b/examples/l3fwd/em_route_parse.c
index da23356dd6..8b534de5f1 100644
--- a/examples/l3fwd/em_route_parse.c
+++ b/examples/l3fwd/em_route_parse.c
@@ -119,7 +119,7 @@ em_add_rules(const char *rule_path,
 	char buff[LINE_MAX];
 	FILE *fh;
 	unsigned int i = 0, rule_size = sizeof(*next);
-	int val;
+	int val, rc;
 
 	*proute_base = NULL;
 	fh = fopen(rule_path, "rb");
@@ -172,13 +172,14 @@ em_add_rules(const char *rule_path,
 			return -EINVAL;
 		}
 
-		if (parser(buff + 1, next) != 0) {
+		rc = parser(buff + 1, next);
+		if (rc != 0) {
 			RTE_LOG(ERR, L3FWD,
-				"%s Line %u: parse rules error\n",
-				rule_path, i);
+				"%s Line %u: parse rules error code = %d\n",
+				rule_path, i, rc);
 			fclose(fh);
 			free(route_rules);
-			return -EINVAL;
+			return rc;
 		}
 
 		route_cnt++;
diff --git a/examples/l3fwd/lpm_route_parse.c b/examples/l3fwd/lpm_route_parse.c
index 2df1b3e925..09f6b1951b 100644
--- a/examples/l3fwd/lpm_route_parse.c
+++ b/examples/l3fwd/lpm_route_parse.c
@@ -183,7 +183,7 @@ lpm_add_rules(const char *rule_path,
 	char buff[LINE_MAX];
 	FILE *fh;
 	unsigned int i = 0, rule_size = sizeof(*next);
-	int val;
+	int val, rc;
 
 	*proute_base = NULL;
 	fh = fopen(rule_path, "rb");
@@ -236,13 +236,14 @@ lpm_add_rules(const char *rule_path,
 			return -EINVAL;
 		}
 
-		if (parser(buff + 1, next) != 0) {
+		rc = parser(buff + 1, next);
+		if (rc != 0) {
 			RTE_LOG(ERR, L3FWD,
-				"%s Line %u: parse rules error\n",
-				rule_path, i);
+				"%s Line %u: parse rules error code = %d\n",
+				rule_path, i, rc);
 			fclose(fh);
 			free(route_rules);
-			return -EINVAL;
+			return rc;
 		}
 
 		route_cnt++;
-- 
2.25.1

[v3 3/3] examples/l3fwd: enhance valid ports checking

[Gagandeep Singh]

The current port ID validation logic in the routes add code has
two issues:

 - It can pass if port ID in route is 31+.
 - It silently skips rules with disabled or invalid
   port IDs

This patch is:
 - Improving the enabled port IDs check logic.
 - Introducing a user option, "exit_on_failure", to control
   the behavior when attempting to add rules for disabled or
   invalid port IDs (either exit or skip)
 - Creating a port ID validation function for use across
   various setup functions

Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
---
 examples/l3fwd/em_route_parse.c |  4 +--
 examples/l3fwd/l3fwd.h          | 16 +++++++++
 examples/l3fwd/l3fwd_em.c       | 22 +++++++++----
 examples/l3fwd/l3fwd_fib.c      | 26 ++++++++++-----
 examples/l3fwd/l3fwd_lpm.c      | 26 ++++++++++-----
 examples/l3fwd/l3fwd_route.h    |  2 ++
 examples/l3fwd/main.c           | 58 ++++++++++++++++++++++++++++-----
 7 files changed, 121 insertions(+), 33 deletions(-)

diff --git a/examples/l3fwd/em_route_parse.c b/examples/l3fwd/em_route_parse.c
index 8b534de5f1..b80442d7b8 100644
--- a/examples/l3fwd/em_route_parse.c
+++ b/examples/l3fwd/em_route_parse.c
@@ -10,8 +10,8 @@
 #include "l3fwd.h"
 #include "l3fwd_route.h"
 
-static struct em_rule *em_route_base_v4;
-static struct em_rule *em_route_base_v6;
+struct em_rule *em_route_base_v4;
+struct em_rule *em_route_base_v6;
 
 enum {
 	CB_FLD_DST_ADDR,
diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
index 0cce3406ee..bcbaf4d143 100644
--- a/examples/l3fwd/l3fwd.h
+++ b/examples/l3fwd/l3fwd.h
@@ -57,6 +57,15 @@
 #define L3FWD_HASH_ENTRIES		(1024*1024*1)
 #endif
 
+/* Select Longest-Prefix, Exact match, Forwarding Information Base or Access Control. */
+enum L3FWD_LOOKUP_MODE {
+	L3FWD_LOOKUP_DEFAULT,
+	L3FWD_LOOKUP_LPM,
+	L3FWD_LOOKUP_EM,
+	L3FWD_LOOKUP_FIB,
+	L3FWD_LOOKUP_ACL
+};
+
 struct parm_cfg {
 	const char *rule_ipv4_name;
 	const char *rule_ipv6_name;
@@ -102,6 +111,9 @@ extern struct rte_ether_addr ports_eth_addr[RTE_MAX_ETHPORTS];
 /* mask of enabled ports */
 extern uint32_t enabled_port_mask;
 
+/* Skip or exit on invalid route */
+extern bool exit_on_failure;
+
 /* Used only in exact match mode. */
 extern int ipv6; /**< ipv6 is false by default. */
 extern uint32_t hash_entry_number;
@@ -222,6 +234,10 @@ init_mem(uint16_t portid, unsigned int nb_mbuf);
 int config_port_max_pkt_len(struct rte_eth_conf *conf,
 			    struct rte_eth_dev_info *dev_info);
 
+int
+l3fwd_validate_routes_port(enum L3FWD_LOOKUP_MODE mode, uint32_t i,
+			   bool is_ipv4);
+
 /* Function pointers for ACL, LPM, EM or FIB functionality. */
 void
 setup_acl(const int socketid);
diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c
index da9c45e3a4..9b482c3c4e 100644
--- a/examples/l3fwd/l3fwd_em.c
+++ b/examples/l3fwd/l3fwd_em.c
@@ -384,9 +384,14 @@ populate_ipv4_flow_into_table(const struct rte_hash *h)
 		struct in_addr src;
 		struct in_addr dst;
 
-		if ((1 << em_route_base_v4[i].if_out &
-				enabled_port_mask) == 0)
-			continue;
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_EM, i, true);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX: %d: Port ID %d in IPv4 rule is not"
+					 " enabled\n", i, em_route_base_v4[i].if_out);
+			else
+				continue;
+		}
 
 		entry = &em_route_base_v4[i];
 		convert_ipv4_5tuple(&(entry->v4_key), &newkey);
@@ -436,9 +441,14 @@ populate_ipv6_flow_into_table(const struct rte_hash *h)
 		struct em_rule *entry;
 		union ipv6_5tuple_host newkey;
 
-		if ((1 << em_route_base_v6[i].if_out &
-				enabled_port_mask) == 0)
-			continue;
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_EM, i, false);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX %d: Port ID %d given in IPv6 rule is not"
+					 " enabled\n", i, em_route_base_v6[i].if_out);
+			else
+				continue;
+		}
 
 		entry = &em_route_base_v6[i];
 		convert_ipv6_5tuple(&(entry->v6_key), &newkey);
diff --git a/examples/l3fwd/l3fwd_fib.c b/examples/l3fwd/l3fwd_fib.c
index 82f1739df7..fa399d8c25 100644
--- a/examples/l3fwd/l3fwd_fib.c
+++ b/examples/l3fwd/l3fwd_fib.c
@@ -667,10 +667,15 @@ setup_fib(const int socketid)
 	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;
+		/* Check for valid ports */
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_FIB, i, true);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX %d: Port ID %d in IPv4 rule is not"
+					 " enabled\n", i, route_base_v4[i].if_out);
+			else
+				continue;
+		}
 
 		ret = rte_eth_dev_info_get(route_base_v4[i].if_out, &dev_info);
 		if (ret < 0)
@@ -725,10 +730,15 @@ setup_fib(const int socketid)
 	/* Populate the fib IPv6 table. */
 	for (i = 0; i < route_num_v6; i++) {
 
-		/* Skip unused ports. */
-		if ((1 << route_base_v6[i].if_out &
-				enabled_port_mask) == 0)
-			continue;
+		/* Check for valid ports. */
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_FIB, i, false);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX %d: Port ID %d given in IPv6 rule is not"
+					" enabled\n", i, route_base_v6[i].if_out);
+			else
+				continue;
+		}
 
 		ret = rte_eth_dev_info_get(route_base_v6[i].if_out, &dev_info);
 		if (ret < 0)
diff --git a/examples/l3fwd/l3fwd_lpm.c b/examples/l3fwd/l3fwd_lpm.c
index fec0aeb79c..e3bf2007db 100644
--- a/examples/l3fwd/l3fwd_lpm.c
+++ b/examples/l3fwd/l3fwd_lpm.c
@@ -583,10 +583,15 @@ setup_lpm(const int socketid)
 	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;
+		/* Check for valid ports */
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_LPM, i, true);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX: %d: Port ID %d in IPv4 rule is not"
+				 " enabled\n", i, route_base_v4[i].if_out);
+			else
+				continue;
+		}
 
 		ret = rte_eth_dev_info_get(route_base_v4[i].if_out, &dev_info);
 		if (ret < 0)
@@ -630,10 +635,15 @@ setup_lpm(const int socketid)
 	/* populate the LPM table */
 	for (i = 0; i < route_num_v6; i++) {
 
-		/* skip unused ports */
-		if ((1 << route_base_v6[i].if_out &
-				enabled_port_mask) == 0)
-			continue;
+		/* Check for valid ports */
+		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_LPM, i, false);
+		if (ret) {
+			if (exit_on_failure)
+				rte_exit(EXIT_FAILURE, "IDX %d Port ID %d given in IPv6 rule is not"
+					" enabled\n", i, route_base_v6[i].if_out);
+			else
+				continue;
+		}
 
 		ret = rte_eth_dev_info_get(route_base_v6[i].if_out, &dev_info);
 		if (ret < 0)
diff --git a/examples/l3fwd/l3fwd_route.h b/examples/l3fwd/l3fwd_route.h
index 62263c3540..98254deeeb 100644
--- a/examples/l3fwd/l3fwd_route.h
+++ b/examples/l3fwd/l3fwd_route.h
@@ -81,6 +81,8 @@ struct em_rule {
 
 extern struct lpm_route_rule *route_base_v4;
 extern struct lpm_route_rule *route_base_v6;
+extern struct em_rule *em_route_base_v4;
+extern struct em_rule *em_route_base_v6;
 extern int route_num_v4;
 extern int route_num_v6;
 
diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 994b7dd8e5..b3b5be4076 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -63,14 +63,6 @@ uint32_t mb_mempool_cache_size = MEMPOOL_CACHE_SIZE;
 /**< Ports set in promiscuous mode off by default. */
 static int promiscuous_on;
 
-/* Select Longest-Prefix, Exact match, Forwarding Information Base or Access Control. */
-enum L3FWD_LOOKUP_MODE {
-	L3FWD_LOOKUP_DEFAULT,
-	L3FWD_LOOKUP_LPM,
-	L3FWD_LOOKUP_EM,
-	L3FWD_LOOKUP_FIB,
-	L3FWD_LOOKUP_ACL
-};
 static enum L3FWD_LOOKUP_MODE lookup_mode;
 
 /* Global variables. */
@@ -92,7 +84,8 @@ xmm_t val_eth[RTE_MAX_ETHPORTS];
 
 /* mask of enabled ports */
 uint32_t enabled_port_mask;
-
+bool exit_on_failure; /**< Skip the route rule with invalid or */
+			      /**< disabled port ID */
 /* Used only in exact match mode. */
 int ipv6; /**< ipv6 is false by default. */
 
@@ -271,6 +264,43 @@ l3fwd_set_alg(const char *optarg)
 	parm_config.alg = parse_acl_alg(optarg);
 }
 
+/* This function will work only after read_config_files step */
+int
+l3fwd_validate_routes_port(enum L3FWD_LOOKUP_MODE mode, uint32_t i,
+			   bool is_ipv4)
+{
+	uint32_t max_port_count = (sizeof(enabled_port_mask) * CHAR_BIT);
+
+	if (mode == L3FWD_LOOKUP_LPM ||
+			mode == L3FWD_LOOKUP_FIB) {
+		if (is_ipv4) {
+			if (route_base_v4[i].if_out >= max_port_count ||
+				((1 << route_base_v4[i].if_out &
+				  enabled_port_mask) == 0))
+				return -1;
+		} else {
+			if (route_base_v6[i].if_out >= max_port_count ||
+				((1 << route_base_v6[i].if_out &
+				  enabled_port_mask) == 0))
+				return -1;
+		}
+	} else if (mode == L3FWD_LOOKUP_EM) {
+		if (is_ipv4) {
+			if (em_route_base_v4[i].if_out >= max_port_count ||
+				((1 << em_route_base_v4[i].if_out &
+				  enabled_port_mask) == 0))
+				return -1;
+		} else {
+			if (em_route_base_v6[i].if_out >= max_port_count ||
+				((1 << em_route_base_v6[i].if_out &
+				  enabled_port_mask) == 0))
+				return -1;
+		}
+	}
+
+	return 0;
+}
+
 /*
  * Setup lookup methods for forwarding.
  * Currently exact-match, longest-prefix-match and forwarding information
@@ -408,6 +438,7 @@ print_usage(const char *prgname)
 		" [--parse-ptype]"
 		" [--per-port-pool]"
 		" [--mode]"
+		" [--exit-on-failure]"
 #ifdef RTE_LIB_EVENTDEV
 		" [--eventq-sched]"
 		" [--event-vector [--event-vector-size SIZE] [--event-vector-tmo NS]]"
@@ -438,6 +469,8 @@ print_usage(const char *prgname)
 		"  --per-port-pool: Use separate buffer pool per port\n"
 		"  --mode: Packet transfer mode for I/O, poll or eventdev\n"
 		"          Default mode = poll\n"
+		"  --exit-on-failure: Exit on invalid/disabled port ID given in route rule\n"
+		"          Default action is skip the rule\n"
 #ifdef RTE_LIB_EVENTDEV
 		"  --eventq-sched: Event queue synchronization method\n"
 		"                  ordered, atomic or parallel.\n"
@@ -758,6 +791,7 @@ static const char short_options[] =
 #define CMD_LINE_OPT_RELAX_RX_OFFLOAD "relax-rx-offload"
 #define CMD_LINE_OPT_PER_PORT_POOL "per-port-pool"
 #define CMD_LINE_OPT_MODE "mode"
+#define CMD_LINE_OPT_EXIT_ON_FAILURE "exit-on-failure"
 #define CMD_LINE_OPT_EVENTQ_SYNC "eventq-sched"
 #define CMD_LINE_OPT_EVENT_ETH_RX_QUEUES "event-eth-rxqs"
 #define CMD_LINE_OPT_LOOKUP "lookup"
@@ -800,6 +834,7 @@ enum {
 	CMD_LINE_OPT_VECTOR_TMO_NS_NUM,
 	CMD_LINE_OPT_PKT_BURST_NUM,
 	CMD_LINE_OPT_MB_CACHE_SIZE_NUM,
+	CMD_LINE_OPT_EXIT_ON_FAILURE_NUM
 };
 
 static const struct option lgopts[] = {
@@ -816,6 +851,7 @@ static const struct option lgopts[] = {
 	{CMD_LINE_OPT_DISABLE_RSS, 0, 0, CMD_LINE_OPT_DISABLE_RSS_NUM},
 	{CMD_LINE_OPT_PER_PORT_POOL, 0, 0, CMD_LINE_OPT_PARSE_PER_PORT_POOL},
 	{CMD_LINE_OPT_MODE, 1, 0, CMD_LINE_OPT_MODE_NUM},
+	{CMD_LINE_OPT_EXIT_ON_FAILURE, 0, 0, CMD_LINE_OPT_EXIT_ON_FAILURE_NUM},
 	{CMD_LINE_OPT_EVENTQ_SYNC, 1, 0, CMD_LINE_OPT_EVENTQ_SYNC_NUM},
 	{CMD_LINE_OPT_EVENT_ETH_RX_QUEUES, 1, 0,
 					CMD_LINE_OPT_EVENT_ETH_RX_QUEUES_NUM},
@@ -968,6 +1004,10 @@ parse_args(int argc, char **argv)
 			parse_mode(optarg);
 			break;
 
+		case CMD_LINE_OPT_EXIT_ON_FAILURE_NUM:
+			exit_on_failure = true;
+			break;
+
 #ifdef RTE_LIB_EVENTDEV
 		case CMD_LINE_OPT_EVENTQ_SYNC_NUM:
 			parse_eventq_sched(optarg);
-- 
2.25.1

RE: [v2 3/3] examples/l3fwd: enhance valid ports checking

[Gagandeep Singh]

Hi,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, November 19, 2024 10:43 PM
> To: Gagandeep Singh <G.Singh@nxp.com>
> Cc: dev@dpdk.org; Stephen Hemminger <stephen@networkplumber.org>
> Subject: Re: [v2 3/3] examples/l3fwd: enhance valid ports checking
> 
> 13/11/2024 20:39, Stephen Hemminger:
> > On Tue,  6 Aug 2024 09:11:20 +0530
> > Gagandeep Singh <g.singh@nxp.com> wrote:
> >
> > > The current port ID validation logic in the routes add code has two
> > > issues:
> > >
> > >  - It can pass if port ID in route is 31+.
> > >  - It silently skips rules with disabled or invalid
> > >    port IDs
> > >
> > > This patch is:
> > >  - Improving the enabled port IDs check logic.
> > >  - Introducing a user option, "exit_on_failure", to control
> > >    the behavior when attempting to add rules for disabled or
> > >    invalid port IDs (either exit or skip)
> > >  - Creating a port ID validation function for use across
> > >    various setup functions
> > >
> > > Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> >
> > Patch looks fine, but other changes in the intervening time have
> > caused conflicts with this patch. Needs to be rebased.
> 
> Waiting for a rebase please?
> 
I have sent  the rebase series as v3.

RE: [v3 3/3] examples/l3fwd: enhance valid ports checking

[Konstantin Ananyev]

> The current port ID validation logic in the routes add code has
> two issues:
> 
>  - It can pass if port ID in route is 31+.
>  - It silently skips rules with disabled or invalid
>    port IDs
> 
> This patch is:
>  - Improving the enabled port IDs check logic.
>  - Introducing a user option, "exit_on_failure", to control
>    the behavior when attempting to add rules for disabled or
>    invalid port IDs (either exit or skip)
>  - Creating a port ID validation function for use across
>    various setup functions
> 
> Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> ---
>  examples/l3fwd/em_route_parse.c |  4 +--
>  examples/l3fwd/l3fwd.h          | 16 +++++++++
>  examples/l3fwd/l3fwd_em.c       | 22 +++++++++----
>  examples/l3fwd/l3fwd_fib.c      | 26 ++++++++++-----
>  examples/l3fwd/l3fwd_lpm.c      | 26 ++++++++++-----
>  examples/l3fwd/l3fwd_route.h    |  2 ++
>  examples/l3fwd/main.c           | 58 ++++++++++++++++++++++++++++-----
>  7 files changed, 121 insertions(+), 33 deletions(-)
> 
> diff --git a/examples/l3fwd/em_route_parse.c b/examples/l3fwd/em_route_parse.c
> index 8b534de5f1..b80442d7b8 100644
> --- a/examples/l3fwd/em_route_parse.c
> +++ b/examples/l3fwd/em_route_parse.c
> @@ -10,8 +10,8 @@
>  #include "l3fwd.h"
>  #include "l3fwd_route.h"
> 
> -static struct em_rule *em_route_base_v4;
> -static struct em_rule *em_route_base_v6;
> +struct em_rule *em_route_base_v4;
> +struct em_rule *em_route_base_v6;
> 
>  enum {
>  	CB_FLD_DST_ADDR,
> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
> index 0cce3406ee..bcbaf4d143 100644
> --- a/examples/l3fwd/l3fwd.h
> +++ b/examples/l3fwd/l3fwd.h
> @@ -57,6 +57,15 @@
>  #define L3FWD_HASH_ENTRIES		(1024*1024*1)
>  #endif
> 
> +/* Select Longest-Prefix, Exact match, Forwarding Information Base or Access Control. */
> +enum L3FWD_LOOKUP_MODE {
> +	L3FWD_LOOKUP_DEFAULT,
> +	L3FWD_LOOKUP_LPM,
> +	L3FWD_LOOKUP_EM,
> +	L3FWD_LOOKUP_FIB,
> +	L3FWD_LOOKUP_ACL
> +};
> +
>  struct parm_cfg {
>  	const char *rule_ipv4_name;
>  	const char *rule_ipv6_name;
> @@ -102,6 +111,9 @@ extern struct rte_ether_addr ports_eth_addr[RTE_MAX_ETHPORTS];
>  /* mask of enabled ports */
>  extern uint32_t enabled_port_mask;
> 
> +/* Skip or exit on invalid route */
> +extern bool exit_on_failure;
> +
>  /* Used only in exact match mode. */
>  extern int ipv6; /**< ipv6 is false by default. */
>  extern uint32_t hash_entry_number;
> @@ -222,6 +234,10 @@ init_mem(uint16_t portid, unsigned int nb_mbuf);
>  int config_port_max_pkt_len(struct rte_eth_conf *conf,
>  			    struct rte_eth_dev_info *dev_info);
> 
> +int
> +l3fwd_validate_routes_port(enum L3FWD_LOOKUP_MODE mode, uint32_t i,
> +			   bool is_ipv4);
> +
>  /* Function pointers for ACL, LPM, EM or FIB functionality. */
>  void
>  setup_acl(const int socketid);
> diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c
> index da9c45e3a4..9b482c3c4e 100644
> --- a/examples/l3fwd/l3fwd_em.c
> +++ b/examples/l3fwd/l3fwd_em.c
> @@ -384,9 +384,14 @@ populate_ipv4_flow_into_table(const struct rte_hash *h)
>  		struct in_addr src;
>  		struct in_addr dst;
> 
> -		if ((1 << em_route_base_v4[i].if_out &
> -				enabled_port_mask) == 0)
> -			continue;
> +		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_EM, i, true);
> +		if (ret) {
> +			if (exit_on_failure)
> +				rte_exit(EXIT_FAILURE, "IDX: %d: Port ID %d in IPv4 rule is not"
> +					 " enabled\n", i, em_route_base_v4[i].if_out);
> +			else
> +				continue;
> +		}
> 
>  		entry = &em_route_base_v4[i];
>  		convert_ipv4_5tuple(&(entry->v4_key), &newkey);
> @@ -436,9 +441,14 @@ populate_ipv6_flow_into_table(const struct rte_hash *h)
>  		struct em_rule *entry;
>  		union ipv6_5tuple_host newkey;
> 
> -		if ((1 << em_route_base_v6[i].if_out &
> -				enabled_port_mask) == 0)
> -			continue;
> +		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_EM, i, false);
> +		if (ret) {
> +			if (exit_on_failure)
> +				rte_exit(EXIT_FAILURE, "IDX %d: Port ID %d given in IPv6 rule is not"
> +					 " enabled\n", i, em_route_base_v6[i].if_out);
> +			else
> +				continue;
> +		}
> 
>  		entry = &em_route_base_v6[i];
>  		convert_ipv6_5tuple(&(entry->v6_key), &newkey);
> diff --git a/examples/l3fwd/l3fwd_fib.c b/examples/l3fwd/l3fwd_fib.c
> index 82f1739df7..fa399d8c25 100644
> --- a/examples/l3fwd/l3fwd_fib.c
> +++ b/examples/l3fwd/l3fwd_fib.c
> @@ -667,10 +667,15 @@ setup_fib(const int socketid)
>  	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;
> +		/* Check for valid ports */
> +		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_FIB, i, true);
> +		if (ret) {
> +			if (exit_on_failure)
> +				rte_exit(EXIT_FAILURE, "IDX %d: Port ID %d in IPv4 rule is not"
> +					 " enabled\n", i, route_base_v4[i].if_out);
> +			else
> +				continue;
> +		}
> 
>  		ret = rte_eth_dev_info_get(route_base_v4[i].if_out, &dev_info);
>  		if (ret < 0)
> @@ -725,10 +730,15 @@ setup_fib(const int socketid)
>  	/* Populate the fib IPv6 table. */
>  	for (i = 0; i < route_num_v6; i++) {
> 
> -		/* Skip unused ports. */
> -		if ((1 << route_base_v6[i].if_out &
> -				enabled_port_mask) == 0)
> -			continue;
> +		/* Check for valid ports. */
> +		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_FIB, i, false);
> +		if (ret) {
> +			if (exit_on_failure)
> +				rte_exit(EXIT_FAILURE, "IDX %d: Port ID %d given in IPv6 rule is not"
> +					" enabled\n", i, route_base_v6[i].if_out);
> +			else
> +				continue;
> +		}
> 
>  		ret = rte_eth_dev_info_get(route_base_v6[i].if_out, &dev_info);
>  		if (ret < 0)
> diff --git a/examples/l3fwd/l3fwd_lpm.c b/examples/l3fwd/l3fwd_lpm.c
> index fec0aeb79c..e3bf2007db 100644
> --- a/examples/l3fwd/l3fwd_lpm.c
> +++ b/examples/l3fwd/l3fwd_lpm.c
> @@ -583,10 +583,15 @@ setup_lpm(const int socketid)
>  	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;
> +		/* Check for valid ports */
> +		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_LPM, i, true);
> +		if (ret) {
> +			if (exit_on_failure)
> +				rte_exit(EXIT_FAILURE, "IDX: %d: Port ID %d in IPv4 rule is not"
> +				 " enabled\n", i, route_base_v4[i].if_out);
> +			else
> +				continue;
> +		}
> 
>  		ret = rte_eth_dev_info_get(route_base_v4[i].if_out, &dev_info);
>  		if (ret < 0)
> @@ -630,10 +635,15 @@ setup_lpm(const int socketid)
>  	/* populate the LPM table */
>  	for (i = 0; i < route_num_v6; i++) {
> 
> -		/* skip unused ports */
> -		if ((1 << route_base_v6[i].if_out &
> -				enabled_port_mask) == 0)
> -			continue;
> +		/* Check for valid ports */
> +		ret = l3fwd_validate_routes_port(L3FWD_LOOKUP_LPM, i, false);
> +		if (ret) {
> +			if (exit_on_failure)
> +				rte_exit(EXIT_FAILURE, "IDX %d Port ID %d given in IPv6 rule is not"
> +					" enabled\n", i, route_base_v6[i].if_out);
> +			else
> +				continue;
> +		}
> 
>  		ret = rte_eth_dev_info_get(route_base_v6[i].if_out, &dev_info);
>  		if (ret < 0)
> diff --git a/examples/l3fwd/l3fwd_route.h b/examples/l3fwd/l3fwd_route.h
> index 62263c3540..98254deeeb 100644
> --- a/examples/l3fwd/l3fwd_route.h
> +++ b/examples/l3fwd/l3fwd_route.h
> @@ -81,6 +81,8 @@ struct em_rule {
> 
>  extern struct lpm_route_rule *route_base_v4;
>  extern struct lpm_route_rule *route_base_v6;
> +extern struct em_rule *em_route_base_v4;
> +extern struct em_rule *em_route_base_v6;
>  extern int route_num_v4;
>  extern int route_num_v6;
> 
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 994b7dd8e5..b3b5be4076 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -63,14 +63,6 @@ uint32_t mb_mempool_cache_size = MEMPOOL_CACHE_SIZE;
>  /**< Ports set in promiscuous mode off by default. */
>  static int promiscuous_on;
> 
> -/* Select Longest-Prefix, Exact match, Forwarding Information Base or Access Control. */
> -enum L3FWD_LOOKUP_MODE {
> -	L3FWD_LOOKUP_DEFAULT,
> -	L3FWD_LOOKUP_LPM,
> -	L3FWD_LOOKUP_EM,
> -	L3FWD_LOOKUP_FIB,
> -	L3FWD_LOOKUP_ACL
> -};
>  static enum L3FWD_LOOKUP_MODE lookup_mode;
> 
>  /* Global variables. */
> @@ -92,7 +84,8 @@ xmm_t val_eth[RTE_MAX_ETHPORTS];
> 
>  /* mask of enabled ports */
>  uint32_t enabled_port_mask;
> -
> +bool exit_on_failure; /**< Skip the route rule with invalid or */
> +			      /**< disabled port ID */
>  /* Used only in exact match mode. */
>  int ipv6; /**< ipv6 is false by default. */
> 
> @@ -271,6 +264,43 @@ l3fwd_set_alg(const char *optarg)
>  	parm_config.alg = parse_acl_alg(optarg);
>  }
> 
> +/* This function will work only after read_config_files step */
> +int
> +l3fwd_validate_routes_port(enum L3FWD_LOOKUP_MODE mode, uint32_t i,
> +			   bool is_ipv4)
> +{
> +	uint32_t max_port_count = (sizeof(enabled_port_mask) * CHAR_BIT);
> +
> +	if (mode == L3FWD_LOOKUP_LPM ||
> +			mode == L3FWD_LOOKUP_FIB) {
> +		if (is_ipv4) {
> +			if (route_base_v4[i].if_out >= max_port_count ||
> +				((1 << route_base_v4[i].if_out &
> +				  enabled_port_mask) == 0))
> +				return -1;
> +		} else {
> +			if (route_base_v6[i].if_out >= max_port_count ||
> +				((1 << route_base_v6[i].if_out &
> +				  enabled_port_mask) == 0))
> +				return -1;
> +		}
> +	} else if (mode == L3FWD_LOOKUP_EM) {
> +		if (is_ipv4) {
> +			if (em_route_base_v4[i].if_out >= max_port_count ||
> +				((1 << em_route_base_v4[i].if_out &
> +				  enabled_port_mask) == 0))
> +				return -1;
> +		} else {
> +			if (em_route_base_v6[i].if_out >= max_port_count ||
> +				((1 << em_route_base_v6[i].if_out &
> +				  enabled_port_mask) == 0))
> +				return -1;
> +		}
> +	}

Stupid q - why we don't have any checks for ACL mode? 

> +	return 0;
> +}