Re: [bpf-next v2] bpf: drop deprecated bpf_jit_enable == 2

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 18/01/2023 à 18:42, Alexei Starovoitov a écrit :
> On Tue, Jan 17, 2023 at 11:36 PM Christophe Leroy
> <christophe.leroy@csgroup.eu> wrote:
>>
>>
>>
>> Le 18/01/2023 à 03:21, Alexei Starovoitov a écrit :
>>> On Tue, Jan 17, 2023 at 6:13 PM Tonghao Zhang <tong@infragraf.org> wrote:
>>>>
>>>>
>>>>
>>>>> On Jan 17, 2023, at 11:59 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>>>
>>>>> On 1/17/23 3:22 PM, Tonghao Zhang wrote:
>>>>>>> On Jan 17, 2023, at 3:30 PM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Le 17/01/2023 à 06:30, Tonghao Zhang a écrit :
>>>>>>>>
>>>>>>>>
>>>>>>>>> On Jan 9, 2023, at 4:15 PM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Le 06/01/2023 à 16:37, Daniel Borkmann a écrit :
>>>>>>>>>> On 1/5/23 6:53 PM, Christophe Leroy wrote:
>>>>>>>>>>> Le 05/01/2023 à 04:06, tong@infragraf.org a écrit :
>>>>>>>>>>>> From: Tonghao Zhang <tong@infragraf.org>
>>>>>>>>>>>>
>>>>>>>>>>>> The x86_64 can't dump the valid insn in this way. A test BPF prog
>>>>>>>>>>>> which include subprog:
>>>>>>>>>>>>
>>>>>>>>>>>> $ llvm-objdump -d subprog.o
>>>>>>>>>>>> Disassembly of section .text:
>>>>>>>>>>>> 0000000000000000 <subprog>:
>>>>>>>>>>>>            0:       18 01 00 00 73 75 62 70 00 00 00 00 72 6f 67 00 r1
>>>>>>>>>>>> = 29114459903653235 ll
>>>>>>>>>>>>            2:       7b 1a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r1
>>>>>>>>>>>>            3:       bf a1 00 00 00 00 00 00 r1 = r10
>>>>>>>>>>>>            4:       07 01 00 00 f8 ff ff ff r1 += -8
>>>>>>>>>>>>            5:       b7 02 00 00 08 00 00 00 r2 = 8
>>>>>>>>>>>>            6:       85 00 00 00 06 00 00 00 call 6
>>>>>>>>>>>>            7:       95 00 00 00 00 00 00 00 exit
>>>>>>>>>>>> Disassembly of section raw_tp/sys_enter:
>>>>>>>>>>>> 0000000000000000 <entry>:
>>>>>>>>>>>>            0:       85 10 00 00 ff ff ff ff call -1
>>>>>>>>>>>>            1:       b7 00 00 00 00 00 00 00 r0 = 0
>>>>>>>>>>>>            2:       95 00 00 00 00 00 00 00 exit
>>>>>>>>>>>>
>>>>>>>>>>>> kernel print message:
>>>>>>>>>>>> [  580.775387] flen=8 proglen=51 pass=3 image=ffffffffa000c20c
>>>>>>>>>>>> from=kprobe-load pid=1643
>>>>>>>>>>>> [  580.777236] JIT code: 00000000: cc cc cc cc cc cc cc cc cc cc cc
>>>>>>>>>>>> cc cc cc cc cc
>>>>>>>>>>>> [  580.779037] JIT code: 00000010: cc cc cc cc cc cc cc cc cc cc cc
>>>>>>>>>>>> cc cc cc cc cc
>>>>>>>>>>>> [  580.780767] JIT code: 00000020: cc cc cc cc cc cc cc cc cc cc cc
>>>>>>>>>>>> cc cc cc cc cc
>>>>>>>>>>>> [  580.782568] JIT code: 00000030: cc cc cc
>>>>>>>>>>>>
>>>>>>>>>>>> $ bpf_jit_disasm
>>>>>>>>>>>> 51 bytes emitted from JIT compiler (pass:3, flen:8)
>>>>>>>>>>>> ffffffffa000c20c + <x>:
>>>>>>>>>>>>        0:   int3
>>>>>>>>>>>>        1:   int3
>>>>>>>>>>>>        2:   int3
>>>>>>>>>>>>        3:   int3
>>>>>>>>>>>>        4:   int3
>>>>>>>>>>>>        5:   int3
>>>>>>>>>>>>        ...
>>>>>>>>>>>>
>>>>>>>>>>>> Until bpf_jit_binary_pack_finalize is invoked, we copy rw_header to
>>>>>>>>>>>> header
>>>>>>>>>>>> and then image/insn is valid. BTW, we can use the "bpftool prog dump"
>>>>>>>>>>>> JITed instructions.
>>>>>>>>>>>
>>>>>>>>>>> NACK.
>>>>>>>>>>>
>>>>>>>>>>> Because the feature is buggy on x86_64, you remove it for all
>>>>>>>>>>> architectures ?
>>>>>>>>>>>
>>>>>>>>>>> On powerpc bpf_jit_enable == 2 works and is very usefull.
>>>>>>>>>>>
>>>>>>>>>>> Last time I tried to use bpftool on powerpc/32 it didn't work. I don't
>>>>>>>>>>> remember the details, I think it was an issue with endianess. Maybe it
>>>>>>>>>>> is fixed now, but it needs to be verified.
>>>>>>>>>>>
>>>>>>>>>>> So please, before removing a working and usefull feature, make sure
>>>>>>>>>>> there is an alternative available to it for all architectures in all
>>>>>>>>>>> configurations.
>>>>>>>>>>>
>>>>>>>>>>> Also, I don't think bpftool is usable to dump kernel BPF selftests.
>>>>>>>>>>> That's vital when a selftest fails if you want to have a chance to
>>>>>>>>>>> understand why it fails.
>>>>>>>>>>
>>>>>>>>>> If this is actively used by JIT developers and considered useful, I'd be
>>>>>>>>>> ok to leave it for the time being. Overall goal is to reach feature parity
>>>>>>>>>> among (at least major arch) JITs and not just have most functionality only
>>>>>>>>>> available on x86-64 JIT. Could you however check what is not working with
>>>>>>>>>> bpftool on powerpc/32? Perhaps it's not too much effort to just fix it,
>>>>>>>>>> but details would be useful otherwise 'it didn't work' is too fuzzy.
>>>>>>>>>
>>>>>>>>> Sure I will try to test bpftool again in the coming days.
>>>>>>>>>
>>>>>>>>> Previous discussion about that subject is here:
>>>>>>>>> https://patchwork.kernel.org/project/linux-riscv/patch/20210415093250.3391257-1-Jianlin.Lv@arm.com/#24176847=
>>>>>>>> Hi Christophe
>>>>>>>> Any progress? We discuss to deprecate the bpf_jit_enable == 2 in 2021, but bpftool can not run on powerpc.
>>>>>>>> Now can we fix this issue?
>>>>>>>
>>>>>>> Hi Tong,
>>>>>>>
>>>>>>> I have started to look at it but I don't have any fruitfull feedback yet.
>>>>>>>
>>>>>>> In the meantime, were you able to confirm that bpftool can also be used
>>>>>>> to dump jitted tests from test_bpf.ko module on x86_64 ? In that can you
>>>>>>> tell me how to proceed ?
>>>>>> Now I do not test, but we can dump the insn after bpf_prog_select_runtime in test_bpf.ko. bpf_map_get_info_by_fd can copy the insn to userspace, but we can
>>>>>> dump them in test_bpf.ko in the same way.
>>>>>
>>>>> Issue is that these progs are not consumable from userspace (and therefore not bpftool).
>>>>> it's just simple bpf_prog_alloc + copy of test insns + bpf_prog_select_runtime() to test
>>>>> JITs (see generate_filter()). Some of them could be converted over to test_verifier, but
>>>>> not all might actually pass verifier, iirc. Don't think it's a good idea to allow exposing
>>>>> them via fd tbh.
>>>> Hi
>>>> I mean that, can we invoke the bpf_jit_dump in test_bpf.ko directly ?. bpf_prog_get_info_by_fd copy the insn to userspace, but we only dump insn in test_bpf.ko
>>>>
>>>>                   if (bpf_dump_raw_ok(file->f_cred)) {// code copied from bpf_prog_get_info_by_fd, not tested
>>>>
>>>>                           /* for multi-function programs, copy the JITed
>>>>                            * instructions for all the functions
>>>>                            */
>>>>                           if (prog->aux->func_cnt) {
>>>>                                   for (i = 0; i < prog->aux->func_cnt; i++) {
>>>>                                           len = prog->aux->func[i]->jited_len;
>>>>                                           img = (u8 *) prog->aux->func[i]->bpf_func;
>>>>                                           bpf_jit_dump(1, len, 1, img);
>>>>                                   }
>>>>                           } else {
>>>>                                   bpf_jit_dump(1, ulen, 1, prog->bpf_func);
>>>>                           }
>>>>                   }
>>>
>>> Let's not reinvent the wheel.
>>> bpftool prog dump jited
>>> is our supported command.
>>> ppc issue with bpftool is related to endianness of embedded skeleton.
>>> which means that none of the bpftool prog commands work on ppc.
>>> It's a bigger issue to address with cross compilation of bpftool.
>>>
>>> bpftool supports gnu and llvm disassembler. It retrieves and
>>> prints BTF, line info and source code along with asm.
>>> The user experience is at different level comparing to bpf_jit_dump.
>>
>> Hi Alexei,
>>
>> Fair enough, we are going to try and fix bpftool.
>>
>> But for test_bpf.ko module, how do you use bpftool to dump the BPF tests
>> ? Even on x86 I have not been able to use bpftool for that until now.
>> Can you tell me how you do ?
> 
> test_bpf.ko is useful to JIT developers when they're starting
> to work on it, but its test coverage is inadequate for real
> world bpf usage comparing to selftests/bpf.
> Johan Almbladh did some great additions to test_bpf.ko back in 2021.
> Since then there wasn't much.
> 
> Here it's important to distinguish the target user.
> Is it a kernel JIT developer or user space bpf prog developer?
> When it's a kernel developer they can easily
> add print_hex_dump() in the right places.
> That's what I did when I was developing bpf trampoline.
> bpf is more than just JIT. There are trampoline, kfuncs, dispatch.
> The kernel devs should not add a debug code.
> Long ago bpf_jit_enable=2 was useful to user space bpf developers.
> They wanted to see how JITed code look like to optimize it and what not.
> Now 'perf record' captures bpf asm and annotates it in 'perf report',
> so performance analysis problem is solved that way.
> bpftool prog dump jit addressed the needs of users and admins who
> want to understand what bpf progs are loaded and what are they doing.
> Both 'dump jited' and 'dump xlated' are useful for this case.
> So bpf_jit_enable=2 remained useful to kernel developers only and
> in that sense it become a kernel debug feature for a narrow set of
> JIT developers. On x86 bpf_jit_dump() was neglected and broken.
> I suspect the other archs will follow the same fate. If not already.
> Having a sysctl for kernel developers is not something the kernel
> developers should have around. Hence the cleanup of this patch.


Ok, I understand. Then it means that ' bpf_jit_enable == 2' can be 
removed once 'bpftool' properly works for dumping userspace BPF progs.

But for the time being it doesn't seem to work, see my other answer.

Christophe