Re: [PATCH bpf-next 0/8] BPF verifier log improvements

[Stanislav Fomichev <sdf@google.com>]

On 11/10, Andrii Nakryiko wrote:
> This patch set moves a big chunk of verifier log related code from gigantic
> verifier.c file into more focused kernel/bpf/log.c. This is not essential to
> the rest of functionality in this patch set, so I can undo it, but it felt
> like it's good to start chipping away from 20K+ verifier.c whenever we can.
> 
> The main purpose of the patch set, though, is in improving verifier log
> further.
> 
> Patches #3-#4 start printing out register state even if that register is
> spilled into stack slot. Previously we'd get only spilled register type, but
> no additional information, like SCALAR_VALUE's ranges. Super limiting during
> debugging. For cases of register spills smaller than 8 bytes, we also print
> out STACK_MISC/STACK_ZERO/STACK_INVALID markers. This, among other things,
> will make it easier to write tests for these mixed spill/misc cases.
> 
> Patch #5 prints map name for PTR_TO_MAP_VALUE/PTR_TO_MAP_KEY/CONST_PTR_TO_MAP
> registers. In big production BPF programs, it's important to map assembly to
> actual map, and it's often non-trivial. Having map name helps.

[..]

> Patch #6 just removes visual noise in form of ubiquitous imm=0 and off=0. They
> are default values, omit them.

If you end up with another respin for some reason:
s/verifierl/verifier/
s/furthre/futher/

(in the commit description)

> Patch #7 is probably the most controversial, but it reworks how verifier log
> prints numbers. For small valued integers we use decimals, but for large ones
> we switch to hexadecimal. From personal experience this is a much more useful
> convention. We can tune what consitutes "small value", for now it's 16-bit
> range.

Not sure why not always print in hex, but no strong preference here.

> Patch #8 prints frame number for PTR_TO_CTX registers, if that frame is
> different from the "current" one. This removes ambiguity and confusion,
> especially in complicated cases with multiple subprogs passing around
> pointers.
> 
> Andrii Nakryiko (8):
>   bpf: move verbose_linfo() into kernel/bpf/log.c
>   bpf: move verifier state printing code to kernel/bpf/log.c
>   bpf: extract register state printing
>   bpf: print spilled register state in stack slot
>   bpf: emit map name in register state if applicable and available
>   bpf: omit default off=0 and imm=0 in register state log
>   bpf: smarter verifier log number printing logic
>   bpf: emit frameno for PTR_TO_STACK regs if it differs from current one

Acked-by: Stanislav Fomichev <sdf@google.com>