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

[Viktor Malik <vmalik@redhat.com>]

On 12/12/22 18:08, Yonghong Song wrote:
> 
> 
> On 12/12/22 4:59 AM, 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 from it and search for the function address in
>> the module.
>>
>> Signed-off-by: Viktor Malik <vmalik@redhat.com>
>> ---
>>   kernel/bpf/verifier.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index a5255a0dcbb6..d646c5263bc5 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/bpf_lsm.h>
>>   #include <linux/btf_ids.h>
>>   #include <linux/poison.h>
>> +#include "../module/internal.h"
>>   #include "disasm.h"
>> @@ -16478,6 +16479,7 @@ int bpf_check_attach_target(struct 
>> bpf_verifier_log *log,
>>       const char *tname;
>>       struct btf *btf;
>>       long addr = 0;
>> +    struct module *mod;
>>       if (!btf_id) {
>>           bpf_log(log, "Tracing programs must provide btf_id\n");
>> @@ -16645,7 +16647,19 @@ 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)) {
>> +                preempt_disable();
>> +                mod = btf_try_get_module(btf);
>> +                if (mod) {
>> +                    addr = find_kallsyms_symbol_value(mod, tname);
>> +                    module_put(mod);
>> +                } else {
>> +                    addr = 0;
>> +                }
>> +                preempt_enable();
> 
> What if module is unloaded right after preempt_enabled so 'addr' becomes 
> invalid? Is this a corner case we should consider?

IIUC, if 'addr' becomes invalid, the attachment will eventually fail.

So I'd say that there's no need to consider that case here, it's not
considered for kallsyms_lookup_name below (which may call
module_kallsyms_lookup_name) either.

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