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

[Viktor Malik <vmalik@redhat.com>]

On 12/7/22 01:10, Alexei Starovoitov wrote:
> On Mon, Dec 05, 2022 at 04:26:05PM +0100, Viktor Malik wrote:
>> When attaching fentry/fexit/fmod_ret/lsm to a function located in a
>> module without specifying the target program, the verifier tries to find
>> the address to attach to in kallsyms. This is always done by searching
>> the entire kallsyms, not respecting the module in which the function is
>> located.
>>
>> This approach causes an incorrect attachment address to be computed if
>> the function to attach to is shadowed by a function of the same name
>> located earlier in kallsyms.
>>
>> Since the attachment must contain the BTF of the program to attach to,
>> we may extract the module name from it (if the attach target is a
>> module) and search for the function address in the correct module.
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> Acked-by: Hao Luo <haoluo@google.com>
>> ---
>>   include/linux/btf.h   | 1 +
>>   kernel/bpf/btf.c      | 5 +++++
>>   kernel/bpf/verifier.c | 5 ++++-
>>   3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/btf.h b/include/linux/btf.h
>> index cbd6e4096f8c..b7b791d1f3d6 100644
>> --- a/include/linux/btf.h
>> +++ b/include/linux/btf.h
>> @@ -188,6 +188,7 @@ u32 btf_obj_id(const struct btf *btf);
>>   bool btf_is_kernel(const struct btf *btf);
>>   bool btf_is_module(const struct btf *btf);
>>   struct module *btf_try_get_module(const struct btf *btf);
>> +const char *btf_module_name(const struct btf *btf);
>>   u32 btf_nr_types(const struct btf *btf);
>>   bool btf_member_is_reg_int(const struct btf *btf, const struct btf_type *s,
>>   			   const struct btf_member *m,
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index c80bd8709e69..f78e8060efa6 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -7208,6 +7208,11 @@ bool btf_is_module(const struct btf *btf)
>>   	return btf->kernel_btf && strcmp(btf->name, "vmlinux") != 0;
>>   }
>>   
>> +const char *btf_module_name(const struct btf *btf)
>> +{
>> +	return btf->name;
>> +}
> 
> It feels that btf->name is leaking a bit of implementation detail.
> How about doing:
> 
> struct module *btf_find_module(const struct btf *btf)
> {
>          reutrn find_module(btf->name);
> }
> 
>> +
>>   enum {
>>   	BTF_MODULE_F_LIVE = (1 << 0),
>>   };
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 1d51bd9596da..0c533db51f92 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -16483,7 +16483,10 @@ 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))
>> +				addr = kallsyms_lookup_name_in_module(btf_module_name(btf), tname);
> 
> and use find_kallsyms_symbol_value() here
> (with preempt_disable dance).
> There won't be a need for patch 1 too.
> 
> wdyt?

This makes sense to me maybe more than the current solution.
I'm not sure where it's best to do preempt_disable, though. Would you
rather do it here or inside find_kallsyms_symbol_value? Or should we
create a new wrapper for find_kallsyms_symbol_value, possibly outside of
kernel/module/internal.h?

Thanks!

> 
>> +			else
>> +				addr = kallsyms_lookup_name(tname);
>>   			if (!addr) {
>>   				bpf_log(log,
>>   					"The address of function %s cannot be found\n",
>> -- 
>> 2.38.1
>>
>