RE: [PATCH bpf-next v4 06/11] bpf: Add BPF_FETCH field / create atomic_fetch_add instruction

[John Fastabend]

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@xxxxxx>
> Signed-off-by: Brendan Jackman <jackmanb@xxxxxxxxxx>
> ---

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@xxxxxxxxx>

>  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;
>  }
>