Brendan Jackman wrote:
> The BPF_FETCH field can be set in bpf_insn.imm, for BPF_ATOMIC
> instructions, in order to have the previous value of the
> atomically-modified memory location loaded into the src register
> after an atomic op is carried out.
>
> Suggested-by: Yonghong Song <y...@fb.com>
> Signed-off-by: Brendan Jackman <jackm...@google.com>
> ---
I like Yonghong suggestion
#define BPF_ATOMIC_FETCH_ADD(SIZE, DST, SRC, OFF) \
BPF_ATOMIC(SIZE, DST, SRC, OFF, BPF_ADD | BPF_FETCH)
otherwise LGTM. One observation to consider below.
Acked-by: John Fastabend <john.fastab...@gmail.com>
> arch/x86/net/bpf_jit_comp.c | 4 ++++
> include/linux/filter.h | 1 +
> include/uapi/linux/bpf.h | 3 +++
> kernel/bpf/core.c | 13 +++++++++++++
> kernel/bpf/disasm.c | 7 +++++++
> kernel/bpf/verifier.c | 33 ++++++++++++++++++++++++---------
> tools/include/linux/filter.h | 11 +++++++++++
> tools/include/uapi/linux/bpf.h | 3 +++
> 8 files changed, 66 insertions(+), 9 deletions(-)
[...]
> @@ -3652,8 +3656,20 @@ static int check_atomic(struct bpf_verifier_env *env,
> int insn_idx, struct bpf_i
> return err;
>
> /* check whether we can write into the same memory */
> - return check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> - BPF_SIZE(insn->code), BPF_WRITE, -1, true);
> + err = check_mem_access(env, insn_idx, insn->dst_reg, insn->off,
> + BPF_SIZE(insn->code), BPF_WRITE, -1, true);
> + if (err)
> + return err;
> +
> + if (!(insn->imm & BPF_FETCH))
> + return 0;
> +
> + /* check and record load of old value into src reg */
> + err = check_reg_arg(env, insn->src_reg, DST_OP);
This will mark the reg unknown. I think this is fine here. Might be nice
to carry bounds through though if possible
> + if (err)
> + return err;
> +
> + return 0;
> }
>
