netdev - Re: [PATCH bpf] bpftool: fix percpu maps updating

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a4dd07fc-03b0-0f5c-7a0c-343fe543d8b3@netronome.com>
Date:   Fri, 18 Jan 2019 19:07:25 +0000
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH bpf] bpftool: fix percpu maps updating

2019-01-18 17:50 UTC+0100 ~ Paolo Abeni <pabeni@...hat.com>
> When updating a percpu map, bpftool currently copies the provided
> value only into the first per CPU copy of the specified value,
> all others instances are left zeroed.
> 
> This change explicitly copies the user-provided bytes to all the
> per CPU instances, keeping the sub-command syntax unchanged.
> 
> Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
>  tools/bpf/bpftool/map.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 2037e3dc864b..a73b5bb33ddf 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -347,6 +347,20 @@ static char **parse_bytes(char **argv, const char *name, unsigned char *val,
>  	return argv + i;
>  }
>  
> +/* on per cpu maps we must copy the provided value on all value instances */
> +static void fill_value(struct bpf_map_info *info, void *value, __u32 value_size)
> +{
> +	unsigned int i, n, step;
> +
> +	if (!map_is_per_cpu(info->type))
> +		return;
> +
> +	n = get_possible_cpus();
> +	step = round_up(info->value_size, 8);
> +	for (i = 1; i < n; i++)
> +		memcpy(value + i * step, value, info->value_size);
> +}
> +
>  static int parse_elem(char **argv, struct bpf_map_info *info,
>  		      void *key, void *value, __u32 key_size, __u32 value_size,
>  		      __u32 *flags, __u32 **value_fd)
> @@ -426,6 +440,8 @@ static int parse_elem(char **argv, struct bpf_map_info *info,
>  			argv = parse_bytes(argv, "value", value, value_size);
>  			if (!argv)
>  				return -1;
> +
> +			fill_value(info, value, value_size);

Hi Paolo,

The patch looks good to me, thanks for it! One minor nit though, could
we make it more obvious in parse_elem() that fill_value() is only for
per-CPU maps? Reading parse_elem()'s code it seems that we need to fill
a value for all map types, this is a bit confusing. I was thinking about
either renaming the function ("fill_per_cpu_value()" or something like
this?), or moving the check on map_is_per_cpu() before the function is
called. What do you think?

>  		}
>  
>  		return parse_elem(argv, info, key, NULL, key_size, value_size,
>