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@xxxxxxxxx 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);
