| Message ID | 20201202111816.92279-1-toke@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,v2] libbpf: sanitise map names before pinning | expand |
| 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>' |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, Dec 2, 2020 at 3:19 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 > 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> > --- > v2: > - Move string munging to helper function > > tools/lib/bpf/libbpf.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 8d05132e1945..08ff7783fb93 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -7651,6 +7651,20 @@ bool bpf_map__is_pinned(const struct bpf_map *map) > return map->pinned; > } > > +static char *sanitize_pin_path(char *str) don't want to be unnecessarily nitpicky, but the return of char * suggests that this function might be allocating new string, so it's a bit misleading. doing void function and having non-const char *str feels most appropriate for this. Nice side-benefit: the implementation will be even shorter :) > +{ > + char *s = str; > + > + /* bpffs disallows periods in path names */ > + while (*s) { > + if (*s == '.') > + *s = '_'; > + s++; > + } > + > + return str; > +} > + > int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > { > struct bpf_map *map; > @@ -7680,7 +7694,7 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > err = -ENAMETOOLONG; > goto err_unpin_maps; > } > - pin_path = buf; > + pin_path = sanitize_pin_path(buf); > } else if (!map->pin_path) { > continue; > } > @@ -7724,7 +7738,7 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > return -EINVAL; > else if (len >= PATH_MAX) > return -ENAMETOOLONG; > - pin_path = buf; > + pin_path = sanitize_pin_path(buf); > } else if (!map->pin_path) { > continue; > } > -- > 2.29.2 >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Wed, Dec 2, 2020 at 3:19 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 >> 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> >> --- >> v2: >> - Move string munging to helper function >> >> tools/lib/bpf/libbpf.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 8d05132e1945..08ff7783fb93 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -7651,6 +7651,20 @@ bool bpf_map__is_pinned(const struct bpf_map *map) >> return map->pinned; >> } >> >> +static char *sanitize_pin_path(char *str) > > don't want to be unnecessarily nitpicky, but the return of char * > suggests that this function might be allocating new string, so it's a > bit misleading. doing void function and having non-const char *str > feels most appropriate for this. Nice side-benefit: the implementation > will be even shorter :) Hmm, fair enough. I added the return because I figured it was convenient to fold the call into the assignment of pin_path, but I don't have a strong preference either way; will send a v3 :) -Toke
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 8d05132e1945..08ff7783fb93 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -7651,6 +7651,20 @@ bool bpf_map__is_pinned(const struct bpf_map *map) return map->pinned; } +static char *sanitize_pin_path(char *str) +{ + char *s = str; + + /* bpffs disallows periods in path names */ + while (*s) { + if (*s == '.') + *s = '_'; + s++; + } + + return str; +} + int bpf_object__pin_maps(struct bpf_object *obj, const char *path) { struct bpf_map *map; @@ -7680,7 +7694,7 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) err = -ENAMETOOLONG; goto err_unpin_maps; } - pin_path = buf; + pin_path = sanitize_pin_path(buf); } else if (!map->pin_path) { continue; } @@ -7724,7 +7738,7 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) return -EINVAL; else if (len >= PATH_MAX) return -ENAMETOOLONG; - pin_path = buf; + pin_path = sanitize_pin_path(buf); } else if (!map->pin_path) { continue; }
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> --- v2: - Move string munging to helper function tools/lib/bpf/libbpf.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)