[dpdk-dev] [PATCH] examples/exception_path: fix shift operation in lcore setup

[dpdk-dev] [PATCH] examples/exception_path: fix shift operation in lcore setup

[Daniel Mrzyglod]

The operaton may have an undefined behavior or yield to an unexpected result.
A bit shift operation has a shift amount which is too large or has a negative value.

Coverity issue: 30688
Fixes: ea977ff1cb0b ("examples/exception_path: fix shift operation in lcore setup")
The previous patch forget to fix values also for input_cores_mask

Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
 examples/exception_path/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/exception_path/main.c b/examples/exception_path/main.c
index e5eedcc..88e7708 100644
--- a/examples/exception_path/main.c
+++ b/examples/exception_path/main.c
@@ -341,7 +341,7 @@ setup_port_lcore_affinities(void)
 
 	/* Setup port_ids[] array, and check masks were ok */
 	RTE_LCORE_FOREACH(i) {
-		if (input_cores_mask & (1ULL << i)) {
+		if (input_cores_mask & (1ULL << (i & 0x3f))) {
 			/* Skip ports that are not enabled */
 			while ((ports_mask & (1 << rx_port)) == 0) {
 				rx_port++;
-- 
2.7.4

Re: [dpdk-dev] [PATCH] examples/exception_path: fix shift operation in lcore setup

[Ferruh Yigit]

On 8/3/2016 12:44 PM, Daniel Mrzyglod wrote:
> The operaton may have an undefined behavior or yield to an unexpected result.
> A bit shift operation has a shift amount which is too large or has a negative value.
> 
> Coverity issue: 30688
> Fixes: ea977ff1cb0b ("examples/exception_path: fix shift operation in lcore setup")
> The previous patch forget to fix values also for input_cores_mask
> 
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> ---
>  examples/exception_path/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/exception_path/main.c b/examples/exception_path/main.c
> index e5eedcc..88e7708 100644
> --- a/examples/exception_path/main.c
> +++ b/examples/exception_path/main.c
> @@ -341,7 +341,7 @@ setup_port_lcore_affinities(void)
>  
>  	/* Setup port_ids[] array, and check masks were ok */
>  	RTE_LCORE_FOREACH(i) {
> -		if (input_cores_mask & (1ULL << i)) {
> +		if (input_cores_mask & (1ULL << (i & 0x3f))) {

I guess 0x3f is because "unsigned long long" is 64bits long, not sure if
we should hardcode this assumption. ULL can be >= 64bits.

RTE_LCORE_FOREACH(i) already makes sure "i" < RTE_MAX_CORE, and
RTE_MAX_CORE is 128 with current default config. So if user provides a
core value > 64, it is valid but will be ignored because of this check.

Another thing is "input_cores_mask" is also 64bits long, so even this
fixed application will not able to use this setting correctly.

I think it is good to
a) add flexible variable size set_bit/clear_bit/test_bit functions, like
Linux ones
b) make "input_cores_mask" an array that is large enough to keep
RTE_MAX_CORE

Although not sure if that is too much effort for this fix.

>  			/* Skip ports that are not enabled */
>  			while ((ports_mask & (1 << rx_port)) == 0) {
>  				rx_port++;
> 

[dpdk-dev] [PATCH v2] examples/exception_path: fix shift operation in lcore setup

[Daniel Mrzyglod]

The operaton may have an undefined behavior or yield to an unexpected result.
A bit shift operation has a shift amount which is too large or has a negative value.

As was mentioned in mailing list core list was limited to 64 so i changed bitmask
to core array

Coverity issue: 30688
Fixes: ea977ff1cb0b ("examples/exception_path: fix shift operation in lcore setup")

Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
---
 examples/exception_path/main.c | 74 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 61 insertions(+), 13 deletions(-)

diff --git a/examples/exception_path/main.c b/examples/exception_path/main.c
index e5eedcc..1493338 100644
--- a/examples/exception_path/main.c
+++ b/examples/exception_path/main.c
@@ -128,11 +128,11 @@ static struct rte_mempool * pktmbuf_pool = NULL;
 /* Mask of enabled ports */
 static uint32_t ports_mask = 0;
 
-/* Mask of cores that read from NIC and write to tap */
-static uint64_t input_cores_mask = 0;
+/* Table of cores that read from NIC and write to tap */
+static uint8_t input_cores_table[RTE_MAX_LCORE];
 
-/* Mask of cores that read from tap and write to NIC */
-static uint64_t output_cores_mask = 0;
+/* Table of cores that read from tap and write to NIC */
+static uint8_t output_cores_table[RTE_MAX_LCORE];
 
 /* Array storing port_id that is associated with each lcore */
 static uint8_t port_ids[RTE_MAX_LCORE];
@@ -224,7 +224,7 @@ main_loop(__attribute__((unused)) void *arg)
 	char tap_name[IFNAMSIZ];
 	int tap_fd;
 
-	if ((1ULL << lcore_id) & input_cores_mask) {
+	if (input_cores_table[lcore_id]) {
 		/* Create new tap interface */
 		snprintf(tap_name, IFNAMSIZ, "tap_dpdk_%.2u", lcore_id);
 		tap_fd = tap_create(tap_name);
@@ -257,7 +257,7 @@ main_loop(__attribute__((unused)) void *arg)
 			}
 		}
 	}
-	else if ((1ULL << lcore_id) & output_cores_mask) {
+	else if (output_cores_table[lcore_id]) {
 		/* Create new tap interface */
 		snprintf(tap_name, IFNAMSIZ, "tap_dpdk_%.2u", lcore_id);
 		tap_fd = tap_create(tap_name);
@@ -341,7 +341,7 @@ setup_port_lcore_affinities(void)
 
 	/* Setup port_ids[] array, and check masks were ok */
 	RTE_LCORE_FOREACH(i) {
-		if (input_cores_mask & (1ULL << i)) {
+		if (input_cores_table[i]) {
 			/* Skip ports that are not enabled */
 			while ((ports_mask & (1 << rx_port)) == 0) {
 				rx_port++;
@@ -350,7 +350,7 @@ setup_port_lcore_affinities(void)
 			}
 
 			port_ids[i] = rx_port++;
-		} else if (output_cores_mask & (1ULL << (i & 0x3f))) {
+		} else if (output_cores_table[i]) {
 			/* Skip ports that are not enabled */
 			while ((ports_mask & (1 << tx_port)) == 0) {
 				tx_port++;
@@ -373,6 +373,54 @@ fail:
 	FATAL_ERROR("Invalid core/port masks specified on command line");
 }
 
+static int parse_hex2coretable(const char *coremask_string, uint8_t *core_table,
+		int core_table_size)
+{
+
+	char portmask[RTE_MAX_LCORE];
+
+	int coremask_string_len;
+	int i = 0, j = 0;
+	uint64_t num;
+	char tmp_char;
+	char *end = NULL;
+
+	coremask_string_len = strlen(coremask_string);
+	if ((coremask_string_len > (RTE_MAX_LCORE / 4) + 2)
+			|| (core_table_size > RTE_MAX_LCORE) || (coremask_string == NULL)
+			|| (core_table == NULL))
+		return -1;
+
+	memset(portmask, '\0', sizeof(portmask));
+	memset(core_table, '\0', sizeof(uint8_t) * core_table_size);
+	strncpy(portmask, coremask_string, coremask_string_len);
+
+	if (coremask_string[i] == '0') {
+		++i;
+		if (coremask_string[i] == 'X' || coremask_string[i] == 'x')
+			++i;
+	}
+
+	j = 0;
+	while (coremask_string_len - i > 0) {
+
+		tmp_char = portmask[coremask_string_len - 1];
+		end = NULL;
+		num = strtoull(&tmp_char, &end, 16);
+		if ((end == &tmp_char))
+			return -1;
+
+		for (int z = 0; z < 4; z++)
+			if (((num) & (1 << (z))) != 0)
+				core_table[z + j * 4] = 1;
+
+		coremask_string_len--;
+		j++;
+	}
+
+	return 0;
+}
+
 /* Parse the arguments given in the command line of the application */
 static void
 parse_args(int argc, char **argv)
@@ -382,15 +430,15 @@ parse_args(int argc, char **argv)
 
 	/* Disable printing messages within getopt() */
 	opterr = 0;
-
+	int reti = 0, reto = 0;
 	/* Parse command line */
 	while ((opt = getopt(argc, argv, "i:o:p:")) != EOF) {
 		switch (opt) {
 		case 'i':
-			input_cores_mask = parse_unsigned(optarg);
+			reti = parse_hex2coretable(optarg, input_cores_table, RTE_MAX_LCORE);
 			break;
 		case 'o':
-			output_cores_mask = parse_unsigned(optarg);
+			reto = parse_hex2coretable(optarg, output_cores_table, RTE_MAX_LCORE);
 			break;
 		case 'p':
 			ports_mask = parse_unsigned(optarg);
@@ -402,11 +450,11 @@ parse_args(int argc, char **argv)
 	}
 
 	/* Check that options were parsed ok */
-	if (input_cores_mask == 0) {
+	if (reti != 0) {
 		print_usage(prgname);
 		FATAL_ERROR("IN_CORES not specified correctly");
 	}
-	if (output_cores_mask == 0) {
+	if (reto != 0) {
 		print_usage(prgname);
 		FATAL_ERROR("OUT_CORES not specified correctly");
 	}
-- 
2.7.4

Re: [dpdk-dev] [PATCH v2] examples/exception_path: fix shift operation in lcore setup

[Yuanhan Liu]

On Tue, Aug 09, 2016 at 02:37:15PM +0200, Daniel Mrzyglod wrote:
> The operaton may have an undefined behavior or yield to an unexpected result.
> A bit shift operation has a shift amount which is too large or has a negative value.
> 
> As was mentioned in mailing list core list was limited to 64 so i changed bitmask
> to core array
> 
> Coverity issue: 30688
> Fixes: ea977ff1cb0b ("examples/exception_path: fix shift operation in lcore setup")
> 
> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
> ---
...

> +		if ((end == &tmp_char))
> +			return -1;

Hi,

FYI, my testrobot caught some errors when this patch is applied.
(BTW, gcc builds fine)

        --yliu

---
x86_64-native-linuxapp-clang
============================
/root/dpdk/examples/exception_path/main.c:410:12: error: equality comparison with extraneous parentheses [-Werror,-Wparentheses-equality]
                if ((end == &tmp_char))
                     ~~~~^~~~~~~~~~~~
/root/dpdk/examples/exception_path/main.c:410:12: note: remove extraneous parentheses around the comparison to silence this warning
                if ((end == &tmp_char))
                    ~    ^           ~
/root/dpdk/examples/exception_path/main.c:410:12: note: use '=' to turn this equality comparison into an assignment
                if ((end == &tmp_char))
                         ^~
                         =
1 error generated.
make[1]: *** [main.o] Error 1
make: *** [all] Error 2
error: build examples/exception_path failed
error: build failed