Re: [PATCH bpf-next v3 1/5] selftests/bpf: specify expected instructions in test_verifier tests

[Eduard Zingerman <eddyz87@gmail.com>]

> On Fri, 2022-06-03 at 14:31 -0700, Song Liu wrote:
> > On Fri, Jun 3, 2022 at 7:11 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> 
> > +#define INSN_OFF_MASK  ((s16)0xFFFF)
> > +#define INSN_IMM_MASK  ((s32)0xFFFFFFFF)
> 
> Shall we use __s16 and __s32 to match struct bpf_insn exactly.

Will do.

[...]

> > +       __u32 buf_elt_size = sizeof(**buf);
> 
> I guess elt means "element"? I would recommend use sizeof(struct bpf_insn)
> directly.

Will do.

[...]

> > +static int null_terminated_insn_len(struct bpf_insn *seq, int max_len)
> > +{
> > +       for (int i = 0; i < max_len; ++i) {
> 
> Sorry for missing this in v1. We should really pull variable
> declaration out, like
> 
> int i;
> 
> for (int i = 0; ...)

Sorry, just to clarify, you want me to pull all loop counter
declarations to the top of the relevant functions, right? (Affecting 4
functions in this patch).

[...]

> > +static int find_insn_subseq(struct bpf_insn *seq, struct bpf_insn *subseq,
> > +       if (check_unexpected &&
> > +           find_all_insn_subseqs(buf, test->unexpected_insns,
> > +                                 cnt, MAX_UNEXPECTED_INSNS)) {
> 
> I wonder whether we want different logic for unexpected_insns. With multiple
> sub sequences, say seq-A and seq-B, it is more natural to reject any results
> with either seq-A or seq-B. However, current logic will reject seq-A => seq-B,
> but will accept seq-B => seq-A. Does this make sense?

Have no strong opinion on this topic. In theory different variants
might be useful in different cases.

In the test cases for bpf_loop inlining I only had to match a single
unexpected instruction, so I opted to use same match function in both
expected and unexpected cases to keep things simple.

One thought that I had was that struct bpf_test might be extended in
the future as follows:

#define MAX_PATTERNS 4
...
struct bpf_test {
	...
	struct bpf_insn unexpected_insns[MAX_UNEXPECTED_INSNS][MAX_PATTERNS];
	...
}

Where each pattern follows logic "seq-A => seq-B", but patterns are
matched independently. So if the goal is to match either "seq-A" or
"seq-B" these should be specified as separate patterns. However, this
seems to be an overkill for the problem at hand.