diff mbox series

[bpf] libbpf: sanitise map names before pinning

Message ID 20201130161720.8688-1-toke@redhat.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf] libbpf: sanitise map names before pinning | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: From:/Signed-off-by: email name mismatch: 'From: "Toke Høiland-Jørgensen" <toke@redhat.com>' != 'Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>' WARNING: suspect code indent for conditional statements (24, 28)
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Toke Høiland-Jørgensen Nov. 30, 2020, 4:17 p.m. UTC
When we added sanitising of map names before loading programs to libbpf, we
still allowed periods in the name. While the kernel will accept these for
the map names themselves, they are not allowed in file names when pinning
maps. This means that bpf_object__pin_maps() will fail if called on an
object that contains internal maps (such as sections .rodata).

Fix this by replacing periods with underscores when constructing map pin
paths. This only affects the paths generated by libbpf when
bpf_object__ping_maps() is called with a path argument. Any pin paths set
by bpf_map__set_pin_path() are unaffected, and it will still be up to the
caller to avoid invalid characters in those.

Fixes: 113e6b7e15e2 ("libbpf: Sanitise internal map names so they are not rejected by the kernel")
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/libbpf.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Andrii Nakryiko Dec. 2, 2020, 12:39 a.m. UTC | #1
On Mon, Nov 30, 2020 at 8:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> When we added sanitising of map names before loading programs to libbpf, we
> still allowed periods in the name. While the kernel will accept these for
> the map names themselves, they are not allowed in file names when pinning

That sounds like an unnecessary difference in kernel behavior. If the
kernel allows maps with '.' in the name, why not allow to pin it?
Should we fix that in the kernel?

> maps. This means that bpf_object__pin_maps() will fail if called on an
> object that contains internal maps (such as sections .rodata).
>
> Fix this by replacing periods with underscores when constructing map pin
> paths. This only affects the paths generated by libbpf when
> bpf_object__ping_maps() is called with a path argument. Any pin paths set
> by bpf_map__set_pin_path() are unaffected, and it will still be up to the
> caller to avoid invalid characters in those.
>
> Fixes: 113e6b7e15e2 ("libbpf: Sanitise internal map names so they are not rejected by the kernel")
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  tools/lib/bpf/libbpf.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 8d05132e1945..8a3b4713b356 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -7665,8 +7665,8 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
>         }
>
>         bpf_object__for_each_map(map, obj) {
> +               char buf[PATH_MAX], *s = buf;
>                 char *pin_path = NULL;
> -               char buf[PATH_MAX];
>
>                 if (path) {
>                         int len;
> @@ -7680,6 +7680,8 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
>                                 err = -ENAMETOOLONG;
>                                 goto err_unpin_maps;
>                         }
> +                       while ((s = strstr(s, ".")))
> +                           *s = '_';

Let's extract this into a helper method?

>                         pin_path = buf;
>                 } else if (!map->pin_path) {
>                         continue;
> @@ -7712,8 +7714,8 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
>                 return -ENOENT;
>
>         bpf_object__for_each_map(map, obj) {
> +               char buf[PATH_MAX], *s = buf;
>                 char *pin_path = NULL;
> -               char buf[PATH_MAX];
>
>                 if (path) {
>                         int len;
> @@ -7724,6 +7726,8 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
>                                 return -EINVAL;
>                         else if (len >= PATH_MAX)
>                                 return -ENAMETOOLONG;
> +                       while ((s = strstr(s, ".")))
> +                           *s = '_';
>                         pin_path = buf;
>                 } else if (!map->pin_path) {
>                         continue;
> --
> 2.29.2
>
Toke Høiland-Jørgensen Dec. 2, 2020, 9:55 a.m. UTC | #2
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Mon, Nov 30, 2020 at 8:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> When we added sanitising of map names before loading programs to libbpf, we
>> still allowed periods in the name. While the kernel will accept these for
>> the map names themselves, they are not allowed in file names when pinning
>
> That sounds like an unnecessary difference in kernel behavior. If the
> kernel allows maps with '.' in the name, why not allow to pin it?
> Should we fix that in the kernel?

Yeah, it is a bit odd. I always assumed the restriction in file names is
to prevent people from creating hidden (.-prefixed) files in bpffs? But
don't actually know for sure. Anyway, if that is the case we could still
allow periods in the middle of names.

I'm certainly not opposed to changing the kernel behaviour and I can
follow up with a patch for this if others agree; but we obviously still
need this for older kernels so I'll send a v2 with the helper method you
suggested below.

-Toke
diff mbox series

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 8d05132e1945..8a3b4713b356 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -7665,8 +7665,8 @@  int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
 	}
 
 	bpf_object__for_each_map(map, obj) {
+		char buf[PATH_MAX], *s = buf;
 		char *pin_path = NULL;
-		char buf[PATH_MAX];
 
 		if (path) {
 			int len;
@@ -7680,6 +7680,8 @@  int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
 				err = -ENAMETOOLONG;
 				goto err_unpin_maps;
 			}
+			while ((s = strstr(s, ".")))
+			    *s = '_';
 			pin_path = buf;
 		} else if (!map->pin_path) {
 			continue;
@@ -7712,8 +7714,8 @@  int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
 		return -ENOENT;
 
 	bpf_object__for_each_map(map, obj) {
+		char buf[PATH_MAX], *s = buf;
 		char *pin_path = NULL;
-		char buf[PATH_MAX];
 
 		if (path) {
 			int len;
@@ -7724,6 +7726,8 @@  int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
 				return -EINVAL;
 			else if (len >= PATH_MAX)
 				return -ENAMETOOLONG;
+			while ((s = strstr(s, ".")))
+			    *s = '_';
 			pin_path = buf;
 		} else if (!map->pin_path) {
 			continue;