Re: [PATCH bpf-next v5 1/2] bpf: Fix attaching fentry/fexit/fmod_ret/lsm to modules

[Viktor Malik <vmalik@redhat.com>]

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
>