Re: [PATCH bpf-next] bpf: support nested rcu critical sections

[Puranjay Mohan <puranjay@kernel.org>]

Eduard Zingerman <eddyz87@gmail.com> writes:

> On Tue, 2025-09-16 at 11:36 +0000, Puranjay Mohan wrote:
>> Currently, nested rcu critical sections are rejected by the verifier and
>> rcu_lock state is managed by a boolean variable. Add support for nested
>> rcu critical sections by make active_rcu_locks a counter similar to
>> active_preempt_locks. bpf_rcu_read_lock() increments this counter and
>> bpf_rcu_read_unlock() decrements it, MEM_RCU -> PTR_UNTRUSTED transition
>> happens when active_rcu_locks drops to 0.
>> 
>> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
>> ---
>
> [...]
>
>> @@ -13874,22 +13874,22 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>  		}
>>  
>>  		if (rcu_lock) {
>> -			verbose(env, "nested rcu read lock (kernel function %s)\n", func_name);
>> -			return -EINVAL;
>> +			env->cur_state->active_rcu_locks++;
>>  		} else if (rcu_unlock) {
>> -			bpf_for_each_reg_in_vstate_mask(env->cur_state, state, reg, clear_mask, ({
>> -				if (reg->type & MEM_RCU) {
>> -					reg->type &= ~(MEM_RCU | PTR_MAYBE_NULL);
>> -					reg->type |= PTR_UNTRUSTED;
>> -				}
>> -			}));
>> -			env->cur_state->active_rcu_lock = false;
>> +			if (--env->cur_state->active_rcu_locks == 0) {
>> +				bpf_for_each_reg_in_vstate_mask(env->cur_state, state, reg, clear_mask, ({
>> +					if (reg->type & MEM_RCU) {
>> +						reg->type &= ~(MEM_RCU | PTR_MAYBE_NULL);
>> +						reg->type |= PTR_UNTRUSTED;
>> +					}
>> +				}));
>> +			}
>>  		} else if (sleepable) {
>>  			verbose(env, "kernel func %s is sleepable within rcu_read_lock region\n", func_name);
>>  			return -EACCES;
>>  		}
>>  	} else if (rcu_lock) {
>> -		env->cur_state->active_rcu_lock = true;
>> +		env->cur_state->active_rcu_locks++;
>>  	} else if (rcu_unlock) {
>>  		verbose(env, "unmatched rcu read unlock (kernel function %s)\n", func_name);
>>  		return -EINVAL;
>
> Nit: active_rcu_locks increment in two places can be avoided e.g. as follows:
>
>         if (rcu_lock) {
>                 env->cur_state->active_rcu_locks++;
>         } else if (rcu_unlock) {
>                 struct bpf_func_state *state;
>                 struct bpf_reg_state *reg;
>                 u32 clear_mask = (1 << STACK_SPILL) | (1 << STACK_ITER);
>
>                 if (env->cur_state->active_rcu_locks == 0) {
>                         verbose(private_data: env, fmt: "unmatched rcu read unlock (kernel function %s)\n", func_name);
>                         return -EINVAL;
>                 }
>                 if (--env->cur_state->active_rcu_locks == 0) {
>                         bpf_for_each_reg_in_vstate_mask(env->cur_state, state, reg, clear_mask, ({
>                                 if (reg->type & MEM_RCU) {
>                                         reg->type &= ~(MEM_RCU | PTR_MAYBE_NULL);
>                                         reg->type |= PTR_UNTRUSTED;
>                                 }
>                         }));
>                 }
>         } else if (sleepable) {
>                 verbose(private_data: env, fmt: "kernel func %s is sleepable within rcu_read_lock region\n", func_name);
>                 return -EACCES;
>         }
>
>         if (in_rbtree_lock_required_cb(env) && (rcu_lock || rcu_unlock)) {
>                 verbose(private_data: env, fmt: "Calling bpf_rcu_read_{lock,unlock} in unnecessary rbtree callback\n");
>                 return -EACCES;
>         }
>
> [...]

I agree, this looks better. Will use it in the next version.

Thanks,
Puranjay