On Sat, Oct 24, 2020 at 7:10 AM Florian Lehner <dev@xxxxxxxxxxx> wrote:
>
> Currently key_size of hashtab is limited to MAX_BPF_STACK.
> As the key of hashtab can also be a value from a per cpu map it can be
> larger than MAX_BPF_STACK.
>
> The use-case for this patch originates to implement allow/disallow
> lists for files and file paths. The maximum length of file paths is
> defined by PATH_MAX with 4096 chars including nul.
> This limit exceeds MAX_BPF_STACK.
>
> Changelog:
>
> v3:
> - Rebase
>
> v2:
> - Add a test for bpf side
>
> Signed-off-by: Florian Lehner <dev@xxxxxxxxxxx>
> ---
You dropped the ack from John, btw.
> 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?
> + 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.
> + 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);
> +}
[...]
