On Fri, 2023-03-03 at 12:21 -0800, Alexei Starovoitov wrote:
> On Fri, Mar 03, 2023 at 12:55:05AM +0200, Eduard Zingerman wrote:
> > - prev_src_type = &env->insn_aux_data[env->insn_idx].ptr_type;
> > -
> > - if (*prev_src_type == NOT_INIT) {
> > - /* saw a valid insn
> > - * dst_reg = *(u32 *)(src_reg + off)
> > - * save type to validate intersecting paths
> > - */
> > - *prev_src_type = src_reg_type;
> > -
> > - } else if (reg_type_mismatch(src_reg_type, *prev_src_type)) {
> > - /* ABuser program is trying to use the same insn
> > - * dst_reg = *(u32*) (src_reg + off)
> > - * with different pointer types:
> > - * src_reg == ctx in one branch and
> > - * src_reg == stack|map in some other branch.
> > - * Reject it.
> > - */
> > - verbose(env, "same insn cannot be used with different pointers\n");
> > - return -EINVAL;
>
> There is a merge conflict with this part.
> LDX is now handled slightly differently comparing to STX.
Merge seems not complicated, will send v2 shortly.
>
> > - }
> > -
> > + err = save_aux_ptr_type(env, src_reg_type);
> > + if (err)
> > + return err;
> > } else if (class == BPF_STX) {
> > - enum bpf_reg_type *prev_dst_type, dst_reg_type;
> > + enum bpf_reg_type dst_reg_type;
> >
> > if (BPF_MODE(insn->code) == BPF_ATOMIC) {
> > err = check_atomic(env, env->insn_idx, insn);
> > @@ -14712,16 +14719,12 @@ static int do_check(struct bpf_verifier_env *env)
> > if (err)
> > return err;
> >
> > - prev_dst_type = &env->insn_aux_data[env->insn_idx].ptr_type;
> > -
> > - if (*prev_dst_type == NOT_INIT) {
> > - *prev_dst_type = dst_reg_type;
> > - } else if (reg_type_mismatch(dst_reg_type, *prev_dst_type)) {
> > - verbose(env, "same insn cannot be used with different pointers\n");
> > - return -EINVAL;
> > - }
> > -
> > + err = save_aux_ptr_type(env, dst_reg_type);
> > + if (err)
> > + return err;
> > } else if (class == BPF_ST) {
> > + enum bpf_reg_type dst_reg_type;
> > +
> > if (BPF_MODE(insn->code) != BPF_MEM ||
> > insn->src_reg != BPF_REG_0) {
> > verbose(env, "BPF_ST uses reserved fields\n");
> > @@ -14732,12 +14735,7 @@ static int do_check(struct bpf_verifier_env *env)
> > if (err)
> > return err;
> >
> > - if (is_ctx_reg(env, insn->dst_reg)) {
> > - verbose(env, "BPF_ST stores into R%d %s is not allowed\n",
> > - insn->dst_reg,
> > - reg_type_str(env, reg_state(env, insn->dst_reg)->type));
> > - return -EACCES;
> > - }
> > + dst_reg_type = regs[insn->dst_reg].type;
> >
> > /* check that memory (dst_reg + off) is writeable */
> > err = check_mem_access(env, env->insn_idx, insn->dst_reg,
> > @@ -14746,6 +14744,9 @@ static int do_check(struct bpf_verifier_env *env)
> > if (err)
> > return err;
> >
> > + err = save_aux_ptr_type(env, dst_reg_type);
> > + if (err)
> > + return err;
> > } else if (class == BPF_JMP || class == BPF_JMP32) {
> > u8 opcode = BPF_OP(insn->code);
> >
> > @@ -15871,7 +15872,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
> > insn->code == (BPF_ST | BPF_MEM | BPF_W) ||
> > insn->code == (BPF_ST | BPF_MEM | BPF_DW)) {
> > type = BPF_WRITE;
> > - ctx_access = BPF_CLASS(insn->code) == BPF_STX;
> > + ctx_access = true;
>
> I think 'ctx_access' variable can be removed, since it will be always true.
Sorry, missed this, will remove in v2.
>
> > } else {
> > continue;
> > }
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 1d6f165923bf..8e819b8464e8 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -9264,11 +9264,15 @@ static struct bpf_insn *bpf_convert_tstamp_write(const struct bpf_prog *prog,
> > #endif
> >
> > /* <store>: skb->tstamp = tstamp */
> > - *insn++ = BPF_STX_MEM(BPF_DW, skb_reg, value_reg,
> > - offsetof(struct sk_buff, tstamp));
> > + *insn++ = BPF_RAW_INSN(BPF_CLASS(si->code) | BPF_DW | BPF_MEM,
> > + skb_reg, value_reg, offsetof(struct sk_buff, tstamp), si->imm);
> > return insn;
> > }
> >
> > +#define BPF_COPY_STORE(size, si, off) \
> > + BPF_RAW_INSN((si)->code | (size) | BPF_MEM, \
> > + (si)->dst_reg, (si)->src_reg, (off), (si)->imm)
> > +
>
> Could you explain the "copy store" name?
I want to replicate registers, code and immediate operand from `si`,
hence the word "copy".
The more descriptive name might be `BPF_CLONE_STORE`.
> I don't understand what it means.
> It emits either STX or ST insn, right?
> Maybe BPF_EMIT_STORE ?
Can use `BPF_EMIT_STORE` one as well.
>
> > static u32 bpf_convert_ctx_access(enum bpf_access_type type,
> > const struct bpf_insn *si,
> > struct bpf_insn *insn_buf,
> > @@ -9298,9 +9302,9 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
> >
> > case offsetof(struct __sk_buff, priority):
> > if (type == BPF_WRITE)
> > - *insn++ = BPF_STX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > - bpf_target_off(struct sk_buff, priority, 4,
> > - target_size));
> > + *insn++ = BPF_COPY_STORE(BPF_W, si,
> > + bpf_target_off(struct sk_buff, priority, 4,
> > + target_size));
> > else
> > *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > bpf_target_off(struct sk_buff, priority, 4,
> > @@ -9331,9 +9335,9 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
> >
> > case offsetof(struct __sk_buff, mark):
> > if (type == BPF_WRITE)
> > - *insn++ = BPF_STX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > - bpf_target_off(struct sk_buff, mark, 4,
> > - target_size));
> > + *insn++ = BPF_COPY_STORE(BPF_W, si,
> > + bpf_target_off(struct sk_buff, mark, 4,
> > + target_size));
> > else
> > *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > bpf_target_off(struct sk_buff, mark, 4,
> > @@ -9352,11 +9356,16 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
> >
> > case offsetof(struct __sk_buff, queue_mapping):
> > if (type == BPF_WRITE) {
> > - *insn++ = BPF_JMP_IMM(BPF_JGE, si->src_reg, NO_QUEUE_MAPPING, 1);
> > - *insn++ = BPF_STX_MEM(BPF_H, si->dst_reg, si->src_reg,
> > - bpf_target_off(struct sk_buff,
> > - queue_mapping,
> > - 2, target_size));
> > + u32 off = bpf_target_off(struct sk_buff, queue_mapping, 2, target_size);
> > +
> > + if (BPF_CLASS(si->code) == BPF_ST && si->imm >= NO_QUEUE_MAPPING) {
> > + *insn++ = BPF_JMP_A(0); /* noop */
> > + break;
> > + }
> > +
> > + if (BPF_CLASS(si->code) == BPF_STX)
> > + *insn++ = BPF_JMP_IMM(BPF_JGE, si->src_reg, NO_QUEUE_MAPPING, 1);
> > + *insn++ = BPF_COPY_STORE(BPF_H, si, off);
> > } else {
> > *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
> > bpf_target_off(struct sk_buff,
> > @@ -9392,8 +9401,7 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
> > off += offsetof(struct sk_buff, cb);
> > off += offsetof(struct qdisc_skb_cb, data);
> > if (type == BPF_WRITE)
> > - *insn++ = BPF_STX_MEM(BPF_SIZE(si->code), si->dst_reg,
> > - si->src_reg, off);
> > + *insn++ = BPF_COPY_STORE(BPF_SIZE(si->code), si, off);
> > else
> > *insn++ = BPF_LDX_MEM(BPF_SIZE(si->code), si->dst_reg,
> > si->src_reg, off);
> > @@ -9408,8 +9416,7 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
> > off += offsetof(struct qdisc_skb_cb, tc_classid);
> > *target_size = 2;
> > if (type == BPF_WRITE)
> > - *insn++ = BPF_STX_MEM(BPF_H, si->dst_reg,
> > - si->src_reg, off);
> > + *insn++ = BPF_COPY_STORE(BPF_H, si, off);
> > else
> > *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg,
> > si->src_reg, off);
> > @@ -9442,9 +9449,9 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
> > case offsetof(struct __sk_buff, tc_index):
> > #ifdef CONFIG_NET_SCHED
> > if (type == BPF_WRITE)
> > - *insn++ = BPF_STX_MEM(BPF_H, si->dst_reg, si->src_reg,
> > - bpf_target_off(struct sk_buff, tc_index, 2,
> > - target_size));
> > + *insn++ = BPF_COPY_STORE(BPF_H, si,
> > + bpf_target_off(struct sk_buff, tc_index, 2,
> > + target_size));
> > else
> > *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
> > bpf_target_off(struct sk_buff, tc_index, 2,
> > @@ -9645,8 +9652,8 @@ u32 bpf_sock_convert_ctx_access(enum bpf_access_type type,
> > BUILD_BUG_ON(sizeof_field(struct sock, sk_bound_dev_if) != 4);
> >
> > if (type == BPF_WRITE)
> > - *insn++ = BPF_STX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > - offsetof(struct sock, sk_bound_dev_if));
> > + *insn++ = BPF_COPY_STORE(BPF_W, si,
> > + offsetof(struct sock, sk_bound_dev_if));
> > else
> > *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > offsetof(struct sock, sk_bound_dev_if));
> > @@ -9656,8 +9663,8 @@ u32 bpf_sock_convert_ctx_access(enum bpf_access_type type,
> > BUILD_BUG_ON(sizeof_field(struct sock, sk_mark) != 4);
> >
> > if (type == BPF_WRITE)
> > - *insn++ = BPF_STX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > - offsetof(struct sock, sk_mark));
> > + *insn++ = BPF_COPY_STORE(BPF_W, si,
> > + offsetof(struct sock, sk_mark));
> > else
> > *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > offsetof(struct sock, sk_mark));
> > @@ -9667,8 +9674,8 @@ u32 bpf_sock_convert_ctx_access(enum bpf_access_type type,
> > BUILD_BUG_ON(sizeof_field(struct sock, sk_priority) != 4);
> >
> > if (type == BPF_WRITE)
> > - *insn++ = BPF_STX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > - offsetof(struct sock, sk_priority));
> > + *insn++ = BPF_COPY_STORE(BPF_W, si,
> > + offsetof(struct sock, sk_priority));
> > else
> > *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> > offsetof(struct sock, sk_priority));
> > @@ -9933,10 +9940,12 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> > offsetof(S, TF)); \
> > *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(S, F), tmp_reg, \
> > si->dst_reg, offsetof(S, F)); \
> > - *insn++ = BPF_STX_MEM(SIZE, tmp_reg, si->src_reg, \
> > + *insn++ = BPF_RAW_INSN(SIZE | BPF_MEM | BPF_CLASS(si->code), \
> > + tmp_reg, si->src_reg, \
>
> the macro didn't work here because of 'tmp_reg' ?
Yes, macro uses (si)->dst_reg in this position.
There are 11 places where this macro applies.
There are 4 places where `tmp_reg` is used for destination:
- 2 in cgroup.c
- 2 in filter.c
I opted not to add new macro to common headers (given that it has very
narrow purpose and not very descriptive name) and use BPF_RAW_INSN in
these cases.
[...]
