[net] bpf: fix map value attribute for hash of maps

Currently, iproute2's BPF ELF loader works fine with array of maps
when retrieving the fd from a pinned node and doing a selfcheck
against the provided map attributes from the object file, but we
fail to do the same for hash of maps and thus refuse to get the
map from pinned node.

Reason is that when allocating hash of maps, fd_htab_map_alloc() will
set the value size to sizeof(void *), and any user space map creation
requests are forced to set 4 bytes as value size. Thus, selfcheck
will complain about exposed 8 bytes on 64 bit archs vs. 4 bytes from
object file as value size. Contract is that fdinfo or BPF_MAP_GET_FD_BY_ID
returns the value size used to create the map.

Fix it by handling it the same way as we do for array of maps, which
means that we leave value size at 4 bytes and in the allocation phase
round up value size to 8 bytes. alloc_htab_elem() needs an adjustment
in order to copy rounded up 8 bytes due to bpf_fd_htab_map_update_elem()
calling into htab_map_update_elem() with the pointer of the map
pointer as value. Unlike array of maps where we just xchg(), we're
using the generic htab_map_update_elem() callback also used from helper
calls, which published the key/value already on return, so we need
to ensure to memcpy() the right size.

Fixes: bcc6b1b7ebf8 ("bpf: Add hash of maps support")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/hashtab.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

On Wed, Aug 23, 2017 at 12:06:09AM +0200, Daniel Borkmann wrote:
> Currently, iproute2's BPF ELF loader works fine with array of maps
> when retrieving the fd from a pinned node and doing a selfcheck
> against the provided map attributes from the object file, but we
> fail to do the same for hash of maps and thus refuse to get the
> map from pinned node.
>
> Reason is that when allocating hash of maps, fd_htab_map_alloc() will
> set the value size to sizeof(void *), and any user space map creation
> requests are forced to set 4 bytes as value size. Thus, selfcheck
> will complain about exposed 8 bytes on 64 bit archs vs. 4 bytes from
> object file as value size. Contract is that fdinfo or BPF_MAP_GET_FD_BY_ID
> returns the value size used to create the map.
>
> Fix it by handling it the same way as we do for array of maps, which
> means that we leave value size at 4 bytes and in the allocation phase
> round up value size to 8 bytes. alloc_htab_elem() needs an adjustment
> in order to copy rounded up 8 bytes due to bpf_fd_htab_map_update_elem()
> calling into htab_map_update_elem() with the pointer of the map
> pointer as value. Unlike array of maps where we just xchg(), we're
> using the generic htab_map_update_elem() callback also used from helper
> calls, which published the key/value already on return, so we need
> to ensure to memcpy() the right size.
>
> Fixes: bcc6b1b7ebf8 ("bpf: Add hash of maps support")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Martin KaFai Lau <kafai@fb.com>

> ---
>  kernel/bpf/hashtab.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 4fb4631..d11c818 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -652,12 +652,27 @@ static void pcpu_copy_value(struct bpf_htab *htab, void __percpu *pptr,
>  	}
>  }
>
> +static bool fd_htab_map_needs_adjust(const struct bpf_htab *htab)
> +{
> +	return htab->map.map_type == BPF_MAP_TYPE_HASH_OF_MAPS &&
> +	       BITS_PER_LONG == 64;
> +}
> +
> +static u32 htab_size_value(const struct bpf_htab *htab, bool percpu)
> +{
> +	u32 size = htab->map.value_size;
> +
> +	if (percpu || fd_htab_map_needs_adjust(htab))
> +		size = round_up(size, 8);
> +	return size;
> +}
> +
>  static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
>  					 void *value, u32 key_size, u32 hash,
>  					 bool percpu, bool onallcpus,
>  					 struct htab_elem *old_elem)
>  {
> -	u32 size = htab->map.value_size;
> +	u32 size = htab_size_value(htab, percpu);
>  	bool prealloc = htab_is_prealloc(htab);
>  	struct htab_elem *l_new, **pl_new;
>  	void __percpu *pptr;
> @@ -696,9 +711,6 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
>
>  	memcpy(l_new->key, key, key_size);
>  	if (percpu) {
> -		/* round up value_size to 8 bytes */
> -		size = round_up(size, 8);
> -
>  		if (prealloc) {
>  			pptr = htab_elem_get_ptr(l_new, key_size);
>  		} else {
> @@ -1209,17 +1221,9 @@ int bpf_percpu_hash_update(struct bpf_map *map, void *key, void *value,
>
>  static struct bpf_map *fd_htab_map_alloc(union bpf_attr *attr)
>  {
> -	struct bpf_map *map;
> -
>  	if (attr->value_size != sizeof(u32))
>  		return ERR_PTR(-EINVAL);
> -
> -	/* pointer is stored internally */
> -	attr->value_size = sizeof(void *);
> -	map = htab_map_alloc(attr);
> -	attr->value_size = sizeof(u32);
> -
> -	return map;
> +	return htab_map_alloc(attr);
>  }
>
>  static void fd_htab_map_free(struct bpf_map *map)
> --
> 1.9.3
>