On 2/13/23 19:33, Jiri Olsa wrote:
On Mon, Feb 13, 2023 at 04:59:58PM +0100, Viktor Malik wrote:
SNIP
@@ -248,8 +223,6 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
ret = bpf_arch_text_poke(ip, BPF_MOD_CALL, NULL, new_addr);
}
- if (ret)
- bpf_trampoline_module_put(tr);
return ret;
}
@@ -719,8 +692,11 @@ int bpf_trampoline_link_cgroup_shim(struct bpf_prog *prog,
bpf_lsm_find_cgroup_shim(prog, &bpf_func);
tr = bpf_trampoline_get(key, &tgt_info);
- if (!tr)
+ if (!tr) {
+ if (tgt_info.tgt_mod)
+ module_put(tgt_info.tgt_mod);
return -ENOMEM;
+ }
mutex_lock(&tr->mutex);
@@ -800,6 +776,14 @@ struct bpf_trampoline *bpf_trampoline_get(u64 key,
return NULL;
mutex_lock(&tr->mutex);
+ if (tgt_info->tgt_mod) {
+ if (tr->mod)
+ /* we already have the module reference, release tgt_info reference */
+ module_put(tgt_info->tgt_mod);
+ else
+ /* take ownership of the module reference */
+ tr->mod = tgt_info->tgt_mod;
this seems tricky, should we take and save module reference in bpf_prog
struct and release it when the program goes out? IIUC the module for
which the program was verified for should stay as long as the program
is loaded
You're right, it makes more sense that the module is associated with the
program, not with the trampoline. So we just save the mod reference into
prog->aux (in bpf_check_attach_target) and release it on bpf_prog_put,
just before the program is freed.
Does that make sense? Anything else to be aware of comes to mind?
Thanks!
Viktor
jirka
+ }
if (tr->func.addr)
goto out;
@@ -819,6 +803,10 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
mutex_lock(&trampoline_mutex);
if (!refcount_dec_and_test(&tr->refcnt))
goto out;
+ if (tr->mod) {
+ module_put(tr->mod);
+ tr->mod = NULL;
+ }
WARN_ON_ONCE(mutex_is_locked(&tr->mutex));
for (i = 0; i < BPF_TRAMP_MAX; i++)
SNIP
