* [PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map
@ 2020-07-29 4:09 Andrii Nakryiko
2020-07-29 4:09 ` [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks Andrii Nakryiko
2020-07-29 23:44 ` [PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map Daniel Borkmann
0 siblings, 2 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2020-07-29 4:09 UTC (permalink / raw)
To: bpf, netdev, ast, daniel
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko, Song Liu, stable
Fix HASH_OF_MAPS bug of not putting inner map pointer on bpf_map_elem_update()
operation. This is due to per-cpu extra_elems optimization, which bypassed
free_htab_elem() logic doing proper clean ups. Make sure that inner map is put
properly in optimized case as well.
Fixes: 8c290e60fa2a ("bpf: fix hashmap extra_elems logic")
Acked-by: Song Liu <songliubraving@fb.com>
Cc: <stable@vger.kernel.org> # v4.14+
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
kernel/bpf/hashtab.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index b4b288a3c3c9..b32cc8ce8ff6 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -779,15 +779,20 @@ static void htab_elem_free_rcu(struct rcu_head *head)
htab_elem_free(htab, l);
}
-static void free_htab_elem(struct bpf_htab *htab, struct htab_elem *l)
+static void htab_put_fd_value(struct bpf_htab *htab, struct htab_elem *l)
{
struct bpf_map *map = &htab->map;
+ void *ptr;
if (map->ops->map_fd_put_ptr) {
- void *ptr = fd_htab_map_get_ptr(map, l);
-
+ ptr = fd_htab_map_get_ptr(map, l);
map->ops->map_fd_put_ptr(ptr);
}
+}
+
+static void free_htab_elem(struct bpf_htab *htab, struct htab_elem *l)
+{
+ htab_put_fd_value(htab, l);
if (htab_is_prealloc(htab)) {
__pcpu_freelist_push(&htab->freelist, &l->fnode);
@@ -839,6 +844,7 @@ static struct htab_elem *alloc_htab_elem(struct bpf_htab *htab, void *key,
*/
pl_new = this_cpu_ptr(htab->extra_elems);
l_new = *pl_new;
+ htab_put_fd_value(htab, old_elem);
*pl_new = old_elem;
} else {
struct pcpu_freelist_node *l;
--
2.24.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks
2020-07-29 4:09 [PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map Andrii Nakryiko
@ 2020-07-29 4:09 ` Andrii Nakryiko
2020-07-29 14:29 ` Jakub Sitnicki
2020-07-29 23:44 ` [PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map Daniel Borkmann
1 sibling, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2020-07-29 4:09 UTC (permalink / raw)
To: bpf, netdev, ast, daniel
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko, Song Liu, stable
Add test validating that all inner maps are released properly after skeleton
is destroyed. To ensure determinism, trigger kernel-side synchronize_rcu()
before checking map existence by their IDs.
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
.../selftests/bpf/prog_tests/btf_map_in_map.c | 124 ++++++++++++++++--
1 file changed, 110 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c b/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
index f7ee8fa377ad..f6eee3fb933c 100644
--- a/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
+++ b/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
@@ -5,10 +5,60 @@
#include "test_btf_map_in_map.skel.h"
+static int duration;
+
+static __u32 bpf_map_id(struct bpf_map *map)
+{
+ struct bpf_map_info info;
+ __u32 info_len = sizeof(info);
+ int err;
+
+ memset(&info, 0, info_len);
+ err = bpf_obj_get_info_by_fd(bpf_map__fd(map), &info, &info_len);
+ if (err)
+ return 0;
+ return info.id;
+}
+
+/*
+ * Trigger synchronize_cpu() in kernel.
+ *
+ * ARRAY_OF_MAPS/HASH_OF_MAPS lookup/update operations trigger
+ * synchronize_rcu(), if looking up/updating non-NULL element. Use this fact
+ * to trigger synchronize_cpu(): create map-in-map, create a trivial ARRAY
+ * map, update map-in-map with ARRAY inner map. Then cleanup. At the end, at
+ * least one synchronize_rcu() would be called.
+ */
+static int kern_sync_rcu(void)
+{
+ int inner_map_fd, outer_map_fd, err, zero = 0;
+
+ inner_map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, 4, 4, 1, 0);
+ if (CHECK(inner_map_fd < 0, "inner_map_create", "failed %d\n", -errno))
+ return -1;
+
+ outer_map_fd = bpf_create_map_in_map(BPF_MAP_TYPE_ARRAY_OF_MAPS, NULL,
+ sizeof(int), inner_map_fd, 1, 0);
+ if (CHECK(outer_map_fd < 0, "outer_map_create", "failed %d\n", -errno)) {
+ close(inner_map_fd);
+ return -1;
+ }
+
+ err = bpf_map_update_elem(outer_map_fd, &zero, &inner_map_fd, 0);
+ if (err)
+ err = -errno;
+ CHECK(err, "outer_map_update", "failed %d\n", err);
+ close(inner_map_fd);
+ close(outer_map_fd);
+ return err;
+}
+
void test_btf_map_in_map(void)
{
- int duration = 0, err, key = 0, val;
- struct test_btf_map_in_map* skel;
+ int err, key = 0, val, i;
+ struct test_btf_map_in_map *skel;
+ int outer_arr_fd, outer_hash_fd;
+ int fd, map1_fd, map2_fd, map1_id, map2_id;
skel = test_btf_map_in_map__open_and_load();
if (CHECK(!skel, "skel_open", "failed to open&load skeleton\n"))
@@ -18,32 +68,78 @@ void test_btf_map_in_map(void)
if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
goto cleanup;
+ map1_fd = bpf_map__fd(skel->maps.inner_map1);
+ map2_fd = bpf_map__fd(skel->maps.inner_map2);
+ outer_arr_fd = bpf_map__fd(skel->maps.outer_arr);
+ outer_hash_fd = bpf_map__fd(skel->maps.outer_hash);
+
/* inner1 = input, inner2 = input + 1 */
- val = bpf_map__fd(skel->maps.inner_map1);
- bpf_map_update_elem(bpf_map__fd(skel->maps.outer_arr), &key, &val, 0);
- val = bpf_map__fd(skel->maps.inner_map2);
- bpf_map_update_elem(bpf_map__fd(skel->maps.outer_hash), &key, &val, 0);
+ map1_fd = bpf_map__fd(skel->maps.inner_map1);
+ bpf_map_update_elem(outer_arr_fd, &key, &map1_fd, 0);
+ map2_fd = bpf_map__fd(skel->maps.inner_map2);
+ bpf_map_update_elem(outer_hash_fd, &key, &map2_fd, 0);
skel->bss->input = 1;
usleep(1);
- bpf_map_lookup_elem(bpf_map__fd(skel->maps.inner_map1), &key, &val);
+ bpf_map_lookup_elem(map1_fd, &key, &val);
CHECK(val != 1, "inner1", "got %d != exp %d\n", val, 1);
- bpf_map_lookup_elem(bpf_map__fd(skel->maps.inner_map2), &key, &val);
+ bpf_map_lookup_elem(map2_fd, &key, &val);
CHECK(val != 2, "inner2", "got %d != exp %d\n", val, 2);
/* inner1 = input + 1, inner2 = input */
- val = bpf_map__fd(skel->maps.inner_map2);
- bpf_map_update_elem(bpf_map__fd(skel->maps.outer_arr), &key, &val, 0);
- val = bpf_map__fd(skel->maps.inner_map1);
- bpf_map_update_elem(bpf_map__fd(skel->maps.outer_hash), &key, &val, 0);
+ bpf_map_update_elem(outer_arr_fd, &key, &map2_fd, 0);
+ bpf_map_update_elem(outer_hash_fd, &key, &map1_fd, 0);
skel->bss->input = 3;
usleep(1);
- bpf_map_lookup_elem(bpf_map__fd(skel->maps.inner_map1), &key, &val);
+ bpf_map_lookup_elem(map1_fd, &key, &val);
CHECK(val != 4, "inner1", "got %d != exp %d\n", val, 4);
- bpf_map_lookup_elem(bpf_map__fd(skel->maps.inner_map2), &key, &val);
+ bpf_map_lookup_elem(map2_fd, &key, &val);
CHECK(val != 3, "inner2", "got %d != exp %d\n", val, 3);
+ for (i = 0; i < 5; i++) {
+ val = i % 2 ? map1_fd : map2_fd;
+ err = bpf_map_update_elem(outer_hash_fd, &key, &val, 0);
+ if (CHECK_FAIL(err)) {
+ printf("failed to update hash_of_maps on iter #%d\n", i);
+ goto cleanup;
+ }
+ err = bpf_map_update_elem(outer_arr_fd, &key, &val, 0);
+ if (CHECK_FAIL(err)) {
+ printf("failed to update hash_of_maps on iter #%d\n", i);
+ goto cleanup;
+ }
+ }
+
+ map1_id = bpf_map_id(skel->maps.inner_map1);
+ map2_id = bpf_map_id(skel->maps.inner_map2);
+ CHECK(map1_id == 0, "map1_id", "failed to get ID 1\n");
+ CHECK(map2_id == 0, "map2_id", "failed to get ID 2\n");
+
+ test_btf_map_in_map__destroy(skel);
+ skel = NULL;
+
+ /* we need to either wait for or force synchronize_rcu(), before
+ * checking for "still exists" condition, otherwise map could still be
+ * resolvable by ID, causing false positives.
+ *
+ * Older kernels (5.8 and earlier) freed map only after two
+ * synchronize_rcu()s, so trigger two, to be entirely sure.
+ */
+ CHECK(kern_sync_rcu(), "sync_rcu", "failed\n");
+ CHECK(kern_sync_rcu(), "sync_rcu", "failed\n");
+
+ fd = bpf_map_get_fd_by_id(map1_id);
+ if (CHECK(fd >= 0, "map1_leak", "inner_map1 leaked!\n")) {
+ close(fd);
+ goto cleanup;
+ }
+ fd = bpf_map_get_fd_by_id(map2_id);
+ if (CHECK(fd >= 0, "map2_leak", "inner_map2 leaked!\n")) {
+ close(fd);
+ goto cleanup;
+ }
+
cleanup:
test_btf_map_in_map__destroy(skel);
}
--
2.24.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks
2020-07-29 4:09 ` [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks Andrii Nakryiko
@ 2020-07-29 14:29 ` Jakub Sitnicki
2020-07-29 17:48 ` Andrii Nakryiko
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Sitnicki @ 2020-07-29 14:29 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, netdev, ast, daniel, andrii.nakryiko, kernel-team, Song Liu,
stable
On Wed, Jul 29, 2020 at 06:09 AM CEST, Andrii Nakryiko wrote:
> Add test validating that all inner maps are released properly after skeleton
> is destroyed. To ensure determinism, trigger kernel-side synchronize_rcu()
> before checking map existence by their IDs.
>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> .../selftests/bpf/prog_tests/btf_map_in_map.c | 124 ++++++++++++++++--
> 1 file changed, 110 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c b/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
> index f7ee8fa377ad..f6eee3fb933c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
> +++ b/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
> @@ -5,10 +5,60 @@
>
> #include "test_btf_map_in_map.skel.h"
>
> +static int duration;
> +
> +static __u32 bpf_map_id(struct bpf_map *map)
> +{
> + struct bpf_map_info info;
> + __u32 info_len = sizeof(info);
> + int err;
> +
> + memset(&info, 0, info_len);
> + err = bpf_obj_get_info_by_fd(bpf_map__fd(map), &info, &info_len);
> + if (err)
> + return 0;
> + return info.id;
> +}
> +
> +/*
> + * Trigger synchronize_cpu() in kernel.
Nit: synchronize_*r*cu().
> + *
> + * ARRAY_OF_MAPS/HASH_OF_MAPS lookup/update operations trigger
> + * synchronize_rcu(), if looking up/updating non-NULL element. Use this fact
> + * to trigger synchronize_cpu(): create map-in-map, create a trivial ARRAY
> + * map, update map-in-map with ARRAY inner map. Then cleanup. At the end, at
> + * least one synchronize_rcu() would be called.
> + */
That's a cool trick. I'm a bit confused by "looking up/updating non-NULL
element". It looks like you're updating an element that is NULL/unset in
the code below. What am I missing?
> +static int kern_sync_rcu(void)
> +{
> + int inner_map_fd, outer_map_fd, err, zero = 0;
> +
> + inner_map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, 4, 4, 1, 0);
> + if (CHECK(inner_map_fd < 0, "inner_map_create", "failed %d\n", -errno))
> + return -1;
> +
> + outer_map_fd = bpf_create_map_in_map(BPF_MAP_TYPE_ARRAY_OF_MAPS, NULL,
> + sizeof(int), inner_map_fd, 1, 0);
> + if (CHECK(outer_map_fd < 0, "outer_map_create", "failed %d\n", -errno)) {
> + close(inner_map_fd);
> + return -1;
> + }
> +
> + err = bpf_map_update_elem(outer_map_fd, &zero, &inner_map_fd, 0);
> + if (err)
> + err = -errno;
> + CHECK(err, "outer_map_update", "failed %d\n", err);
> + close(inner_map_fd);
> + close(outer_map_fd);
> + return err;
> +}
> +
> void test_btf_map_in_map(void)
> {
> - int duration = 0, err, key = 0, val;
> - struct test_btf_map_in_map* skel;
> + int err, key = 0, val, i;
> + struct test_btf_map_in_map *skel;
> + int outer_arr_fd, outer_hash_fd;
> + int fd, map1_fd, map2_fd, map1_id, map2_id;
>
> skel = test_btf_map_in_map__open_and_load();
> if (CHECK(!skel, "skel_open", "failed to open&load skeleton\n"))
> @@ -18,32 +68,78 @@ void test_btf_map_in_map(void)
> if (CHECK(err, "skel_attach", "skeleton attach failed: %d\n", err))
> goto cleanup;
>
> + map1_fd = bpf_map__fd(skel->maps.inner_map1);
> + map2_fd = bpf_map__fd(skel->maps.inner_map2);
> + outer_arr_fd = bpf_map__fd(skel->maps.outer_arr);
> + outer_hash_fd = bpf_map__fd(skel->maps.outer_hash);
> +
> /* inner1 = input, inner2 = input + 1 */
> - val = bpf_map__fd(skel->maps.inner_map1);
> - bpf_map_update_elem(bpf_map__fd(skel->maps.outer_arr), &key, &val, 0);
> - val = bpf_map__fd(skel->maps.inner_map2);
> - bpf_map_update_elem(bpf_map__fd(skel->maps.outer_hash), &key, &val, 0);
> + map1_fd = bpf_map__fd(skel->maps.inner_map1);
> + bpf_map_update_elem(outer_arr_fd, &key, &map1_fd, 0);
> + map2_fd = bpf_map__fd(skel->maps.inner_map2);
> + bpf_map_update_elem(outer_hash_fd, &key, &map2_fd, 0);
> skel->bss->input = 1;
> usleep(1);
>
> - bpf_map_lookup_elem(bpf_map__fd(skel->maps.inner_map1), &key, &val);
> + bpf_map_lookup_elem(map1_fd, &key, &val);
> CHECK(val != 1, "inner1", "got %d != exp %d\n", val, 1);
> - bpf_map_lookup_elem(bpf_map__fd(skel->maps.inner_map2), &key, &val);
> + bpf_map_lookup_elem(map2_fd, &key, &val);
> CHECK(val != 2, "inner2", "got %d != exp %d\n", val, 2);
>
> /* inner1 = input + 1, inner2 = input */
> - val = bpf_map__fd(skel->maps.inner_map2);
> - bpf_map_update_elem(bpf_map__fd(skel->maps.outer_arr), &key, &val, 0);
> - val = bpf_map__fd(skel->maps.inner_map1);
> - bpf_map_update_elem(bpf_map__fd(skel->maps.outer_hash), &key, &val, 0);
> + bpf_map_update_elem(outer_arr_fd, &key, &map2_fd, 0);
> + bpf_map_update_elem(outer_hash_fd, &key, &map1_fd, 0);
> skel->bss->input = 3;
> usleep(1);
>
> - bpf_map_lookup_elem(bpf_map__fd(skel->maps.inner_map1), &key, &val);
> + bpf_map_lookup_elem(map1_fd, &key, &val);
> CHECK(val != 4, "inner1", "got %d != exp %d\n", val, 4);
> - bpf_map_lookup_elem(bpf_map__fd(skel->maps.inner_map2), &key, &val);
> + bpf_map_lookup_elem(map2_fd, &key, &val);
> CHECK(val != 3, "inner2", "got %d != exp %d\n", val, 3);
>
> + for (i = 0; i < 5; i++) {
> + val = i % 2 ? map1_fd : map2_fd;
> + err = bpf_map_update_elem(outer_hash_fd, &key, &val, 0);
> + if (CHECK_FAIL(err)) {
> + printf("failed to update hash_of_maps on iter #%d\n", i);
> + goto cleanup;
> + }
> + err = bpf_map_update_elem(outer_arr_fd, &key, &val, 0);
> + if (CHECK_FAIL(err)) {
> + printf("failed to update hash_of_maps on iter #%d\n", i);
> + goto cleanup;
> + }
> + }
> +
> + map1_id = bpf_map_id(skel->maps.inner_map1);
> + map2_id = bpf_map_id(skel->maps.inner_map2);
> + CHECK(map1_id == 0, "map1_id", "failed to get ID 1\n");
> + CHECK(map2_id == 0, "map2_id", "failed to get ID 2\n");
> +
> + test_btf_map_in_map__destroy(skel);
> + skel = NULL;
> +
> + /* we need to either wait for or force synchronize_rcu(), before
> + * checking for "still exists" condition, otherwise map could still be
> + * resolvable by ID, causing false positives.
> + *
> + * Older kernels (5.8 and earlier) freed map only after two
> + * synchronize_rcu()s, so trigger two, to be entirely sure.
> + */
> + CHECK(kern_sync_rcu(), "sync_rcu", "failed\n");
> + CHECK(kern_sync_rcu(), "sync_rcu", "failed\n");
> +
> + fd = bpf_map_get_fd_by_id(map1_id);
> + if (CHECK(fd >= 0, "map1_leak", "inner_map1 leaked!\n")) {
> + close(fd);
> + goto cleanup;
> + }
> + fd = bpf_map_get_fd_by_id(map2_id);
> + if (CHECK(fd >= 0, "map2_leak", "inner_map2 leaked!\n")) {
> + close(fd);
> + goto cleanup;
> + }
> +
> cleanup:
> test_btf_map_in_map__destroy(skel);
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks
2020-07-29 14:29 ` Jakub Sitnicki
@ 2020-07-29 17:48 ` Andrii Nakryiko
2020-07-29 20:29 ` Jakub Sitnicki
2020-07-29 21:17 ` Daniel Borkmann
0 siblings, 2 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2020-07-29 17:48 UTC (permalink / raw)
To: Jakub Sitnicki
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team, Song Liu, linux- stable
On Wed, Jul 29, 2020 at 7:29 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> On Wed, Jul 29, 2020 at 06:09 AM CEST, Andrii Nakryiko wrote:
> > Add test validating that all inner maps are released properly after skeleton
> > is destroyed. To ensure determinism, trigger kernel-side synchronize_rcu()
> > before checking map existence by their IDs.
> >
> > Acked-by: Song Liu <songliubraving@fb.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > .../selftests/bpf/prog_tests/btf_map_in_map.c | 124 ++++++++++++++++--
> > 1 file changed, 110 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c b/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
> > index f7ee8fa377ad..f6eee3fb933c 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/btf_map_in_map.c
> > @@ -5,10 +5,60 @@
> >
> > #include "test_btf_map_in_map.skel.h"
> >
> > +static int duration;
> > +
> > +static __u32 bpf_map_id(struct bpf_map *map)
> > +{
> > + struct bpf_map_info info;
> > + __u32 info_len = sizeof(info);
> > + int err;
> > +
> > + memset(&info, 0, info_len);
> > + err = bpf_obj_get_info_by_fd(bpf_map__fd(map), &info, &info_len);
> > + if (err)
> > + return 0;
> > + return info.id;
> > +}
> > +
> > +/*
> > + * Trigger synchronize_cpu() in kernel.
>
> Nit: synchronize_*r*cu().
welp, yeah
>
> > + *
> > + * ARRAY_OF_MAPS/HASH_OF_MAPS lookup/update operations trigger
> > + * synchronize_rcu(), if looking up/updating non-NULL element. Use this fact
> > + * to trigger synchronize_cpu(): create map-in-map, create a trivial ARRAY
> > + * map, update map-in-map with ARRAY inner map. Then cleanup. At the end, at
> > + * least one synchronize_rcu() would be called.
> > + */
>
> That's a cool trick. I'm a bit confused by "looking up/updating non-NULL
> element". It looks like you're updating an element that is NULL/unset in
> the code below. What am I missing?
I was basically trying to say that it has to be a successful lookup or
update. For lookup that means looking up non-NULL (existing) entry.
For update -- setting valid inner map FD.
Not sure fixing this and typo above is worth it to post v5.
>
> > +static int kern_sync_rcu(void)
> > +{
> > + int inner_map_fd, outer_map_fd, err, zero = 0;
> > +
> > + inner_map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, 4, 4, 1, 0);
> > + if (CHECK(inner_map_fd < 0, "inner_map_create", "failed %d\n", -errno))
> > + return -1;
> > +
> > + outer_map_fd = bpf_create_map_in_map(BPF_MAP_TYPE_ARRAY_OF_MAPS, NULL,
> > + sizeof(int), inner_map_fd, 1, 0);
> > + if (CHECK(outer_map_fd < 0, "outer_map_create", "failed %d\n", -errno)) {
> > + close(inner_map_fd);
> > + return -1;
> > + }
> > +
> > + err = bpf_map_update_elem(outer_map_fd, &zero, &inner_map_fd, 0);
> > + if (err)
> > + err = -errno;
> > + CHECK(err, "outer_map_update", "failed %d\n", err);
> > + close(inner_map_fd);
> > + close(outer_map_fd);
> > + return err;
> > +}
> > +
[...]
trimming's good ;)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks
2020-07-29 17:48 ` Andrii Nakryiko
@ 2020-07-29 20:29 ` Jakub Sitnicki
2020-07-29 21:17 ` Daniel Borkmann
1 sibling, 0 replies; 7+ messages in thread
From: Jakub Sitnicki @ 2020-07-29 20:29 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team, Song Liu, linux- stable
On Wed, Jul 29, 2020 at 07:48 PM CEST, Andrii Nakryiko wrote:
> On Wed, Jul 29, 2020 at 7:29 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> On Wed, Jul 29, 2020 at 06:09 AM CEST, Andrii Nakryiko wrote:
[...]
>> > +/*
>> > + * Trigger synchronize_cpu() in kernel.
>>
>> Nit: synchronize_*r*cu().
>
> welp, yeah
>
>>
>> > + *
>> > + * ARRAY_OF_MAPS/HASH_OF_MAPS lookup/update operations trigger
>> > + * synchronize_rcu(), if looking up/updating non-NULL element. Use this fact
>> > + * to trigger synchronize_cpu(): create map-in-map, create a trivial ARRAY
>> > + * map, update map-in-map with ARRAY inner map. Then cleanup. At the end, at
>> > + * least one synchronize_rcu() would be called.
>> > + */
>>
>> That's a cool trick. I'm a bit confused by "looking up/updating non-NULL
>> element". It looks like you're updating an element that is NULL/unset in
>> the code below. What am I missing?
>
> I was basically trying to say that it has to be a successful lookup or
> update. For lookup that means looking up non-NULL (existing) entry.
> For update -- setting valid inner map FD.
>
> Not sure fixing this and typo above is worth it to post v5.
I just wanted to understand that the helper is working as intended. It
seems handy. I agree that it's not worth respinning the patches just for
this.
>
>>
>> > +static int kern_sync_rcu(void)
>> > +{
>> > + int inner_map_fd, outer_map_fd, err, zero = 0;
>> > +
>> > + inner_map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, 4, 4, 1, 0);
>> > + if (CHECK(inner_map_fd < 0, "inner_map_create", "failed %d\n", -errno))
>> > + return -1;
>> > +
>> > + outer_map_fd = bpf_create_map_in_map(BPF_MAP_TYPE_ARRAY_OF_MAPS, NULL,
>> > + sizeof(int), inner_map_fd, 1, 0);
>> > + if (CHECK(outer_map_fd < 0, "outer_map_create", "failed %d\n", -errno)) {
>> > + close(inner_map_fd);
>> > + return -1;
>> > + }
>> > +
>> > + err = bpf_map_update_elem(outer_map_fd, &zero, &inner_map_fd, 0);
>> > + if (err)
>> > + err = -errno;
>> > + CHECK(err, "outer_map_update", "failed %d\n", err);
>> > + close(inner_map_fd);
>> > + close(outer_map_fd);
>> > + return err;
>> > +}
>> > +
>
> [...]
>
> trimming's good ;)
You caught me. Just being lazy. No excuses :-)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks
2020-07-29 17:48 ` Andrii Nakryiko
2020-07-29 20:29 ` Jakub Sitnicki
@ 2020-07-29 21:17 ` Daniel Borkmann
1 sibling, 0 replies; 7+ messages in thread
From: Daniel Borkmann @ 2020-07-29 21:17 UTC (permalink / raw)
To: Andrii Nakryiko, Jakub Sitnicki
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov, Kernel Team,
Song Liu, linux- stable
On 7/29/20 7:48 PM, Andrii Nakryiko wrote:
> On Wed, Jul 29, 2020 at 7:29 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> On Wed, Jul 29, 2020 at 06:09 AM CEST, Andrii Nakryiko wrote:
>>> Add test validating that all inner maps are released properly after skeleton
>>> is destroyed. To ensure determinism, trigger kernel-side synchronize_rcu()
>>> before checking map existence by their IDs.
>>>
>>> Acked-by: Song Liu <songliubraving@fb.com>
>>> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
[...]
>>> +/*
>>> + * Trigger synchronize_cpu() in kernel.
>>
>> Nit: synchronize_*r*cu().
>
> welp, yeah
>
>>
>>> + *
>>> + * ARRAY_OF_MAPS/HASH_OF_MAPS lookup/update operations trigger
>>> + * synchronize_rcu(), if looking up/updating non-NULL element. Use this fact
>>> + * to trigger synchronize_cpu(): create map-in-map, create a trivial ARRAY
>>> + * map, update map-in-map with ARRAY inner map. Then cleanup. At the end, at
>>> + * least one synchronize_rcu() would be called.
>>> + */
>>
>> That's a cool trick. I'm a bit confused by "looking up/updating non-NULL
>> element". It looks like you're updating an element that is NULL/unset in
>> the code below. What am I missing?
>
> I was basically trying to say that it has to be a successful lookup or
> update. For lookup that means looking up non-NULL (existing) entry.
> For update -- setting valid inner map FD.
>
> Not sure fixing this and typo above is worth it to post v5.
Nope, I'll fix it up while applying.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map
2020-07-29 4:09 [PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map Andrii Nakryiko
2020-07-29 4:09 ` [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks Andrii Nakryiko
@ 2020-07-29 23:44 ` Daniel Borkmann
1 sibling, 0 replies; 7+ messages in thread
From: Daniel Borkmann @ 2020-07-29 23:44 UTC (permalink / raw)
To: Andrii Nakryiko, bpf, netdev, ast
Cc: andrii.nakryiko, kernel-team, Song Liu, stable
On 7/29/20 6:09 AM, Andrii Nakryiko wrote:
> Fix HASH_OF_MAPS bug of not putting inner map pointer on bpf_map_elem_update()
> operation. This is due to per-cpu extra_elems optimization, which bypassed
> free_htab_elem() logic doing proper clean ups. Make sure that inner map is put
> properly in optimized case as well.
>
> Fixes: 8c290e60fa2a ("bpf: fix hashmap extra_elems logic")
> Acked-by: Song Liu <songliubraving@fb.com>
> Cc: <stable@vger.kernel.org> # v4.14+
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Both applied, thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-29 23:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-29 4:09 [PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map Andrii Nakryiko
2020-07-29 4:09 ` [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks Andrii Nakryiko
2020-07-29 14:29 ` Jakub Sitnicki
2020-07-29 17:48 ` Andrii Nakryiko
2020-07-29 20:29 ` Jakub Sitnicki
2020-07-29 21:17 ` Daniel Borkmann
2020-07-29 23:44 ` [PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map Daniel Borkmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.