[PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map

[PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map

[Andrii Nakryiko]

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

[PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks

[Andrii Nakryiko]

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

Re: [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks

[Jakub Sitnicki]

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);
>  }

Re: [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks

[Andrii Nakryiko]

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 ;)

Re: [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks

[Jakub Sitnicki]

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 :-)

Re: [PATCH v4 bpf 2/2] selftests/bpf: extend map-in-map selftest to detect memory leaks

[Daniel Borkmann]

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.

Re: [PATCH v4 bpf 1/2] bpf: fix map leak in HASH_OF_MAPS map

[Daniel Borkmann]

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!