On Mon, Jan 8, 2024 at 4:52 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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?
>
Yes, much more clear. What I had in mind was something like:
if (pkt_data_vs_pkt_end || pkt_meta_vs_pkt_data) {
switch (opcode) {
...
}
}
But what you have below is easy to follow as well
> 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.
Ah, I missed that it's other_branch register. Never mind then, it's
fine and it was minor anyways.
>
> [...]
