netdev - Re: [PATCH bpf v2] 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: <aa8768eb-82c1-5cc6-14f4-052d5c24a13e@netronome.com>
Date:   Mon, 21 Jan 2019 10:47:18 +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 v2] bpftool: fix percpu maps updating

2019-01-21 11:33 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.
> 
> v1 -> v2:
>  - rename the helper as per Quentin's suggestion
> 
> Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool")
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>

Thanks a lot for the change! Another one below, though.

> ---
>  tools/bpf/bpftool/map.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 2037e3dc864b..27c51dbc887c 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -347,6 +347,21 @@ 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_per_cpu_value(struct bpf_map_info *info, void *value,
> +			       __u32 value_size)

The last "value_size" argument is never used in your function (you just
use info->value_type). I just realised it now, sorry :s.

> +{
> +	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 +441,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_per_cpu_value(info, value, value_size);
>  		}
>  
>  		return parse_elem(argv, info, key, NULL, key_size, value_size,
>