Re: [PATCH bpf-next v3 1/2] bpf: Enforce RCU protection for KF_RCU_PROTECTED

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2025-09-17 at 03:27 +0000, Kumar Kartikeya Dwivedi wrote:
> Currently, KF_RCU_PROTECTED only applies to iterator APIs and that too
> in a convoluted fashion: the presence of this flag on the kfunc is used
> to set MEM_RCU in iterator type, and the lack of RCU protection results
> in an error only later, once next() or destroy() methods are invoked on
> the iterator. While there is no bug, this is certainly a bit
> unintuitive, and makes the enforcement of the flag iterator specific.
> 
> In the interest of making this flag useful for other upcoming kfuncs,
> e.g. scx_bpf_cpu_curr() [0][1], add enforcement for invoking the kfunc
> in an RCU critical section in general.
> 
> This would also mean that iterator APIs using KF_RCU_PROTECTED will
> error out earlier, instead of throwing an error for lack of RCU CS
> protection when next() or destroy() methods are invoked.
> 
> In addition to this, if the kfuncs tagged KF_RCU_PROTECTED return a
> pointer value, ensure that this pointer value is only usable in an RCU
> critical section. There might be edge cases where the return value is
> special and doesn't need to imply MEM_RCU semantics, but in general, the
> assumption should hold for the majority of kfuncs, and we can revisit
> things if necessary later.
> 
>   [0]: https://lore.kernel.org/all/20250903212311.369697-3-christian.loehle@arm.com
>   [1]: https://lore.kernel.org/all/20250909195709.92669-1-arighi@nvidia.com
> 
> Tested-by: Andrea Righi <arighi@nvidia.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> @@ -14037,6 +14045,8 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>  
>  			if (meta.func_id == special_kfunc_list[KF_bpf_get_kmem_cache])
>  				regs[BPF_REG_0].type |= PTR_UNTRUSTED;
> +			else if (is_kfunc_rcu_protected(&meta))
> +				regs[BPF_REG_0].type |= MEM_RCU;
>  
>  			if (is_iter_next_kfunc(&meta)) {
>  				struct bpf_reg_state *cur_iter;

The code below this hunk looks as follows:

			if (is_iter_next_kfunc(&meta)) {
				struct bpf_reg_state *cur_iter;

				cur_iter = get_iter_from_state(env->cur_state, &meta);

				if (cur_iter->type & MEM_RCU) /* KF_RCU_PROTECTED */
					regs[BPF_REG_0].type |= MEM_RCU;
				else
					regs[BPF_REG_0].type |= PTR_TRUSTED;
			}

Do we want to reduce it to:

			if (meta.func_id == special_kfunc_list[KF_bpf_get_kmem_cache])
				regs[BPF_REG_0].type |= PTR_UNTRUSTED;
			else if (is_kfunc_rcu_protected(&meta))
				regs[BPF_REG_0].type |= MEM_RCU;
			else if (is_iter_next_kfunc(&meta))
				regs[BPF_REG_0].type |= PTR_TRUSTED;

And mark relevant iterator next (and destroy?) functions as KF_RCU_PROTECTED?
(bpf_iter_css_next, bpf_iter_task_next, bpf_iter_scx_dsq_next).

I ask, because setting |= MEM_RCU in two places of this if branch
looks a bit iffy.

[...]