Re: [PATCH 6.1 00/86] 6.1.104-rc2 review

[Quentin Monnet <qmo@kernel.org>]

2024-08-10 07:02 UTC+0200 ~ Salvatore Bonaccorso <carnil@debian.org>
> Hi Greg,
> 
> [adding as well people involved in the original commit and the
> backporting for 6.1.y branch]
> 
> On Thu, Aug 08, 2024 at 12:33:22PM +0200, Salvatore Bonaccorso wrote:
>> Hi Greg,
>>
>> On Thu, Aug 08, 2024 at 11:11:49AM +0200, Greg Kroah-Hartman wrote:
>>> This is the start of the stable review cycle for the 6.1.104 release.
>>> There are 86 patches in this series, all will be posted as a response
>>> to this one.  If anyone has any issues with these being applied, please
>>> let me know.
>>>
>>> Responses should be made by Sat, 10 Aug 2024 09:11:02 +0000.
>>> Anything received after that time might be too late.
>>
>> Sorry for bothering you again with it (see previous comment on
>> 6.1.103, respectively 6.1.104-rc1): bpftool still would fail to
>> compile:
>>
>> gcc -O2 -W -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls -Wstrict-prototypes -Wswitch-default -Wundef -Wwrite-strings -Wformat -Wno-type-limits -Wstrict-aliasing=3 -Wshadow -DPACKAGE='"bpftool"' -D__EXPORTED_HEADERS__ -I. -I/home/build/linux-stable-rc/tools/bpf/bpftool/libbpf/include -I/home/build/linux-stable-rc/kernel/bpf/ -I/home/build/linux-stable-rc/tools/include -I/home/build/linux-stable-rc/tools/include/uapi -DUSE_LIBCAP -DBPFTOOL_WITHOUT_SKELETONS -c -MMD prog.c -o prog.o
>> prog.c: In function ‘load_with_options’:
>> prog.c:1710:23: warning: implicit declaration of function ‘create_and_mount_bpffs_dir’ [-Wimplicit-function-declaration]
>>  1710 |                 err = create_and_mount_bpffs_dir(pinmaps);
>>       |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> gcc -O2 -W -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wbad-function-cast -Wdeclaration-after-statement -Wformat-security -Wformat-y2k -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wno-system-headers -Wold-style-definition -Wpacked -Wredundant-decls -Wstrict-prototypes -Wswitch-default -Wundef -Wwrite-strings -Wformat -Wno-type-limits -Wstrict-aliasing=3 -Wshadow -DPACKAGE='"bpftool"' -D__EXPORTED_HEADERS__ -I. -I/home/build/linux-stable-rc/tools/bpf/bpftool/libbpf/include -I/home/build/linux-stable-rc/kernel/bpf/ -I/home/build/linux-stable-rc/tools/include -I/home/build/linux-stable-rc/tools/include/uapi -DUSE_LIBCAP -DBPFTOOL_WITHOUT_SKELETONS  btf.o btf_dumper.o cfg.o cgroup.o common.o feature.o gen.o iter.o json_writer.o link.o main.o map.o map_perf_ring.o net.o netlink_dumper.o perf.o pids.o prog.o struct_ops.o tracelog.o xlated_dumper.o disasm.o /home/build/linux-stable-rc/tools/bpf/bpftool/libbpf/libbpf.a -lelf -lz -lcap -o bpftool
>> /bin/ld: prog.o: in function `load_with_options':
>> prog.c:(.text+0x2f98): undefined reference to `create_and_mount_bpffs_dir'
>> /bin/ld: prog.c:(.text+0x2ff2): undefined reference to `create_and_mount_bpffs_dir'
>> collect2: error: ld returned 1 exit status
>> make[1]: *** [Makefile:216: bpftool] Error 1
>> make: *** [Makefile:113: bpftool] Error 2
>>
>> Reverting 65dd9cbafec2f6f7908cebcab0386f750fc352af fixes the issue. In
>> fact 65dd9cbafec2f6f7908cebcab0386f750fc352af is the only commit
>> adding call to create_and_mount_bpffs_dir:
>>
>> $ git grep create_and_mount_bpffs_dir
>> tools/bpf/bpftool/prog.c:               err = create_and_mount_bpffs_dir(pinmaps);
> 
> Just one additional note, at least 478a535ae54a ("bpftool: Mount bpffs
> on provided dir instead of parent dir") would be a reqisite where the
> code was refactored introducing create_and_mount_bpffs_dir() (but
> won't apply cleanly to 6.1.y). But are more requisites needed?
> 
> Should it be safest to just revert the breaking commit for the bpftool
> build?
> 
> Regards,
> Salvatore
> 

Hi,

You should be able to fix the build by first cherry-picking commit
2a36c26fe3b8 ("bpftool: Support bpffs mountpoint as pin path for prog
loadall"), and then commit 478a535ae54a ("bpftool: Mount bpffs on
provided dir instead of parent dir") as you figured. Both commits have a
minor conflict on tools/bpf/bpftool/struct_ops.c, which should be
addressed by discarding the relevant hunk (for both commit).

Alternatively, it's also fine to revert the breaking commit. It's a
quality of life improvement without which users may have to manually
mount the bpffs at the location they want to pin their maps when loading
multiple BPF programs with "bpftool prog loadall", in the unlikely event
they're not using /sys/kernel/bpf, prior to running the bpftool command.
It's not in use during the kernel build process or for the BPF
selftests, so not necessary on stable branches.

I hope this helps,
Quentin