On Mon, Jan 8, 2024 at 4:40 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> 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);
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?)
> > - } 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;
>
> [...]
