* [RFC iproute2] tc/u32: use standard flexible array
@ 2024-02-09 17:06 Stephen Hemminger
2024-02-09 18:09 ` Phil Sutter
0 siblings, 1 reply; 2+ messages in thread
From: Stephen Hemminger @ 2024-02-09 17:06 UTC (permalink / raw)
To: netdev; +Cc: Stephen Hemminger
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));
+
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))) {
--
2.43.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [RFC iproute2] tc/u32: use standard flexible array
2024-02-09 17:06 [RFC iproute2] tc/u32: use standard flexible array Stephen Hemminger
@ 2024-02-09 18:09 ` Phil Sutter
0 siblings, 0 replies; 2+ messages in thread
From: Phil Sutter @ 2024-02-09 18:09 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-02-09 18:09 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-09 17:06 [RFC iproute2] tc/u32: use standard flexible array Stephen Hemminger
2024-02-09 18:09 ` Phil Sutter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).