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 <dan...@iogearbox.net>
> Acked-by: Alexei Starovoitov <a...@kernel.org>
Acked-by: Martin KaFai Lau <ka...@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
>
