Re: [PATCH bpf-next v2 4/8] bpf: Add kfunc bpf_rcu_read_lock/unlock()

[Yonghong Song <yhs@meta.com>]

On 11/8/22 8:56 AM, Alexei Starovoitov wrote:
> On 11/7/22 11:41 PM, Yonghong Song wrote:
>> Add two kfunc's bpf_rcu_read_lock() and bpf_rcu_read_unlock(). These 
>> two kfunc's
>> can be used for all program types. A new kfunc hook type 
>> BTF_KFUNC_HOOK_GENERIC
>> is added which corresponds to prog type BPF_PROG_TYPE_UNSPEC, 
>> indicating the
>> kfunc intends to be used for all prog types.
>>
>> The kfunc bpf_rcu_read_lock() is tagged with new flag KF_RCU_LOCK and
>> bpf_rcu_read_unlock() with new flag KF_RCU_UNLOCK. These two new flags
>> are used by the verifier to identify these two helpers.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   include/linux/bpf.h  |  3 +++
>>   include/linux/btf.h  |  2 ++
>>   kernel/bpf/btf.c     |  8 ++++++++
>>   kernel/bpf/helpers.c | 25 ++++++++++++++++++++++++-
>>   4 files changed, 37 insertions(+), 1 deletion(-)
>>
>> For new kfuncs, I added KF_RCU_LOCK and KF_RCU_UNLOCK flags to
>> indicate a helper could be bpf_rcu_read_lock/unlock(). This could
>> be a waste for kfunc flag space as the flag is used to identify
>> one helper. Alternatively, we might identify kfunc based on
>> btf_id. Any suggestions are welcome.
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index 5011cb50abf1..b4bbcafd1c9b 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -2118,6 +2118,9 @@ bool bpf_prog_has_kfunc_call(const struct 
>> bpf_prog *prog);
>>   const struct btf_func_model *
>>   bpf_jit_find_kfunc_model(const struct bpf_prog *prog,
>>                const struct bpf_insn *insn);
>> +void bpf_rcu_read_lock(void);
>> +void bpf_rcu_read_unlock(void);
>> +
>>   struct bpf_core_ctx {
>>       struct bpf_verifier_log *log;
>>       const struct btf *btf;
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index d80345fa566b..8783ca7e6079 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -51,6 +51,8 @@
>>   #define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer 
>> arguments */
>>   #define KF_SLEEPABLE    (1 << 5) /* kfunc may sleep */
>>   #define KF_DESTRUCTIVE  (1 << 6) /* kfunc performs destructive 
>> actions */
>> +#define KF_RCU_LOCK     (1 << 7) /* kfunc does rcu_read_lock() */
>> +#define KF_RCU_UNLOCK   (1 << 8) /* kfunc does rcu_read_unlock() */
> 
> Please don't use KF flags for these. It's not going to scale.
> Compare btf_id instead.

Will do. Kumar has a suggestion like:
   https://lore.kernel.org/bpf/20221107230950.7117-17-memxor@gmail.com
which I will explore.