Re: [RFC bpf-next v2 2/9] bpf: add register and unregister functions for struct_ops.

[Martin KaFai Lau <martin.lau@linux.dev>]

On 9/15/23 6:14 PM, Kui-Feng Lee wrote:
> 
> 
> On 9/15/23 17:05, Martin KaFai Lau wrote:
>> On 9/12/23 11:14 PM, thinker.li@gmail.com wrote:
>>> +int register_bpf_struct_ops(struct bpf_struct_ops_mod *mod)
>>> +{
>>> +    struct bpf_struct_ops *st_ops = mod->st_ops;
>>> +    struct bpf_verifier_log *log;
>>> +    struct btf *btf;
>>> +    int err;
>>> +
>>> +    if (mod->st_ops == NULL ||
>>> +        mod->owner == NULL)
>>> +        return -EINVAL;
>>> +
>>> +    log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
>>> +    if (!log) {
>>> +        err = -ENOMEM;
>>> +        goto errout;
>>> +    }
>>> +
>>> +    log->level = BPF_LOG_KERNEL;
>>> +
>>> +    btf = btf_get_module_btf(mod->owner);
>>
>> Where is btf_put called?
>>
>> It is not stored anywhere in patch 2, so a bit confusing. I quickly looked at 
>> the following patches but also don't see the bpf_put.
> 
> It is my fault to use it without calling btf_put().
> I miss-understood the API, thought it doesn't increase refcount by
> mistake.
> 
>>
>>> +    if (!btf) {
>>> +        err = -EINVAL;
>>> +        goto errout;
>>> +    }
>>> +
>>> +    bpf_struct_ops_init_one(st_ops, btf, log);
>>> +    err = add_struct_ops(st_ops);
>>> +
>>> +errout:
>>> +    kfree(log);
>>> +
>>> +    return err;
>>> +}
>>> +EXPORT_SYMBOL(register_bpf_struct_ops);
>>> +
>>> +int unregister_bpf_struct_ops(struct bpf_struct_ops_mod *mod)
>>
>> It is not clear to me why the subsystem needs to explicitly call 
>> unregister_bpf_struct_ops(). Can it be done similar to the module kfunc 
>> support (the kfunc_set_tab goes away with the btf)?
> 
> It could be. However, registering to module notifier
> (register_module_notifier()) is more straight forward if we go
> this way. What do you think?

Right, but not sure if struct_ops needs to create yet another notifier 
considering there is already a btf_module_notify(). It is why the earlier 
question on btf_put because I was trying to understand if the struct_ops can go 
away together during btf_free. More on this next.

> 
>>
>> Related to this, does it need to maintain a global struct_ops array for all 
>> kernel module? Can the struct_ops be maintained under its corresponding module 
>> btf itself?
> 
> What is the purpose?
> We have a global struct_ops array already, although it is not
> per-module. For now, the number of struct_ops is pretty small.
> We have only one so far, and it is unlikely to grow fast in
> near future. It is probably a bit overkill to have
> per-module ones if this is what you mean.

The array size is not the concern.

The global struct_ops array was created before btf supporting kernel module. 
Since then, btf module and kfunc module support were added.

To maintain this global struct_ops array, it needs to register its own module 
notifier, maintains its own mutex_lock (in patch 5), and also the modified 
bpf_struct_ops_find*() is searching something under a specific btf module.

afaict, the current btf kfunc support has the infrastructure to do all these 
(for example, the global LIST_HEAD(btf_modules), btf_module_mutex, 
btf_module_notify()...etc). Why struct_ops needs to be special and reinvent 
something which looks very similar to btf kfunc? Did I missing something that 
struct_ops needs special handling?

> 
>>
>>> +{
>>> +    struct bpf_struct_ops *st_ops = mod->st_ops;
>>> +    int err;
>>> +
>>> +    err = remove_struct_ops(st_ops);
>>> +    if (!err && st_ops->uninit)
>>> +        err = st_ops->uninit();
>>> +
>>> +    return err;
>>> +}
>>> +EXPORT_SYMBOL(unregister_bpf_struct_ops);
>>
>>