[PATCH bpf] libbpf: Sanitise internal map names so they are not rejected by the kernel

The kernel only accepts map names with alphanumeric characters, underscores
and periods in their name. However, the auto-generated internal map names
used by libbpf takes their prefix from the user-supplied BPF object name,
which has no such restriction. This can lead to "Invalid argument" errors
when trying to load a BPF program using global variables.

Fix this by sanitising the map names, replacing any non-allowed characters
with underscores.

Fixes: d859900c4c56 ("bpf, libbpf: support global data/bss/rodata sections")
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 tools/lib/bpf/libbpf.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 514b1a524abb..7469c7dcc15e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c

@@ -24,6 +24,7 @@
 #include <endian.h>
 #include <fcntl.h>
 #include <errno.h>
+#include <ctype.h>
 #include <asm/unistd.h>
 #include <linux/err.h>
 #include <linux/kernel.h>

@@ -1283,7 +1284,7 @@ static size_t bpf_map_mmap_sz(const struct bpf_map *map)
 static char *internal_map_name(struct bpf_object *obj,
 			       enum libbpf_map_type type)
 {
-	char map_name[BPF_OBJ_NAME_LEN];
+	char map_name[BPF_OBJ_NAME_LEN], *p;
 	const char *sfx = libbpf_type_to_btf_name[type];
 	int sfx_len = max((size_t)7, strlen(sfx));
 	int pfx_len = min((size_t)BPF_OBJ_NAME_LEN - sfx_len - 1,

@@ -1292,6 +1293,11 @@ static char *internal_map_name(struct bpf_object *obj,
 	snprintf(map_name, sizeof(map_name), "%.*s%.*s", pfx_len, obj->name,
 		 sfx_len, libbpf_type_to_btf_name[type]);
 
+	/* sanitise map name to characters allowed by kernel */
+	for (p = map_name; *p && p < map_name + sizeof(map_name); p++)
+		if (!isalnum(*p) && *p != '_' && *p != '.')
+			*p = '_';
+
 	return strdup(map_name);
 }
 

-- 
2.25.0

On 2/17/20 6:17 PM, Toke Høiland-Jørgensen wrote:

The kernel only accepts map names with alphanumeric characters, underscores
and periods in their name. However, the auto-generated internal map names
used by libbpf takes their prefix from the user-supplied BPF object name,
which has no such restriction. This can lead to "Invalid argument" errors
when trying to load a BPF program using global variables.

Fix this by sanitising the map names, replacing any non-allowed characters
with underscores.

Fixes: d859900c4c56 ("bpf, libbpf: support global data/bss/rodata sections")
Signed-off-by: Toke Høiland-Jørgensen <redacted>

Makes sense to me, applied, thanks! I presume you had something like '-' in the
global var leading to rejection?

On 2/18/20 6:40 AM, Daniel Borkmann wrote:

On 2/17/20 6:17 PM, Toke Høiland-Jørgensen wrote:

quoted

The kernel only accepts map names with alphanumeric characters, 
underscores
and periods in their name. However, the auto-generated internal map names
used by libbpf takes their prefix from the user-supplied BPF object name,
which has no such restriction. This can lead to "Invalid argument" errors
when trying to load a BPF program using global variables.

Fix this by sanitising the map names, replacing any non-allowed 
characters
with underscores.

Fixes: d859900c4c56 ("bpf, libbpf: support global data/bss/rodata 
sections")
Signed-off-by: Toke Høiland-Jørgensen <redacted>

Makes sense to me, applied, thanks! I presume you had something like '-' 
in the
global var leading to rejection?

The C global variable cannot have '-'. I saw a complain in bcc mailing 
list sometimes back like: if an object file is a-b.o, then we will 
generate a map name like a-b.bss for the bss ELF section data. The
map name "a-b.bss" name will be rejected by the kernel. The workaround
is to change object file name. Not sure whether this is the only
issue which may introduce non [a-zA-Z0-9_] or not. But this patch indeed 
should fix the issue I just described.

Yonghong Song [off-list ref] writes:

On 2/18/20 6:40 AM, Daniel Borkmann wrote:

quoted

On 2/17/20 6:17 PM, Toke Høiland-Jørgensen wrote:

quoted

The kernel only accepts map names with alphanumeric characters, 
underscores
and periods in their name. However, the auto-generated internal map names
used by libbpf takes their prefix from the user-supplied BPF object name,
which has no such restriction. This can lead to "Invalid argument" errors
when trying to load a BPF program using global variables.

Fix this by sanitising the map names, replacing any non-allowed 
characters
with underscores.

Fixes: d859900c4c56 ("bpf, libbpf: support global data/bss/rodata 
sections")
Signed-off-by: Toke Høiland-Jørgensen <redacted>

Makes sense to me, applied, thanks! I presume you had something like '-' 
in the
global var leading to rejection?

The C global variable cannot have '-'. I saw a complain in bcc mailing 
list sometimes back like: if an object file is a-b.o, then we will 
generate a map name like a-b.bss for the bss ELF section data. The
map name "a-b.bss" name will be rejected by the kernel. The workaround
is to change object file name. Not sure whether this is the only
issue which may introduce non [a-zA-Z0-9_] or not. But this patch indeed 
should fix the issue I just described.

Yes, this was exactly my problem; my object file is called
'xdp-dispatcher.o'. Fun error to track down :P

Why doesn't the kernel allow dashes in the name anyway?

-Toke

On 2/18/20 5:42 PM, Toke Høiland-Jørgensen wrote:

Yonghong Song [off-list ref] writes:

quoted

On 2/18/20 6:40 AM, Daniel Borkmann wrote:

quoted

On 2/17/20 6:17 PM, Toke Høiland-Jørgensen wrote:

quoted

The kernel only accepts map names with alphanumeric characters,
underscores
and periods in their name. However, the auto-generated internal map names
used by libbpf takes their prefix from the user-supplied BPF object name,
which has no such restriction. This can lead to "Invalid argument" errors
when trying to load a BPF program using global variables.

Fix this by sanitising the map names, replacing any non-allowed
characters
with underscores.

Fixes: d859900c4c56 ("bpf, libbpf: support global data/bss/rodata
sections")
Signed-off-by: Toke Høiland-Jørgensen <redacted>

Makes sense to me, applied, thanks! I presume you had something like '-'
in the
global var leading to rejection?

The C global variable cannot have '-'. I saw a complain in bcc mailing
list sometimes back like: if an object file is a-b.o, then we will
generate a map name like a-b.bss for the bss ELF section data. The
map name "a-b.bss" name will be rejected by the kernel. The workaround
is to change object file name. Not sure whether this is the only
issue which may introduce non [a-zA-Z0-9_] or not. But this patch indeed
should fix the issue I just described.

Yep, meant object file name, just realized too late after sending. :/

Yes, this was exactly my problem; my object file is called
'xdp-dispatcher.o'. Fun error to track down :P

Why doesn't the kernel allow dashes in the name anyway?

Commit cb4d2b3f03d8 ("bpf: Add name, load_time, uid and map_ids to bpf_prog_info")
doesn't state a specific reason, and we did later extend it via 3e0ddc4f3ff1 ("bpf:
allow . char as part of the object name"). My best guess right now is potentially
not to confuse BPF's kallsyms handling with dashes etc.

Daniel Borkmann [off-list ref] writes:

On 2/18/20 5:42 PM, Toke Høiland-Jørgensen wrote:

quoted

Yonghong Song [off-list ref] writes:

quoted

On 2/18/20 6:40 AM, Daniel Borkmann wrote:

quoted

On 2/17/20 6:17 PM, Toke Høiland-Jørgensen wrote:

quoted

The kernel only accepts map names with alphanumeric characters,
underscores
and periods in their name. However, the auto-generated internal map names
used by libbpf takes their prefix from the user-supplied BPF object name,
which has no such restriction. This can lead to "Invalid argument" errors
when trying to load a BPF program using global variables.

Fix this by sanitising the map names, replacing any non-allowed
characters
with underscores.

Fixes: d859900c4c56 ("bpf, libbpf: support global data/bss/rodata
sections")
Signed-off-by: Toke Høiland-Jørgensen <redacted>

Makes sense to me, applied, thanks! I presume you had something like '-'
in the
global var leading to rejection?

The C global variable cannot have '-'. I saw a complain in bcc mailing
list sometimes back like: if an object file is a-b.o, then we will
generate a map name like a-b.bss for the bss ELF section data. The
map name "a-b.bss" name will be rejected by the kernel. The workaround
is to change object file name. Not sure whether this is the only
issue which may introduce non [a-zA-Z0-9_] or not. But this patch indeed
should fix the issue I just described.

Yep, meant object file name, just realized too late after sending. :/

quoted

Yes, this was exactly my problem; my object file is called
'xdp-dispatcher.o'. Fun error to track down :P

Why doesn't the kernel allow dashes in the name anyway?

Commit cb4d2b3f03d8 ("bpf: Add name, load_time, uid and map_ids to bpf_prog_info")
doesn't state a specific reason, and we did later extend it via 3e0ddc4f3ff1 ("bpf:
allow . char as part of the object name"). My best guess right now is potentially
not to confuse BPF's kallsyms handling with dashes etc.

Right, OK, fair enough I suppose. I was just wondering since this is
the second time I've run into hard-to-debug problems because of the
naming restrictions.

Really, it would be nice to have something like the netlink extack
mechanism so the kernel can return something more than just an error
code when a bpf() call fails. Is there any way to do something similar
for a syscall? Could we invent something?

-Toke

On 2/19/20 2:28 AM, Toke Høiland-Jørgensen wrote:

Daniel Borkmann [off-list ref] writes:

quoted

On 2/18/20 5:42 PM, Toke Høiland-Jørgensen wrote:

quoted

Yonghong Song [off-list ref] writes:

quoted

On 2/18/20 6:40 AM, Daniel Borkmann wrote:

quoted

On 2/17/20 6:17 PM, Toke Høiland-Jørgensen wrote:

quoted

The kernel only accepts map names with alphanumeric characters,
underscores
and periods in their name. However, the auto-generated internal map names
used by libbpf takes their prefix from the user-supplied BPF object name,
which has no such restriction. This can lead to "Invalid argument" errors
when trying to load a BPF program using global variables.

Fix this by sanitising the map names, replacing any non-allowed
characters
with underscores.

Fixes: d859900c4c56 ("bpf, libbpf: support global data/bss/rodata
sections")
Signed-off-by: Toke Høiland-Jørgensen <redacted>

Makes sense to me, applied, thanks! I presume you had something like '-'
in the
global var leading to rejection?

The C global variable cannot have '-'. I saw a complain in bcc mailing
list sometimes back like: if an object file is a-b.o, then we will
generate a map name like a-b.bss for the bss ELF section data. The
map name "a-b.bss" name will be rejected by the kernel. The workaround
is to change object file name. Not sure whether this is the only
issue which may introduce non [a-zA-Z0-9_] or not. But this patch indeed
should fix the issue I just described.

Yep, meant object file name, just realized too late after sending. :/

quoted

Yes, this was exactly my problem; my object file is called
'xdp-dispatcher.o'. Fun error to track down :P

Why doesn't the kernel allow dashes in the name anyway?

Commit cb4d2b3f03d8 ("bpf: Add name, load_time, uid and map_ids to bpf_prog_info")
doesn't state a specific reason, and we did later extend it via 3e0ddc4f3ff1 ("bpf:
allow . char as part of the object name"). My best guess right now is potentially
not to confuse BPF's kallsyms handling with dashes etc.

Right, OK, fair enough I suppose. I was just wondering since this is
the second time I've run into hard-to-debug problems because of the
naming restrictions.

Really, it would be nice to have something like the netlink extack
mechanism so the kernel can return something more than just an error
code when a bpf() call fails. Is there any way to do something similar
for a syscall? Could we invent something?

Currently, BPF_PROG_LOAD and BPF_BTF_LOAD has log_buf as part of syscall 
interface. Esp. for BPF_PROG_LOAD, maybe we could put some non-verifier 
logs here?

Maybe we could introduce log_buf to other syscall commands if there is
a great need in user space to get more details about the error code?

Yonghong Song [off-list ref] writes:

On 2/19/20 2:28 AM, Toke Høiland-Jørgensen wrote:

quoted

Daniel Borkmann [off-list ref] writes:

quoted

On 2/18/20 5:42 PM, Toke Høiland-Jørgensen wrote:

quoted

Yonghong Song [off-list ref] writes:

quoted

On 2/18/20 6:40 AM, Daniel Borkmann wrote:

quoted

On 2/17/20 6:17 PM, Toke Høiland-Jørgensen wrote:

quoted

The kernel only accepts map names with alphanumeric characters,
underscores
and periods in their name. However, the auto-generated internal map names
used by libbpf takes their prefix from the user-supplied BPF object name,
which has no such restriction. This can lead to "Invalid argument" errors
when trying to load a BPF program using global variables.

Fix this by sanitising the map names, replacing any non-allowed
characters
with underscores.

Fixes: d859900c4c56 ("bpf, libbpf: support global data/bss/rodata
sections")
Signed-off-by: Toke Høiland-Jørgensen <redacted>

Makes sense to me, applied, thanks! I presume you had something like '-'
in the
global var leading to rejection?

The C global variable cannot have '-'. I saw a complain in bcc mailing
list sometimes back like: if an object file is a-b.o, then we will
generate a map name like a-b.bss for the bss ELF section data. The
map name "a-b.bss" name will be rejected by the kernel. The workaround
is to change object file name. Not sure whether this is the only
issue which may introduce non [a-zA-Z0-9_] or not. But this patch indeed
should fix the issue I just described.

Yep, meant object file name, just realized too late after sending. :/

quoted

Yes, this was exactly my problem; my object file is called
'xdp-dispatcher.o'. Fun error to track down :P

Why doesn't the kernel allow dashes in the name anyway?

Commit cb4d2b3f03d8 ("bpf: Add name, load_time, uid and map_ids to bpf_prog_info")
doesn't state a specific reason, and we did later extend it via 3e0ddc4f3ff1 ("bpf:
allow . char as part of the object name"). My best guess right now is potentially
not to confuse BPF's kallsyms handling with dashes etc.

Right, OK, fair enough I suppose. I was just wondering since this is
the second time I've run into hard-to-debug problems because of the
naming restrictions.

Really, it would be nice to have something like the netlink extack
mechanism so the kernel can return something more than just an error
code when a bpf() call fails. Is there any way to do something similar
for a syscall? Could we invent something?

Currently, BPF_PROG_LOAD and BPF_BTF_LOAD has log_buf as part of syscall 
interface. Esp. for BPF_PROG_LOAD, maybe we could put some non-verifier 
logs here?

Maybe we could introduce log_buf to other syscall commands if there is
a great need in user space to get more details about the error code?

Hmm, that's not a bad idea, actually. I guess I'll take a stab at that
the next time I get really annoyed at having to track down an -EINVAL ;)

Unless someone else beats me to it, of course, which would be great!

-Toke