Re: [PATCH bpf-next v3] bpf: Lift hashtab key_size limit

[Florian Lehner <dev@der-flo.net>]

On Mon, Oct 26, 2020 at 04:07:58PM -0700, Andrii Nakryiko wrote:
> >
> > Signed-off-by: Florian Lehner <dev@der-flo.net>
> > ---
> 
> You dropped the ack from John, btw.

I was not sure if it is ok to keep the ACK for an updated patch. So I
did not include it.

> >  kernel/bpf/hashtab.c                          | 16 +++----
> >  .../selftests/bpf/prog_tests/hash_large_key.c | 28 ++++++++++++
> >  .../selftests/bpf/progs/test_hash_large_key.c | 45 +++++++++++++++++++
> >  tools/testing/selftests/bpf/test_maps.c       |  2 +-
> >  4 files changed, 79 insertions(+), 12 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/hash_large_key.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_hash_large_key.c
> >
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index 1815e97d4c9c..10097d6bcc35 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -390,17 +390,11 @@ static int htab_map_alloc_check(union bpf_attr *attr)
> >             attr->value_size == 0)
> >                 return -EINVAL;
> >
> > -       if (attr->key_size > MAX_BPF_STACK)
> > -               /* eBPF programs initialize keys on stack, so they cannot be
> > -                * larger than max stack size
> > -                */
> > -               return -E2BIG;
> > -
> > -       if (attr->value_size >= KMALLOC_MAX_SIZE -
> > -           MAX_BPF_STACK - sizeof(struct htab_elem))
> > -               /* if value_size is bigger, the user space won't be able to
> > -                * access the elements via bpf syscall. This check also makes
> > -                * sure that the elem_size doesn't overflow and it's
> > +       if ((attr->key_size + attr->value_size) >= KMALLOC_MAX_SIZE -
> 
> key_size+value_size can overflow, can't it? So probably want to cast
> to (size_t) or __u64?
> 

I will add this cast to u64.

> > +           sizeof(struct htab_elem))
> > +               /* if key_size + value_size is bigger, the user space won't be
> > +                * able to access the elements via bpf syscall. This check
> > +                * also makes sure that the elem_size doesn't overflow and it's
> >                  * kmalloc-able later in htab_map_update_elem()
> >                  */
> >                 return -E2BIG;
> > diff --git a/tools/testing/selftests/bpf/prog_tests/hash_large_key.c b/tools/testing/selftests/bpf/prog_tests/hash_large_key.c
> > new file mode 100644
> > index 000000000000..962f56060b76
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/hash_large_key.c
> > @@ -0,0 +1,28 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2020 Florian Lehner
> > +
> > +#include <test_progs.h>
> > +
> > +void test_hash_large_key(void)
> > +{
> > +       const char *file = "./test_hash_large_key.o";
> > +       int prog_fd, map_fd[2];
> > +       struct bpf_object *obj = NULL;
> > +       int err = 0;
> > +
> > +       err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
> 
> Please utilize the BPF skeleton in the new tests.
> 

I will update the test and utilize the BPF skeleton.

> > +       if (CHECK_FAIL(err)) {
> > +               printf("test_hash_large_key: bpf_prog_load errno %d", errno);
> > +               goto close_prog;
> > +       }
> > +
> > +       map_fd[0] = bpf_find_map(__func__, obj, "hash_map");
> > +       if (CHECK_FAIL(map_fd[0] < 0))
> > +               goto close_prog;
> > +       map_fd[1] = bpf_find_map(__func__, obj, "key_map");
> > +       if (CHECK_FAIL(map_fd[1] < 0))
> > +               goto close_prog;
> 
> You are not really checking much here.
> 
> Why don't you check that the value was set to 42 here? Consider also
> using big global variables as your key and value. You can specify them
> from user-space with BPF skeleton easily to any custom value (not just
> zeroes).
> 
> 
> > +
> > +close_prog:
> > +       bpf_object__close(obj);
> > +}
> 
> [...]