Re: [PATCH bpf-next v3 09/13] bpf: Mark OBJ_RELEASE argument as MEM_RCU when possible

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Sun, Aug 27, 2023 at 08:28:16AM -0700, Yonghong Song wrote:
> In previous selftests/bpf patch, we have
>   p = bpf_percpu_obj_new(struct val_t);
>   if (!p)
>           goto out;
> 
>   p1 = bpf_kptr_xchg(&e->pc, p);
>   if (p1) {
>           /* race condition */
>           bpf_percpu_obj_drop(p1);
>   }
> 
>   p = e->pc;
>   if (!p)
>           goto out;
> 
> After bpf_kptr_xchg(), we need to re-read e->pc into 'p'.
> This is due to that the second argument of bpf_kptr_xchg() is marked
> OBJ_RELEASE and it will be marked as invalid after the call.
> So after bpf_kptr_xchg(), 'p' is an unknown scalar,
> and the bpf program needs to reread from the map value.
> 
> This patch checks if the 'p' has type MEM_ALLOC and MEM_PERCPU,
> and if 'p' is RCU protected. If this is the case, 'p' can be marked
> as MEM_RCU. MEM_ALLOC needs to be removed since 'p' is not
> an owning reference any more. Such a change makes re-read
> from the map value unnecessary.
> 
> Note that re-reading 'e->pc' after bpf_kptr_xchg() might get
> a different value from 'p' if immediately before 'p = e->pc',
> another cpu may do another bpf_kptr_xchg() and swap in another value
> into 'e->pc'. If this is the case, then 'p = e->pc' may
> get either 'p' or another value, and race condition already exists.
> So removing direct re-reading seems fine too.
...
> +		} else if (func_id == BPF_FUNC_kptr_xchg && meta.ref_obj_id) {
> +			u32 ref_obj_id = meta.ref_obj_id;
> +			bool in_rcu = in_rcu_cs(env);
> +			struct bpf_func_state *state;
> +			struct bpf_reg_state *reg;
> +
> +			err = release_reference_state(cur_func(env), ref_obj_id);
> +			if (!err) {
> +				bpf_for_each_reg_in_vstate(env->cur_state, state, reg, ({
> +					if (reg->ref_obj_id == ref_obj_id) {
> +						if (in_rcu && (reg->type & MEM_ALLOC) && (reg->type & MEM_PERCPU)) {
> +							reg->ref_obj_id = 0;
> +							reg->type &= ~MEM_ALLOC;
> +							reg->type |= MEM_RCU;
> +						} else {
> +							mark_reg_invalid(env, reg);
> +						}
> +					}
> +				}));
> +			}
>  		} else if (meta.ref_obj_id) {
>  			err = release_reference(env, meta.ref_obj_id);

I think this open coded version of release_reference() can be safely folded into release_reference().
If it's safe to do for kptr_xchg() then it's safe to do for all KF_RELEASE kfuncs too
that call release_reference().
bpf_percpu_obj_drop() is the only such kfunc and converting its arg1
from MEM_ALLOC | MEM_PERCPU to MEM_RCU | MEM_PERCPU should be equally valid,
since bpf_percpu_obj_drop() is doing bpf_mem_free_rcu.

I'm planning to apply the whole set. Above nit can be a follow up.