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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Yonghong Song <yhs@fb.com> writes:

> On 2/19/20 2:28 AM, Toke Høiland-Jørgensen wrote:
>> Daniel Borkmann <daniel@iogearbox.net> writes:
>> 
>>> On 2/18/20 5:42 PM, Toke Høiland-Jørgensen wrote:
>>>> Yonghong Song <yhs@fb.com> writes:
>>>>> On 2/18/20 6:40 AM, Daniel Borkmann wrote:
>>>>>> 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 <toke@redhat.com>
>>>>>>
>>>>>> 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.
>> 
>> 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