Re: [PATCH v4 bpf-next] Small BPF verifier log improvements

[Daniel Borkmann <daniel@xxxxxxxxxxxxx>]

On 3/1/22 7:56 PM, Mykola Lysenko wrote:
> > On Mar 1, 2022, at 5:33 AM, Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> > On 2/24/22 1:37 AM, Mykola Lysenko wrote:
> > > In particular:
> > > 1) remove output of inv for scalars in print_verifier_state
> > > 2) replace inv with scalar in verifier error messages
> > > 3) remove _value suffixes for umin/umax/s32_min/etc (except map_value)
> > > 4) remove output of id=0
> > > 5) remove output of ref_obj_id=0
> > > Signed-off-by: Mykola Lysenko <mykolal@xxxxxx>
> >
> > Thanks for helping to improve the verifier output. Small comment below:
>
> Thanks for the review!
>
> > [...]
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/align.c b/tools/testing/selftests/bpf/prog_tests/align.c
> > > index 0ee29e11eaee..210dc6b4a169 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/align.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/align.c
> > > @@ -39,13 +39,13 @@ static struct bpf_align_test tests[] = {
> > >   		},
> > >   		.prog_type = BPF_PROG_TYPE_SCHED_CLS,
> > >   		.matches = {
> > > -			{0, "R1=ctx(id=0,off=0,imm=0)"},
> > > +			{0, "R1=ctx(off=0,imm=0)"},
> > >   			{0, "R10=fp0"},
> > > -			{0, "R3_w=inv2"},
> > > -			{1, "R3_w=inv4"},
> > > -			{2, "R3_w=inv8"},
> > > -			{3, "R3_w=inv16"},
> > > -			{4, "R3_w=inv32"},
> > > +			{0, "R3_w=2"},
> > > +			{1, "R3_w=4"},
> > > +			{2, "R3_w=8"},
> > > +			{3, "R3_w=16"},
> > > +			{4, "R3_w=32"},
> >
> > Ack, definitely better compared to the state today. :)
> >
> > [...]
> > > @@ -161,19 +161,19 @@ static struct bpf_align_test tests[] = {
> > >   		},
> > >   		.prog_type = BPF_PROG_TYPE_SCHED_CLS,
> > >   		.matches = {
> > > -			{6, "R0_w=pkt(id=0,off=8,r=8,imm=0)"},
> > > -			{6, "R3_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
> > > -			{7, "R3_w=inv(id=0,umax_value=510,var_off=(0x0; 0x1fe))"},
> > > -			{8, "R3_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
> > > -			{9, "R3_w=inv(id=0,umax_value=2040,var_off=(0x0; 0x7f8))"},
> > > -			{10, "R3_w=inv(id=0,umax_value=4080,var_off=(0x0; 0xff0))"},
> > > -			{12, "R3_w=pkt_end(id=0,off=0,imm=0)"},
> > > -			{17, "R4_w=inv(id=0,umax_value=255,var_off=(0x0; 0xff))"},
> > > -			{18, "R4_w=inv(id=0,umax_value=8160,var_off=(0x0; 0x1fe0))"},
> > > -			{19, "R4_w=inv(id=0,umax_value=4080,var_off=(0x0; 0xff0))"},
> > > -			{20, "R4_w=inv(id=0,umax_value=2040,var_off=(0x0; 0x7f8))"},
> > > -			{21, "R4_w=inv(id=0,umax_value=1020,var_off=(0x0; 0x3fc))"},
> > > -			{22, "R4_w=inv(id=0,umax_value=510,var_off=(0x0; 0x1fe))"},
> > > +			{6, "R0_w=pkt(off=8,r=8,imm=0)"},
> > > +			{6, "R3_w=(umax=255,var_off=(0x0; 0xff))"},
> > > +			{7, "R3_w=(umax=510,var_off=(0x0; 0x1fe))"},
> > > +			{8, "R3_w=(umax=1020,var_off=(0x0; 0x3fc))"},
> > > +			{9, "R3_w=(umax=2040,var_off=(0x0; 0x7f8))"},
> > > +			{10, "R3_w=(umax=4080,var_off=(0x0; 0xff0))"},
> > > +			{12, "R3_w=pkt_end(off=0,imm=0)"},
> > > +			{17, "R4_w=(umax=255,var_off=(0x0; 0xff))"},
> > > +			{18, "R4_w=(umax=8160,var_off=(0x0; 0x1fe0))"},
> > > +			{19, "R4_w=(umax=4080,var_off=(0x0; 0xff0))"},
> > > +			{20, "R4_w=(umax=2040,var_off=(0x0; 0x7f8))"},
> > > +			{21, "R4_w=(umax=1020,var_off=(0x0; 0x3fc))"},
> > > +			{22, "R4_w=(umax=510,var_off=(0x0; 0x1fe))"},
> > >   		},
> > >   	},
> > >   	{
> >
> > However, not printing any type info here is imho more confusing. For debugging /
> > troubleshooting knowing that the register type is inv or scalar would be helpful.
> > Fwiw, scalar is probably a better fit, although longer..
>
> So, just to confirm. You are proposing to leave cases like "R3_w=8” as is, but change cases like "R3_w=(umax=255,var_off=(0x0; 0xff))” to “R3_w=scalar(umax=255,var_off=(0x0; 0xff))”, correct?

Yes, that would look reasonable and pretty self-descriptive to me.

Thanks,
Daniel