diff mbox series

[RFC,iproute2] tc/u32: use standard flexible array

Message ID 20240209170714.370259-1-stephen@networkplumber.org (mailing list archive)
State RFC
Delegated to: David Ahern
Headers show
Series [RFC,iproute2] tc/u32: use standard flexible array | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Stephen Hemminger Feb. 9, 2024, 5:06 p.m. UTC 
The code to parse selectors was depending on C extension to implement
the array of keys. This would cause warnings if built with clang.
Instead use ISO C99 flexible array.

Also the maximum number of keys was hardcoded already in two places.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 tc/f_u32.c | 46 +++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

Comments

Phil Sutter Feb. 9, 2024, 6:09 p.m. UTC | #1 
On Fri, Feb 09, 2024 at 09:06:17AM -0800, Stephen Hemminger wrote:
> The code to parse selectors was depending on C extension to implement
> the array of keys. This would cause warnings if built with clang.
> Instead use ISO C99 flexible array.
> 
> Also the maximum number of keys was hardcoded already in two places.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  tc/f_u32.c | 46 +++++++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/tc/f_u32.c b/tc/f_u32.c
> index 913ec1de435d..a6e699d53d24 100644
> --- a/tc/f_u32.c
> +++ b/tc/f_u32.c
> @@ -21,6 +21,8 @@
>  #include "utils.h"
>  #include "tc_util.h"
>  
> +#define SEL_MAX_KEYS	128
> +
>  static void explain(void)
>  {
>  	fprintf(stderr,
> @@ -129,7 +131,7 @@ static int pack_key(struct tc_u32_sel *sel, __u32 key, __u32 mask,
>  		}
>  	}
>  
> -	if (hwm >= 128)
> +	if (hwm >= SEL_MAX_KEYS)
>  		return -1;
>  	if (off % 4)
>  		return -1;
> @@ -1017,10 +1019,7 @@ static __u32 u32_hash_fold(struct tc_u32_key *key)
>  static int u32_parse_opt(struct filter_util *qu, char *handle,
>  			 int argc, char **argv, struct nlmsghdr *n)
>  {
> -	struct {
> -		struct tc_u32_sel sel;
> -		struct tc_u32_key keys[128];
> -	} sel = {};
> +	struct tc_u32_sel *sel;
>  	struct tcmsg *t = NLMSG_DATA(n);
>  	struct rtattr *tail;
>  	int sel_ok = 0, terminal_ok = 0;
> @@ -1037,12 +1036,18 @@ static int u32_parse_opt(struct filter_util *qu, char *handle,
>  	if (argc == 0)
>  		return 0;
>  
> +	sel = alloca(sizeof(*sel) + SEL_MAX_KEYS * sizeof(struct tc_u32_key));
> +	if (sel == NULL)
> +		return -1;
> +
> +	memset(sel, 0, sizeof(*sel) + SEL_MAX_KEYS * sizeof(struct tc_u32_key));

Maybe a wrapper would clean things up a bit (untested):

| static void *calloca(size_t nmemb, size_t size)
| {
| 	void *ret = alloca(nmemb * size);
| 
| 	if (ret)
| 		memset(ret, 0, nmemb * size);
| 
| 	return ret;
| }

[...]
> @@ -1122,17 +1127,21 @@ static int u32_parse_opt(struct filter_util *qu, char *handle,
>  		} else if (strcmp(*argv, "sample") == 0) {
>  			__u32 hash;
>  			unsigned int divisor = 0x100;
> -			struct {
> -				struct tc_u32_sel sel;
> -				struct tc_u32_key keys[4];
> -			} sel2 = {};
> +			struct tc_u32_sel *sel2;

Not directly related to your patch, but seeing this makes me wonder
where the code asserts parse_selector won't exceed the key space.
pack_key() itself only checks it won't exceed 128 keys.

>  
>  			NEXT_ARG();
> -			if (parse_selector(&argc, &argv, &sel2.sel, n)) {
> +
> +			sel2 = alloca(sizeof(*sel) + 4 * sizeof(struct tc_u32_key));
> +			if (sel2 == NULL)
> +				return -1;
> +
> +			memset(sel2, 0, sizeof(*sel2));

The sizeof(*sel2) is identical to sizeof(struct tc_u32_sel) and thus too
small. Another point for the wrapper suggested above, as it avoids these
typical mistakes.

Cheers, Phil
diff mbox series

Patch

diff --git a/tc/f_u32.c b/tc/f_u32.c
index 913ec1de435d..a6e699d53d24 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -21,6 +21,8 @@ 
 #include "utils.h"
 #include "tc_util.h"
 
+#define SEL_MAX_KEYS	128
+
 static void explain(void)
 {
 	fprintf(stderr,
@@ -129,7 +131,7 @@  static int pack_key(struct tc_u32_sel *sel, __u32 key, __u32 mask,
 		}
 	}
 
-	if (hwm >= 128)
+	if (hwm >= SEL_MAX_KEYS)
 		return -1;
 	if (off % 4)
 		return -1;
@@ -1017,10 +1019,7 @@  static __u32 u32_hash_fold(struct tc_u32_key *key)
 static int u32_parse_opt(struct filter_util *qu, char *handle,
 			 int argc, char **argv, struct nlmsghdr *n)
 {
-	struct {
-		struct tc_u32_sel sel;
-		struct tc_u32_key keys[128];
-	} sel = {};
+	struct tc_u32_sel *sel;
 	struct tcmsg *t = NLMSG_DATA(n);
 	struct rtattr *tail;
 	int sel_ok = 0, terminal_ok = 0;
@@ -1037,12 +1036,18 @@  static int u32_parse_opt(struct filter_util *qu, char *handle,
 	if (argc == 0)
 		return 0;
 
+	sel = alloca(sizeof(*sel) + SEL_MAX_KEYS * sizeof(struct tc_u32_key));
+	if (sel == NULL)
+		return -1;
+
+	memset(sel, 0, sizeof(*sel) + SEL_MAX_KEYS * sizeof(struct tc_u32_key));
+
 	tail = addattr_nest(n, MAX_MSG, TCA_OPTIONS);
 
 	while (argc > 0) {
 		if (matches(*argv, "match") == 0) {
 			NEXT_ARG();
-			if (parse_selector(&argc, &argv, &sel.sel, n)) {
+			if (parse_selector(&argc, &argv, sel, n)) {
 				fprintf(stderr, "Illegal \"match\"\n");
 				return -1;
 			}
@@ -1050,14 +1055,14 @@  static int u32_parse_opt(struct filter_util *qu, char *handle,
 			continue;
 		} else if (matches(*argv, "offset") == 0) {
 			NEXT_ARG();
-			if (parse_offset(&argc, &argv, &sel.sel)) {
+			if (parse_offset(&argc, &argv, sel)) {
 				fprintf(stderr, "Illegal \"offset\"\n");
 				return -1;
 			}
 			continue;
 		} else if (matches(*argv, "hashkey") == 0) {
 			NEXT_ARG();
-			if (parse_hashkey(&argc, &argv, &sel.sel)) {
+			if (parse_hashkey(&argc, &argv, sel)) {
 				fprintf(stderr, "Illegal \"hashkey\"\n");
 				return -1;
 			}
@@ -1072,7 +1077,7 @@  static int u32_parse_opt(struct filter_util *qu, char *handle,
 				return -1;
 			}
 			addattr_l(n, MAX_MSG, TCA_U32_CLASSID, &flowid, 4);
-			sel.sel.flags |= TC_U32_TERMINAL;
+			sel->flags |= TC_U32_TERMINAL;
 		} else if (matches(*argv, "divisor") == 0) {
 			unsigned int divisor;
 
@@ -1122,17 +1127,21 @@  static int u32_parse_opt(struct filter_util *qu, char *handle,
 		} else if (strcmp(*argv, "sample") == 0) {
 			__u32 hash;
 			unsigned int divisor = 0x100;
-			struct {
-				struct tc_u32_sel sel;
-				struct tc_u32_key keys[4];
-			} sel2 = {};
+			struct tc_u32_sel *sel2;
 
 			NEXT_ARG();
-			if (parse_selector(&argc, &argv, &sel2.sel, n)) {
+
+			sel2 = alloca(sizeof(*sel) + 4 * sizeof(struct tc_u32_key));
+			if (sel2 == NULL)
+				return -1;
+
+			memset(sel2, 0, sizeof(*sel2));
+
+			if (parse_selector(&argc, &argv, sel2, n)) {
 				fprintf(stderr, "Illegal \"sample\"\n");
 				return -1;
 			}
-			if (sel2.sel.nkeys != 1) {
+			if (sel2->nkeys != 1) {
 				fprintf(stderr, "\"sample\" must contain exactly ONE key.\n");
 				return -1;
 			}
@@ -1146,7 +1155,7 @@  static int u32_parse_opt(struct filter_util *qu, char *handle,
 				}
 				NEXT_ARG();
 			}
-			hash = u32_hash_fold(&sel2.keys[0]);
+			hash = u32_hash_fold(&sel2->keys[0]);
 			htid = ((hash % divisor) << 12) | (htid & 0xFFF00000);
 			sample_ok = 1;
 			continue;
@@ -1197,7 +1206,7 @@  static int u32_parse_opt(struct filter_util *qu, char *handle,
 
 	/* We don't necessarily need class/flowids */
 	if (terminal_ok)
-		sel.sel.flags |= TC_U32_TERMINAL;
+		sel->flags |= TC_U32_TERMINAL;
 
 	if (order) {
 		if (TC_U32_NODE(t->tcm_handle) &&
@@ -1212,8 +1221,7 @@  static int u32_parse_opt(struct filter_util *qu, char *handle,
 		addattr_l(n, MAX_MSG, TCA_U32_HASH, &htid, 4);
 	if (sel_ok)
 		addattr_l(n, MAX_MSG, TCA_U32_SEL, &sel,
-			  sizeof(sel.sel) +
-			  sel.sel.nkeys * sizeof(struct tc_u32_key));
+			  sizeof(*sel) + sel->nkeys * sizeof(struct tc_u32_key));
 	if (flags) {
 		if (!(flags ^ (TCA_CLS_FLAGS_SKIP_HW |
 			       TCA_CLS_FLAGS_SKIP_SW))) {