On Wed, Jul 12, 2023 at 11:07:24PM -0700, Yonghong Song wrote:
>
> @@ -1942,6 +1945,16 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn)
> LDST(DW, u64)
> #undef LDST
>
> +#define LDS(SIZEOP, SIZE) \
LDSX ?
> + LDX_MEMSX_##SIZEOP: \
> + DST = *(SIZE *)(unsigned long) (SRC + insn->off); \
> + CONT;
> +
> + LDS(B, s8)
> + LDS(H, s16)
> + LDS(W, s32)
> +#undef LDS
...
> @@ -17503,7 +17580,10 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> if (insn->code == (BPF_LDX | BPF_MEM | BPF_B) ||
> insn->code == (BPF_LDX | BPF_MEM | BPF_H) ||
> insn->code == (BPF_LDX | BPF_MEM | BPF_W) ||
> - insn->code == (BPF_LDX | BPF_MEM | BPF_DW)) {
> + insn->code == (BPF_LDX | BPF_MEM | BPF_DW) ||
> + insn->code == (BPF_LDX | BPF_MEMSX | BPF_B) ||
> + insn->code == (BPF_LDX | BPF_MEMSX | BPF_H) ||
> + insn->code == (BPF_LDX | BPF_MEMSX | BPF_W)) {
> type = BPF_READ;
> } else if (insn->code == (BPF_STX | BPF_MEM | BPF_B) ||
> insn->code == (BPF_STX | BPF_MEM | BPF_H) ||
> @@ -17562,6 +17642,11 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> */
> case PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED:
> if (type == BPF_READ) {
> + /* it is hard to differentiate that the
> + * BPF_PROBE_MEM is for BPF_MEM or BPF_MEMSX,
> + * let us use insn->imm to remember it.
> + */
> + insn->imm = BPF_MODE(insn->code);
That's a fragile approach.
And the evidence is in this patch.
This part of interpreter:
LDX_PROBE_MEM_##SIZEOP: \
bpf_probe_read_kernel(&DST, sizeof(SIZE), \
(const void *)(long) (SRC + insn->off)); \
DST = *((SIZE *)&DST); \
wasn't updated to handle sign extension.
How about
#define BPF_PROBE_MEMSX 0x40 /* same as BPF_IND */
and handle it in JITs and interpreter.
We need a selftest for BTF style access to signed fields to make sure both
interpreter and JIT handling of BPF_PROBE_MEMSX is tested.
