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;
[...]
