On Fri, Feb 9, 2024 at 9:20 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Thu, 2024-02-08 at 20:05 -0800, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@xxxxxxxxxx>
> >
> > Add support for [LDX | STX | ST], PROBE_MEM32, [B | H | W | DW] instructions.
> > They are similar to PROBE_MEM instructions with the following differences:
> > - PROBE_MEM has to check that the address is in the kernel range with
> > src_reg + insn->off >= TASK_SIZE_MAX + PAGE_SIZE check
> > - PROBE_MEM doesn't support store
> > - PROBE_MEM32 relies on the verifier to clear upper 32-bit in the register
> > - PROBE_MEM32 adds 64-bit kern_vm_start address (which is stored in %r12 in the prologue)
> > Due to bpf_arena constructions such %r12 + %reg + off16 access is guaranteed
> > to be within arena virtual range, so no address check at run-time.
> > - PROBE_MEM32 allows STX and ST. If they fault the store is a nop.
> > When LDX faults the destination register is zeroed.
> >
> > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> > ---
>
> It would be great to add support for these new probe instructions in disasm,
> otherwise commands like "bpftool prog dump xlated" can't print them.
>
> I sort-of brute-force verified jit code generated for new instructions
> and disassembly seem to be as expected.
yeah. added a fix to the verifier patch.
> [...]
>
> > @@ -1564,6 +1697,52 @@ st: if (is_imm8(insn->off))
> > emit_stx(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
> > break;
> >
> > + case BPF_ST | BPF_PROBE_MEM32 | BPF_B:
> > + case BPF_ST | BPF_PROBE_MEM32 | BPF_H:
> > + case BPF_ST | BPF_PROBE_MEM32 | BPF_W:
> > + case BPF_ST | BPF_PROBE_MEM32 | BPF_DW:
> > + start_of_ldx = prog;
> > + emit_st_r12(&prog, BPF_SIZE(insn->code), dst_reg, insn->off, insn->imm);
> > + goto populate_extable;
> > +
> > + /* LDX: dst_reg = *(u8*)(src_reg + r12 + off) */
> > + case BPF_LDX | BPF_PROBE_MEM32 | BPF_B:
> > + case BPF_LDX | BPF_PROBE_MEM32 | BPF_H:
> > + case BPF_LDX | BPF_PROBE_MEM32 | BPF_W:
> > + case BPF_LDX | BPF_PROBE_MEM32 | BPF_DW:
> > + case BPF_STX | BPF_PROBE_MEM32 | BPF_B:
> > + case BPF_STX | BPF_PROBE_MEM32 | BPF_H:
> > + case BPF_STX | BPF_PROBE_MEM32 | BPF_W:
> > + case BPF_STX | BPF_PROBE_MEM32 | BPF_DW:
> > + start_of_ldx = prog;
> > + if (BPF_CLASS(insn->code) == BPF_LDX)
> > + emit_ldx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
> > + else
> > + emit_stx_r12(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
> > +populate_extable:
> > + {
> > + struct exception_table_entry *ex;
> > + u8 *_insn = image + proglen + (start_of_ldx - temp);
> > + s64 delta;
> > +
> > + if (!bpf_prog->aux->extable)
> > + break;
> > +
> > + ex = &bpf_prog->aux->extable[excnt++];
>
> Nit: this seem to mostly repeat exception logic for
> "BPF_LDX | BPF_MEM | BPF_B" & co,
> is there a way to abstract it a bit?
I don't see a good way. A macro is meh.
A helper with 5+ args is also meh.
> Also note that there excnt is checked for overflow.
indeed. added overflow check.
