On Mon, Jan 8, 2024 at 5:28 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > Reduce number of cases handled in try_match_pkt_pointers() > to <pkt_data> <op> <pkt_end> or <pkt_meta> <op> <pkt_data> > by flipping opcode. > > Suggested-by: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > --- > kernel/bpf/verifier.c | 104 ++++++++++-------------------------------- > 1 file changed, 24 insertions(+), 80 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index adbf330d364b..918e6a7912e2 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -14677,6 +14677,9 @@ static bool try_match_pkt_pointers(const struct bpf_insn *insn, > struct bpf_verifier_state *this_branch, > struct bpf_verifier_state *other_branch) > { > + int opcode = BPF_OP(insn->code); > + int dst_regno = insn->dst_reg; > + > if (BPF_SRC(insn->code) != BPF_X) > return false; > > @@ -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. > + > + 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); > - } else if ((dst_reg->type == PTR_TO_PACKET_END && > - src_reg->type == PTR_TO_PACKET) || > - (reg_is_init_pkt_pointer(dst_reg, PTR_TO_PACKET) && > - src_reg->type == PTR_TO_PACKET_META)) { > - /* pkt_end > pkt_data', pkt_data > pkt_meta' */ > - find_good_pkt_pointers(other_branch, src_reg, > - src_reg->type, true); > - mark_pkt_end(this_branch, insn->src_reg, false); > - } else { > - return false; > - } > - break; [...]