On 7/14/23 11:13 AM, Alexei Starovoitov wrote:
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 ?
Ack.
+ 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.
Thanks for catching this!
How about
#define BPF_PROBE_MEMSX 0x40 /* same as BPF_IND */
and handle it in JITs and interpreter.
Good idea. Will do.
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.
Will do.
