[PATCH libbpf] libbpf: check if pin_path was set even map fd exist

[PATCH libbpf] libbpf: check if pin_path was set even map fd exist

[Hangbin Liu]

Say a user reuse map fd after creating a map manually and set the
pin_path, then load the object via libbpf.

In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will
return 0 if there is no pinned map in map->pin_path. Then after
checking if map fd exist, we should also check if pin_path was set
and do bpf_map__pin() instead of continue the loop.

Fix it by creating map if fd not exist and continue checking pin_path
after that.

Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/lib/bpf/libbpf.c | 75 +++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 38 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e493d6048143..d4149585a76c 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -3861,50 +3861,49 @@ bpf_object__create_maps(struct bpf_object *obj)
 			}
 		}
 
-		if (map->fd >= 0) {
-			pr_debug("map '%s': skipping creation (preset fd=%d)\n",
-				 map->name, map->fd);
-			continue;
-		}
-
-		err = bpf_object__create_map(obj, map);
-		if (err)
-			goto err_out;
-
-		pr_debug("map '%s': created successfully, fd=%d\n", map->name,
-			 map->fd);
-
-		if (bpf_map__is_internal(map)) {
-			err = bpf_object__populate_internal_map(obj, map);
-			if (err < 0) {
-				zclose(map->fd);
+		if (map->fd < 0) {
+			err = bpf_object__create_map(obj, map);
+			if (err)
 				goto err_out;
-			}
-		}
-
-		if (map->init_slots_sz) {
-			for (j = 0; j < map->init_slots_sz; j++) {
-				const struct bpf_map *targ_map;
-				int fd;
 
-				if (!map->init_slots[j])
-					continue;
+			pr_debug("map '%s': created successfully, fd=%d\n", map->name,
+				 map->fd);
 
-				targ_map = map->init_slots[j];
-				fd = bpf_map__fd(targ_map);
-				err = bpf_map_update_elem(map->fd, &j, &fd, 0);
-				if (err) {
-					err = -errno;
-					pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
-						map->name, j, targ_map->name,
-						fd, err);
+			if (bpf_map__is_internal(map)) {
+				err = bpf_object__populate_internal_map(obj, map);
+				if (err < 0) {
+					zclose(map->fd);
 					goto err_out;
 				}
-				pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
-					 map->name, j, targ_map->name, fd);
 			}
-			zfree(&map->init_slots);
-			map->init_slots_sz = 0;
+
+			if (map->init_slots_sz) {
+				for (j = 0; j < map->init_slots_sz; j++) {
+					const struct bpf_map *targ_map;
+					int fd;
+
+					if (!map->init_slots[j])
+						continue;
+
+					targ_map = map->init_slots[j];
+					fd = bpf_map__fd(targ_map);
+					err = bpf_map_update_elem(map->fd, &j, &fd, 0);
+					if (err) {
+						err = -errno;
+						pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
+							map->name, j, targ_map->name,
+							fd, err);
+						goto err_out;
+					}
+					pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
+						map->name, j, targ_map->name, fd);
+				}
+				zfree(&map->init_slots);
+				map->init_slots_sz = 0;
+			}
+		} else {
+			pr_debug("map '%s': skipping creation (preset fd=%d)\n",
+				 map->name, map->fd);
 		}
 
 		if (map->pin_path && !map->pinned) {
-- 
2.25.4

Re: [PATCH libbpf] libbpf: check if pin_path was set even map fd exist

[Maciej Fijalkowski]

On Fri, Oct 02, 2020 at 03:57:50PM +0800, Hangbin Liu wrote:
> Say a user reuse map fd after creating a map manually and set the
> pin_path, then load the object via libbpf.
> 
> In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will
> return 0 if there is no pinned map in map->pin_path. Then after
> checking if map fd exist, we should also check if pin_path was set
> and do bpf_map__pin() instead of continue the loop.
> 
> Fix it by creating map if fd not exist and continue checking pin_path
> after that.
> 
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 75 +++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 38 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index e493d6048143..d4149585a76c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -3861,50 +3861,49 @@ bpf_object__create_maps(struct bpf_object *obj)
>  			}
>  		}
>  
> -		if (map->fd >= 0) {
> -			pr_debug("map '%s': skipping creation (preset fd=%d)\n",
> -				 map->name, map->fd);
> -			continue;
> -		}
> -
> -		err = bpf_object__create_map(obj, map);
> -		if (err)
> -			goto err_out;
> -
> -		pr_debug("map '%s': created successfully, fd=%d\n", map->name,
> -			 map->fd);
> -
> -		if (bpf_map__is_internal(map)) {
> -			err = bpf_object__populate_internal_map(obj, map);
> -			if (err < 0) {
> -				zclose(map->fd);
> +		if (map->fd < 0) {
> +			err = bpf_object__create_map(obj, map);
> +			if (err)
>  				goto err_out;
> -			}
> -		}
> -
> -		if (map->init_slots_sz) {
> -			for (j = 0; j < map->init_slots_sz; j++) {
> -				const struct bpf_map *targ_map;
> -				int fd;
>  
> -				if (!map->init_slots[j])
> -					continue;
> +			pr_debug("map '%s': created successfully, fd=%d\n", map->name,
> +				 map->fd);
>  
> -				targ_map = map->init_slots[j];
> -				fd = bpf_map__fd(targ_map);
> -				err = bpf_map_update_elem(map->fd, &j, &fd, 0);
> -				if (err) {
> -					err = -errno;
> -					pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
> -						map->name, j, targ_map->name,
> -						fd, err);
> +			if (bpf_map__is_internal(map)) {
> +				err = bpf_object__populate_internal_map(obj, map);
> +				if (err < 0) {
> +					zclose(map->fd);
>  					goto err_out;
>  				}
> -				pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
> -					 map->name, j, targ_map->name, fd);
>  			}
> -			zfree(&map->init_slots);
> -			map->init_slots_sz = 0;
> +
> +			if (map->init_slots_sz) {

Couldn't we flatten the code by inverting the logic here and using goto?

	if (!map->init_slot_sz) {
		pr_debug("map '%s': skipping creation (preset fd=%d)\n",
			 map->name, map->fd);
		goto map_pin;
	}

	(...)
map_pin:
	if (map->pin_path && !map->pinned) {

If I'm reading this right.

> +				for (j = 0; j < map->init_slots_sz; j++) {
> +					const struct bpf_map *targ_map;
> +					int fd;
> +
> +					if (!map->init_slots[j])
> +						continue;
> +
> +					targ_map = map->init_slots[j];
> +					fd = bpf_map__fd(targ_map);
> +					err = bpf_map_update_elem(map->fd, &j, &fd, 0);
> +					if (err) {
> +						err = -errno;
> +						pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
> +							map->name, j, targ_map->name,
> +							fd, err);
> +						goto err_out;
> +					}
> +					pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
> +						map->name, j, targ_map->name, fd);
> +				}
> +				zfree(&map->init_slots);
> +				map->init_slots_sz = 0;
> +			}
> +		} else {
> +			pr_debug("map '%s': skipping creation (preset fd=%d)\n",
> +				 map->name, map->fd);
>  		}
>  
>  		if (map->pin_path && !map->pinned) {
> -- 
> 2.25.4
> 

Re: [PATCH libbpf] libbpf: check if pin_path was set even map fd exist

[Andrii Nakryiko]

On Fri, Oct 2, 2020 at 12:58 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Say a user reuse map fd after creating a map manually and set the
> pin_path, then load the object via libbpf.
>
> In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will
> return 0 if there is no pinned map in map->pin_path. Then after
> checking if map fd exist, we should also check if pin_path was set
> and do bpf_map__pin() instead of continue the loop.
>
> Fix it by creating map if fd not exist and continue checking pin_path
> after that.
>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 75 +++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 38 deletions(-)
>

Please add a selftests that validates the logic you are going to rely on.

> +                                       targ_map = map->init_slots[j];
> +                                       fd = bpf_map__fd(targ_map);
> +                                       err = bpf_map_update_elem(map->fd, &j, &fd, 0);
> +                                       if (err) {
> +                                               err = -errno;
> +                                               pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
> +                                                       map->name, j, targ_map->name,
> +                                                       fd, err);

I just noticed that we don't zclose(map->fd) here, can you please fix
it with a separate patch along these changes? Thanks!

> +                                               goto err_out;
> +                                       }
> +                                       pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
> +                                               map->name, j, targ_map->name, fd);
> +                               }
> +                               zfree(&map->init_slots);
> +                               map->init_slots_sz = 0;

Let's move this slot initting logic into a helper function
(init_map_slots() or something like that? doesn't have to use
"bpf_object__" prefix as it is internal static function), that will
simplify overall flow.

> +                       }
> +               } else {
> +                       pr_debug("map '%s': skipping creation (preset fd=%d)\n",
> +                                map->name, map->fd);

to make diff a bit smaller, maybe let's keep the original order, but
do if/else instead of continuing:

if (map->fd >= 0) {
    pr_debug("skipping...");
} else {
   /* do the creation here */
}

/* pinning logic here */

>                 }
>
>                 if (map->pin_path && !map->pinned) {
> --
> 2.25.4
>

Re: [PATCH libbpf] libbpf: check if pin_path was set even map fd exist

[Andrii Nakryiko]

On Fri, Oct 2, 2020 at 4:56 AM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Fri, Oct 02, 2020 at 03:57:50PM +0800, Hangbin Liu wrote:
> > Say a user reuse map fd after creating a map manually and set the
> > pin_path, then load the object via libbpf.
> >
> > In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will
> > return 0 if there is no pinned map in map->pin_path. Then after
> > checking if map fd exist, we should also check if pin_path was set
> > and do bpf_map__pin() instead of continue the loop.
> >
> > Fix it by creating map if fd not exist and continue checking pin_path
> > after that.
> >
> > Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> > ---
> >  tools/lib/bpf/libbpf.c | 75 +++++++++++++++++++++---------------------
> >  1 file changed, 37 insertions(+), 38 deletions(-)
> >

[...]

> > +
> > +                     if (map->init_slots_sz) {
>
> Couldn't we flatten the code by inverting the logic here and using goto?

I explicitly asked Hangbin to not use goto to alter control flow here,
I'd like to keep goto within libbpf mostly for error handling.

>
>         if (!map->init_slot_sz) {
>                 pr_debug("map '%s': skipping creation (preset fd=%d)\n",
>                          map->name, map->fd);
>                 goto map_pin;
>         }
>
>         (...)
> map_pin:
>         if (map->pin_path && !map->pinned) {
>
> If I'm reading this right.
>

[...]

> > --
> > 2.25.4
> >

[PATCHv2 bpf 0/3] Fix pining maps after reuse map fd

[Hangbin Liu]

When a user reuse map fd after creating a map manually and set the
pin_path, then load the object via libbpf. bpf_object__create_maps()
will skip pinning map if map fd exist. Fix it by add moving bpf creation
to else condition and go on checking map pin_path after that.

v2:
a) close map fd if init map slots failed
b) add bpf selftest for this scenario

Hangbin Liu (3):
  libbpf: close map fd if init map slots failed
  libbpf: check if pin_path was set even map fd exist
  selftest/bpf: test pinning map with reused map fd

 tools/lib/bpf/libbpf.c                        | 80 +++++++++++--------
 .../selftests/bpf/prog_tests/pinning.c        | 46 ++++++++++-
 2 files changed, 91 insertions(+), 35 deletions(-)

-- 
2.25.4

[PATCHv2 bpf 1/3] libbpf: close map fd if init map slots failed

[Hangbin Liu]

Previously we forgot to close the map fd if bpf_map_update_elem()
failed during map slot init, which will leak map fd.

Let's move map slot initialization to new function init_map_slots() to
simplify the code. And close the map fd if init slot failed.

Reported-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/lib/bpf/libbpf.c | 55 ++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 32dc444224d8..c8b5fe45682d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4192,6 +4192,36 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map)
 	return 0;
 }
 
+static int init_map_slots(struct bpf_map *map)
+{
+	const struct bpf_map *targ_map;
+	unsigned int i;
+	int fd, err;
+
+	for (i = 0; i < map->init_slots_sz; i++) {
+		if (!map->init_slots[i])
+			continue;
+
+		targ_map = map->init_slots[i];
+		fd = bpf_map__fd(targ_map);
+		err = bpf_map_update_elem(map->fd, &i, &fd, 0);
+		if (err) {
+			err = -errno;
+			pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
+				map->name, i, targ_map->name,
+				fd, err);
+			return err;
+		}
+		pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
+			 map->name, i, targ_map->name, fd);
+	}
+
+	zfree(&map->init_slots);
+	map->init_slots_sz = 0;
+
+	return 0;
+}
+
 static int
 bpf_object__create_maps(struct bpf_object *obj)
 {
@@ -4234,28 +4264,11 @@ bpf_object__create_maps(struct bpf_object *obj)
 		}
 
 		if (map->init_slots_sz) {
-			for (j = 0; j < map->init_slots_sz; j++) {
-				const struct bpf_map *targ_map;
-				int fd;
-
-				if (!map->init_slots[j])
-					continue;
-
-				targ_map = map->init_slots[j];
-				fd = bpf_map__fd(targ_map);
-				err = bpf_map_update_elem(map->fd, &j, &fd, 0);
-				if (err) {
-					err = -errno;
-					pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
-						map->name, j, targ_map->name,
-						fd, err);
-					goto err_out;
-				}
-				pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
-					 map->name, j, targ_map->name, fd);
+			err = init_map_slots(map);
+			if (err < 0) {
+				zclose(map->fd);
+				goto err_out;
 			}
-			zfree(&map->init_slots);
-			map->init_slots_sz = 0;
 		}
 
 		if (map->pin_path && !map->pinned) {
-- 
2.25.4

[PATCHv2 bpf 2/3] libbpf: check if pin_path was set even map fd exist

[Hangbin Liu]

Say a user reuse map fd after creating a map manually and set the
pin_path, then load the object via libbpf.

In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will
return 0 if there is no pinned map in map->pin_path. Then after
checking if map fd exist, we should also check if pin_path was set
and do bpf_map__pin() instead of continue the loop.

Fix it by creating map if fd not exist and continue checking pin_path
after that.

v2: keep if condition with existing order

Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/lib/bpf/libbpf.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index c8b5fe45682d..1fb3fd733b23 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4245,29 +4245,28 @@ bpf_object__create_maps(struct bpf_object *obj)
 		if (map->fd >= 0) {
 			pr_debug("map '%s': skipping creation (preset fd=%d)\n",
 				 map->name, map->fd);
-			continue;
-		}
-
-		err = bpf_object__create_map(obj, map);
-		if (err)
-			goto err_out;
+		} else {
+			err = bpf_object__create_map(obj, map);
+			if (err)
+				goto err_out;
 
-		pr_debug("map '%s': created successfully, fd=%d\n", map->name,
-			 map->fd);
+			pr_debug("map '%s': created successfully, fd=%d\n",
+				 map->name, map->fd);
 
-		if (bpf_map__is_internal(map)) {
-			err = bpf_object__populate_internal_map(obj, map);
-			if (err < 0) {
-				zclose(map->fd);
-				goto err_out;
+			if (bpf_map__is_internal(map)) {
+				err = bpf_object__populate_internal_map(obj, map);
+				if (err < 0) {
+					zclose(map->fd);
+					goto err_out;
+				}
 			}
-		}
 
-		if (map->init_slots_sz) {
-			err = init_map_slots(map);
-			if (err < 0) {
-				zclose(map->fd);
-				goto err_out;
+			if (map->init_slots_sz) {
+				err = init_map_slots(map);
+				if (err < 0) {
+					zclose(map->fd);
+					goto err_out;
+				}
 			}
 		}
 
-- 
2.25.4

[PATCHv2 bpf 3/3] selftest/bpf: test pinning map with reused map fd

[Hangbin Liu]

This add a test to make sure that we can still pin maps with
reused map fd.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 .../selftests/bpf/prog_tests/pinning.c        | 46 ++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/pinning.c b/tools/testing/selftests/bpf/prog_tests/pinning.c
index 041952524c55..299f99ef92b2 100644
--- a/tools/testing/selftests/bpf/prog_tests/pinning.c
+++ b/tools/testing/selftests/bpf/prog_tests/pinning.c
@@ -37,7 +37,7 @@ void test_pinning(void)
 	struct stat statbuf = {};
 	struct bpf_object *obj;
 	struct bpf_map *map;
-	int err;
+	int err, map_fd;
 	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
 		.pin_root_path = custpath,
 	);
@@ -213,6 +213,50 @@ void test_pinning(void)
 	if (CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno))
 		goto out;
 
+	/* remove the custom pin path to re-test it with reuse fd below */
+	err = unlink(custpinpath);
+	if (CHECK(err, "unlink custpinpath", "err %d errno %d\n", err, errno))
+		goto out;
+
+	err = rmdir(custpath);
+	if (CHECK(err, "rmdir custpindir", "err %d errno %d\n", err, errno))
+		goto out;
+
+	bpf_object__close(obj);
+
+	/* test pinning at custom path with reuse fd */
+	obj = bpf_object__open_file(file, NULL);
+	if (CHECK_FAIL(libbpf_get_error(obj))) {
+		obj = NULL;
+		goto out;
+	}
+
+	map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(__u32),
+				sizeof(__u64), 1, 0);
+	if (CHECK(map_fd < 0, "create pinmap manually", "fd %d\n", map_fd))
+		goto out;
+
+	map = bpf_object__find_map_by_name(obj, "pinmap");
+	if (CHECK(!map, "find map", "NULL map"))
+		goto out;
+
+	err = bpf_map__reuse_fd(map, map_fd);
+	if (CHECK(err, "reuse pinmap fd", "err %d errno %d\n", err, errno))
+		goto out;
+
+	err = bpf_map__set_pin_path(map, custpinpath);
+	if (CHECK(err, "set pin path", "err %d errno %d\n", err, errno))
+		goto out;
+
+	err = bpf_object__load(obj);
+	if (CHECK(err, "custom load", "err %d errno %d\n", err, errno))
+		goto out;
+
+	/* check that pinmap was pinned at the custom path */
+	err = stat(custpinpath, &statbuf);
+	if (CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno))
+		goto out;
+
 out:
 	unlink(pinpath);
 	unlink(nopinpath);
-- 
2.25.4

Re: [PATCHv2 bpf 1/3] libbpf: close map fd if init map slots failed

[Andrii Nakryiko]

On Sat, Oct 3, 2020 at 1:55 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Previously we forgot to close the map fd if bpf_map_update_elem()
> failed during map slot init, which will leak map fd.
>
> Let's move map slot initialization to new function init_map_slots() to
> simplify the code. And close the map fd if init slot failed.
>
> Reported-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---

LGTM, thanks for the fix!

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  tools/lib/bpf/libbpf.c | 55 ++++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 21 deletions(-)
>

[...]

Re: [PATCHv2 bpf 2/3] libbpf: check if pin_path was set even map fd exist

[Andrii Nakryiko]

On Sat, Oct 3, 2020 at 1:55 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> Say a user reuse map fd after creating a map manually and set the
> pin_path, then load the object via libbpf.
>
> In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will
> return 0 if there is no pinned map in map->pin_path. Then after
> checking if map fd exist, we should also check if pin_path was set
> and do bpf_map__pin() instead of continue the loop.
>
> Fix it by creating map if fd not exist and continue checking pin_path
> after that.
>
> v2: keep if condition with existing order
>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  tools/lib/bpf/libbpf.c | 37 ++++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)
>

[...]

Re: [PATCHv2 bpf 3/3] selftest/bpf: test pinning map with reused map fd

[Andrii Nakryiko]

On Sat, Oct 3, 2020 at 1:55 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> This add a test to make sure that we can still pin maps with
> reused map fd.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/pinning.c        | 46 ++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/pinning.c b/tools/testing/selftests/bpf/prog_tests/pinning.c
> index 041952524c55..299f99ef92b2 100644
> --- a/tools/testing/selftests/bpf/prog_tests/pinning.c
> +++ b/tools/testing/selftests/bpf/prog_tests/pinning.c
> @@ -37,7 +37,7 @@ void test_pinning(void)
>         struct stat statbuf = {};
>         struct bpf_object *obj;
>         struct bpf_map *map;
> -       int err;
> +       int err, map_fd;
>         DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
>                 .pin_root_path = custpath,
>         );
> @@ -213,6 +213,50 @@ void test_pinning(void)
>         if (CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno))
>                 goto out;
>
> +       /* remove the custom pin path to re-test it with reuse fd below */
> +       err = unlink(custpinpath);
> +       if (CHECK(err, "unlink custpinpath", "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +       err = rmdir(custpath);
> +       if (CHECK(err, "rmdir custpindir", "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +       bpf_object__close(obj);
> +
> +       /* test pinning at custom path with reuse fd */
> +       obj = bpf_object__open_file(file, NULL);
> +       if (CHECK_FAIL(libbpf_get_error(obj))) {

please use CHECK, might try new ASSERT_OK_PTR(obj, "obj_open") as well

> +               obj = NULL;
> +               goto out;
> +       }
> +
> +       map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(__u32),
> +                               sizeof(__u64), 1, 0);
> +       if (CHECK(map_fd < 0, "create pinmap manually", "fd %d\n", map_fd))
> +               goto out;
> +
> +       map = bpf_object__find_map_by_name(obj, "pinmap");
> +       if (CHECK(!map, "find map", "NULL map"))

here and below you are not closing map_fd on error

> +               goto out;
> +
> +       err = bpf_map__reuse_fd(map, map_fd);
> +       if (CHECK(err, "reuse pinmap fd", "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +       err = bpf_map__set_pin_path(map, custpinpath);
> +       if (CHECK(err, "set pin path", "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +       err = bpf_object__load(obj);
> +       if (CHECK(err, "custom load", "err %d errno %d\n", err, errno))
> +               goto out;
> +
> +       /* check that pinmap was pinned at the custom path */
> +       err = stat(custpinpath, &statbuf);
> +       if (CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno))
> +               goto out;
> +
>  out:
>         unlink(pinpath);
>         unlink(nopinpath);
> --
> 2.25.4
>

[PATCHv3 bpf 0/3] Fix pining maps after reuse map fd

[Hangbin Liu]

When a user reuse map fd after creating a map manually and set the
pin_path, then load the object via libbpf. bpf_object__create_maps()
will skip pinning map if map fd exist. Fix it by add moving bpf creation
to else condition and go on checking map pin_path after that.

v3:
for selftest: use CHECK() for bpf_object__open_file() and close map fd on error

v2:
a) close map fd if init map slots failed
b) add bpf selftest for this scenario


Hangbin Liu (3):
  libbpf: close map fd if init map slots failed
  libbpf: check if pin_path was set even map fd exist
  selftest/bpf: test pinning map with reused map fd

 tools/lib/bpf/libbpf.c                        | 80 +++++++++++--------
 .../selftests/bpf/prog_tests/pinning.c        | 49 +++++++++++-
 2 files changed, 94 insertions(+), 35 deletions(-)

-- 
2.25.4

[PATCHv3 bpf 1/3] libbpf: close map fd if init map slots failed

[Hangbin Liu]

Previously we forgot to close the map fd if bpf_map_update_elem()
failed during map slot init, which will leak map fd.

Let's move map slot initialization to new function init_map_slots() to
simplify the code. And close the map fd if init slot failed.

Reported-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/lib/bpf/libbpf.c | 55 ++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 32dc444224d8..c8b5fe45682d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4192,6 +4192,36 @@ static int bpf_object__create_map(struct bpf_object *obj, struct bpf_map *map)
 	return 0;
 }
 
+static int init_map_slots(struct bpf_map *map)
+{
+	const struct bpf_map *targ_map;
+	unsigned int i;
+	int fd, err;
+
+	for (i = 0; i < map->init_slots_sz; i++) {
+		if (!map->init_slots[i])
+			continue;
+
+		targ_map = map->init_slots[i];
+		fd = bpf_map__fd(targ_map);
+		err = bpf_map_update_elem(map->fd, &i, &fd, 0);
+		if (err) {
+			err = -errno;
+			pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
+				map->name, i, targ_map->name,
+				fd, err);
+			return err;
+		}
+		pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
+			 map->name, i, targ_map->name, fd);
+	}
+
+	zfree(&map->init_slots);
+	map->init_slots_sz = 0;
+
+	return 0;
+}
+
 static int
 bpf_object__create_maps(struct bpf_object *obj)
 {
@@ -4234,28 +4264,11 @@ bpf_object__create_maps(struct bpf_object *obj)
 		}
 
 		if (map->init_slots_sz) {
-			for (j = 0; j < map->init_slots_sz; j++) {
-				const struct bpf_map *targ_map;
-				int fd;
-
-				if (!map->init_slots[j])
-					continue;
-
-				targ_map = map->init_slots[j];
-				fd = bpf_map__fd(targ_map);
-				err = bpf_map_update_elem(map->fd, &j, &fd, 0);
-				if (err) {
-					err = -errno;
-					pr_warn("map '%s': failed to initialize slot [%d] to map '%s' fd=%d: %d\n",
-						map->name, j, targ_map->name,
-						fd, err);
-					goto err_out;
-				}
-				pr_debug("map '%s': slot [%d] set to map '%s' fd=%d\n",
-					 map->name, j, targ_map->name, fd);
+			err = init_map_slots(map);
+			if (err < 0) {
+				zclose(map->fd);
+				goto err_out;
 			}
-			zfree(&map->init_slots);
-			map->init_slots_sz = 0;
 		}
 
 		if (map->pin_path && !map->pinned) {
-- 
2.25.4

[PATCHv3 bpf 2/3] libbpf: check if pin_path was set even map fd exist

[Hangbin Liu]

Say a user reuse map fd after creating a map manually and set the
pin_path, then load the object via libbpf.

In libbpf bpf_object__create_maps(), bpf_object__reuse_map() will
return 0 if there is no pinned map in map->pin_path. Then after
checking if map fd exist, we should also check if pin_path was set
and do bpf_map__pin() instead of continue the loop.

Fix it by creating map if fd not exist and continue checking pin_path
after that.

Suggested-by: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v3: no update
v2: keep if condition with existing order
---
 tools/lib/bpf/libbpf.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index c8b5fe45682d..1fb3fd733b23 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4245,29 +4245,28 @@ bpf_object__create_maps(struct bpf_object *obj)
 		if (map->fd >= 0) {
 			pr_debug("map '%s': skipping creation (preset fd=%d)\n",
 				 map->name, map->fd);
-			continue;
-		}
-
-		err = bpf_object__create_map(obj, map);
-		if (err)
-			goto err_out;
+		} else {
+			err = bpf_object__create_map(obj, map);
+			if (err)
+				goto err_out;
 
-		pr_debug("map '%s': created successfully, fd=%d\n", map->name,
-			 map->fd);
+			pr_debug("map '%s': created successfully, fd=%d\n",
+				 map->name, map->fd);
 
-		if (bpf_map__is_internal(map)) {
-			err = bpf_object__populate_internal_map(obj, map);
-			if (err < 0) {
-				zclose(map->fd);
-				goto err_out;
+			if (bpf_map__is_internal(map)) {
+				err = bpf_object__populate_internal_map(obj, map);
+				if (err < 0) {
+					zclose(map->fd);
+					goto err_out;
+				}
 			}
-		}
 
-		if (map->init_slots_sz) {
-			err = init_map_slots(map);
-			if (err < 0) {
-				zclose(map->fd);
-				goto err_out;
+			if (map->init_slots_sz) {
+				err = init_map_slots(map);
+				if (err < 0) {
+					zclose(map->fd);
+					goto err_out;
+				}
 			}
 		}
 
-- 
2.25.4

[PATCHv3 bpf 3/3] selftest/bpf: test pinning map with reused map fd

[Hangbin Liu]

This add a test to make sure that we can still pin maps with
reused map fd.

Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
v3: use CHECK() for bpf_object__open_file() and close map fd on error
v2: no update
---
 .../selftests/bpf/prog_tests/pinning.c        | 49 ++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/pinning.c b/tools/testing/selftests/bpf/prog_tests/pinning.c
index 041952524c55..fcf54b3a1dd0 100644
--- a/tools/testing/selftests/bpf/prog_tests/pinning.c
+++ b/tools/testing/selftests/bpf/prog_tests/pinning.c
@@ -37,7 +37,7 @@ void test_pinning(void)
 	struct stat statbuf = {};
 	struct bpf_object *obj;
 	struct bpf_map *map;
-	int err;
+	int err, map_fd;
 	DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
 		.pin_root_path = custpath,
 	);
@@ -213,6 +213,53 @@ void test_pinning(void)
 	if (CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno))
 		goto out;
 
+	/* remove the custom pin path to re-test it with reuse fd below */
+	err = unlink(custpinpath);
+	if (CHECK(err, "unlink custpinpath", "err %d errno %d\n", err, errno))
+		goto out;
+
+	err = rmdir(custpath);
+	if (CHECK(err, "rmdir custpindir", "err %d errno %d\n", err, errno))
+		goto out;
+
+	bpf_object__close(obj);
+
+	/* test pinning at custom path with reuse fd */
+	obj = bpf_object__open_file(file, NULL);
+	err = libbpf_get_error(obj);
+	if (CHECK(err, "default open", "err %d errno %d\n", err, errno)) {
+		obj = NULL;
+		goto out;
+	}
+
+	map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(__u32),
+				sizeof(__u64), 1, 0);
+	if (CHECK(map_fd < 0, "create pinmap manually", "fd %d\n", map_fd))
+		goto out;
+
+	map = bpf_object__find_map_by_name(obj, "pinmap");
+	if (CHECK(!map, "find map", "NULL map"))
+		goto close_map_fd;
+
+	err = bpf_map__reuse_fd(map, map_fd);
+	if (CHECK(err, "reuse pinmap fd", "err %d errno %d\n", err, errno))
+		goto close_map_fd;
+
+	err = bpf_map__set_pin_path(map, custpinpath);
+	if (CHECK(err, "set pin path", "err %d errno %d\n", err, errno))
+		goto close_map_fd;
+
+	err = bpf_object__load(obj);
+	if (CHECK(err, "custom load", "err %d errno %d\n", err, errno))
+		goto close_map_fd;
+
+	/* check that pinmap was pinned at the custom path */
+	err = stat(custpinpath, &statbuf);
+	if (CHECK(err, "stat custpinpath", "err %d errno %d\n", err, errno))
+		goto close_map_fd;
+
+close_map_fd:
+	close(map_fd);
 out:
 	unlink(pinpath);
 	unlink(nopinpath);
-- 
2.25.4

Re: [PATCHv3 bpf 3/3] selftest/bpf: test pinning map with reused map fd

[Andrii Nakryiko]

On Mon, Oct 5, 2020 at 7:15 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> This add a test to make sure that we can still pin maps with
> reused map fd.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>

[...]

Re: [PATCHv3 bpf 0/3] Fix pining maps after reuse map fd

[patchwork-bot+bpf]

Hello:

This series was applied to bpf/bpf-next.git (refs/heads/master):

On Tue,  6 Oct 2020 10:13:42 +0800 you wrote:
> When a user reuse map fd after creating a map manually and set the
> pin_path, then load the object via libbpf. bpf_object__create_maps()
> will skip pinning map if map fd exist. Fix it by add moving bpf creation
> to else condition and go on checking map pin_path after that.
> 
> v3:
> for selftest: use CHECK() for bpf_object__open_file() and close map fd on error
> 
> [...]

Here is the summary with links:
  - [PATCHv3,1/3] libbpf: close map fd if init map slots failed
    https://git.kernel.org/bpf/bpf-next/c/a0f2b7acb4b1
  - [PATCHv3,2/3] libbpf: check if pin_path was set even map fd exist
    https://git.kernel.org/bpf/bpf-next/c/2c193d32caee
  - [PATCHv3,3/3] selftest/bpf: test pinning map with reused map fd
    https://git.kernel.org/bpf/bpf-next/c/44c4aa2bd151

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html