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

[Viktor Malik <vmalik@redhat.com>]

On 3/10/23 20:16, Luis Chamberlain wrote:
> Please add linux-modules in the future. My review below.

Sorry for missing that, I'll add it next time.

> 
> On Fri, Mar 10, 2023 at 08:40:59AM +0100, Viktor Malik wrote:

[snip]

> My review of the critical part below.
> 
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 45a082284464..3905bb20b9a1 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -18432,8 +18434,17 @@ int bpf_check_attach_target(struct bpf_verifier_log *log,
>>   			else
>>   				addr = (long) tgt_prog->aux->func[subprog]->bpf_func;
>>   		} else {
>> -			addr = kallsyms_lookup_name(tname);
>> +			if (btf_is_module(btf)) {
>> +				mod = btf_try_get_module(btf);
>> +				if (mod)
>> +					addr = find_kallsyms_symbol_value(mod, tname);
>> +				else
>> +					addr = 0;
>> +			} else {
>> +				addr = kallsyms_lookup_name(tname);
>> +			}
>>   			if (!addr) {
>> +				module_put(mod);
>>   				bpf_log(log,
>>   					"The address of function %s cannot be found\n",
>>   					tname);
> 
> If btf_modules linked list is ensured to not remove the btf module
> during this operation, sure this is safe, as per the new guidelines I've
> posted for try_module_get() this seems to be using try_module_get()
> using the implied protection.

I believe that is the case. btf_try_get_module checks the
BTF_F_MODULE_LIVE flag before calling try_module_get and the flag is set
only when the module notifier callback is called with MODULE_STATE_LIVE.
In addition, all BTF module operations are called under the same mutex,
so the module cannot be removed in-between.

> 
> Please review the docs. *If* it respects that usage then feel free to
> add:
> 
> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

Thanks for the review. Unless there are more change requests, I'll leave
it up to the maintainers to add the tag.

Viktor

> 
>    Luis
>