Re: [PATCH bpf-next 1/3] bpf: simplify try_match_pkt_pointers()

[Eduard Zingerman <eddyz87@xxxxxxxxx>]

On Mon, 2024-01-08 at 16:40 -0800, Andrii Nakryiko wrote:
[...]
> > @@ -14684,90 +14687,31 @@ static bool try_match_pkt_pointers(const struct bpf_insn *insn,
> >         if (BPF_CLASS(insn->code) == BPF_JMP32)
> >                 return false;
> > 
> > -       switch (BPF_OP(insn->code)) {
> > +       if (dst_reg->type == PTR_TO_PACKET_END ||
> > +           src_reg->type == PTR_TO_PACKET_META) {
> > +               swap(src_reg, dst_reg);
> > +               dst_regno = insn->src_reg;
> > +               opcode = flip_opcode(opcode);
> > +       }
> > +
> > +       if ((dst_reg->type != PTR_TO_PACKET ||
> > +            src_reg->type != PTR_TO_PACKET_END) &&
> > +           (dst_reg->type != PTR_TO_PACKET_META ||
> > +            !reg_is_init_pkt_pointer(src_reg, PTR_TO_PACKET)))
> > +               return false;
> 
> this inverted original condition just breaks my brain, I can't wrap my
> head around it :) I think the original is easier to reason about
> because it's two clear allowable patterns for which we do something. I
> understand that this early exit reduces nestedness, but at least for
> me it would be simpler to have the original non-inverted condition
> with a nested switch.

I'm not sure I understand what you mean by nested switch.
If I write it down like below, would that be more clear?

	bool pkt_data_vs_pkt_end;
    bool pkt_meta_vs_pkt_data;
    ...
    pkt_data_vs_pkt_end =
      dst_reg->type == PTR_TO_PACKET && src_reg->type == PTR_TO_PACKET_END;
    pkt_meta_vs_pkt_data =
      dst_reg->type == PTR_TO_PACKET_META && reg_is_init_pkt_pointer(src_reg, PTR_TO_PACKET);

    if (!pkt_data_vs_pkt_end && !pkt_meta_vs_pkt_data)
        return false;

> > +
> > +       switch (opcode) {
> >         case BPF_JGT:
> > -               if ((dst_reg->type == PTR_TO_PACKET &&
> > -                    src_reg->type == PTR_TO_PACKET_END) ||
> > -                   (dst_reg->type == PTR_TO_PACKET_META &&
> > -                    reg_is_init_pkt_pointer(src_reg, PTR_TO_PACKET))) {
> > -                       /* pkt_data' > pkt_end, pkt_meta' > pkt_data */
> > -                       find_good_pkt_pointers(this_branch, dst_reg,
> > -                                              dst_reg->type, false);
> > -                       mark_pkt_end(other_branch, insn->dst_reg, true);

> it seems like you can make a bit of simplification if mark_pkt_end
> would just accept struct bpf_reg_state * instead of int regn (you
> won't need to keep track of dst_regno at all, right?)

mark_pkt_end() changes the register from either this_branch or other_branch.
I can introduce local pointers dst_this/dst_other and swap those,
but I'm not sure it's worth it.

[...]