lkml.org 
[lkml]   [2020]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRE: [PATCH bpf-next v4 06/11] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction
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 <yhs@fb.com>
> Signed-off-by: Brendan Jackman <jackmanb@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.fastabend@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;
> }
>

\
 
 \ /
  Last update: 2020-12-08 06:33    [W:0.296 / U:0.808 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site