| Message ID | 20201024080541.51683-1-dev@der-flo.net |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,v3] bpf: Lift hashtab key_size limit | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for bpf-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | success | Errors and warnings before: 14 this patch: 14 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | warning | WARNING: Prefer using '"%s...", __func__' to using 'test_hash_large_key', this function's name, in a string WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 14 this patch: 14 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Sat, Oct 24, 2020 at 7:10 AM Florian Lehner <dev@der-flo.net> 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@der-flo.net> > --- 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); > +} [...]
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); > > +} > > [...]
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 - + 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); + 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; + +close_prog: + bpf_object__close(obj); +} diff --git a/tools/testing/selftests/bpf/progs/test_hash_large_key.c b/tools/testing/selftests/bpf/progs/test_hash_large_key.c new file mode 100644 index 000000000000..622ee73a4572 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_hash_large_key.c @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2020 Florian Lehner + +#include <linux/bpf.h> +#include <linux/version.h> +#include <bpf/bpf_helpers.h> + +struct bigelement { + int a; + char b[4096]; + long long c; +}; + +struct { + __uint(type, BPF_MAP_TYPE_HASH); + __uint(max_entries, 1); + __type(key, struct bigelement); + __type(value, __u32); +} hash_map SEC(".maps"); + +struct { + __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, struct bigelement); +} key_map SEC(".maps"); + +SEC("hash_large_key_demo") +int bpf_hash_large_key_test(struct __sk_buf *skb) +{ + int zero = 0, err = 1, value = 42; + struct bigelement *key; + + key = bpf_map_lookup_elem(&key_map, &zero); + if (!key) + goto err; + + if (bpf_map_update_elem(&hash_map, key, &value, BPF_ANY)) + goto err; + err = 0; +err: + return err; +} + +char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c index 0d92ebcb335d..17fe19a2114d 100644 --- a/tools/testing/selftests/bpf/test_maps.c +++ b/tools/testing/selftests/bpf/test_maps.c @@ -1225,7 +1225,7 @@ static void test_map_large(void) { struct bigkey { int a; - char b[116]; + char b[4096]; long long c; } key; int fd, i, value;
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@der-flo.net> --- 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