[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZcZqVDfUeOZsVP6j@orbyte.nwl.cc>
Date: Fri, 9 Feb 2024 19:09:24 +0100
From: Phil Sutter <phil@....cc>
To: Stephen Hemminger <stephen@...workplumber.org>
Cc: netdev@...r.kernel.org
Subject: Re: [RFC iproute2] tc/u32: use standard flexible array
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@...workplumber.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
Powered by blists - more mailing lists