Re: [PATCH] kallsyms: let kallsyms_on_each_match_symbol match symbols exactly

[Yonghong Song <yhs@meta.com>]

On 6/21/23 12:18 PM, Song Liu wrote:
> 
> 
>> On Jun 21, 2023, at 1:52 AM, Petr Mladek <pmladek@suse.com> wrote:
>>
>> On Tue 2023-06-20 22:36:14, Song Liu wrote:
>>>> On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@suse.com> wrote:
>>>>
>>>> On Sun 2023-06-18 22:05:19, Song Liu wrote:
>>>>> On Sun, Jun 18, 2023 at 8:32 PM Leizhen (ThunderTown)
>>>>> <thunder.leizhen@huawei.com> wrote:
>>>
>>> [...]
> 
> [...]
> 
>>>
>>> I am not quite following the direction here. Do we need more
>>> work for this patch?
>>
>> Good question. I primary tried to add more details so that
>> we better understand the problem.
>>
>> Honestly, I do not know the answer. I am neither familiar with
>> kpatch nor with clang. Let me think loudly.
>>
>> kpatch produce livepatches by comparing binaries of the original
>> and fixed kernel. It adds a symbol into the livepatch when
>> the same symbol has different code in the two binaries.
>>
>> So one important question is how clang generates the suffix.
>> Is the suffix the same in every build? Is it the same even
>> after the function gets modified by a fix?
>>
>> If the suffix is always the same then then the full symbol name
>> would be better for kpatch. kpatch would get it for free.
>> And kpatch would not longer need to use "old_sympos".
> 
> This is pretty complicated.
> 
> 1. clang with LTO does not use the suffix to eliminated duplicated
> kallsyms, so old_sympos is still needed. Here is an example:
> 
> # grep init_once /proc/kallsyms
> ffffffff8120ba80 t init_once.llvm.14172910296636650566
> ffffffff8120ba90 t inode_init_once
> ffffffff813ea5d0 t bpf_user_rnd_init_once
> ffffffff813fd5b0 t init_once.llvm.17912494158778303782
> ffffffff813ffbf0 t init_once
> ffffffff813ffc60 t init_once
> ffffffff813ffc70 t init_once
> ffffffff813ffcd0 t init_once
> ffffffff813ffce0 t init_once
> 
> There are two "init_once" with suffix, but there are also ones
> without them.

This is correct. At LTO mode, when a static function/variable
is promoted to the global. The '.llvm.<hash>' is added to the
static function/variable name to form a global function name.
The '<hash>' here is computed based on IR of the compiled file
before LTO. So if one file is not modified, then <hash>
won't change after additional code change in other files.

> 
> 2. kpatch-build does "Building original source", "Building patched
> source", and then do binary diff of the two. From our experiments,
> the suffix doesn't change between the two builds. However, we need
> to match the build environment (path of kernel source, etc.) to
> make sure suffix from kpatch matches the kernel.

The goal here is to generate the same IR (hence <hash>) if
file content is not changed. This way, <hash> value will
change only for those changed files.

> 
> 3. The goal of this patch is NOT to resolve different suffix by
> llvm (.llvm.[0-9]+). Instead, we are trying fix issues like:
> 
> #  grep bpf_verifier_vlog /proc/kallsyms
> ffffffff81549f60 t bpf_verifier_vlog
> ffffffff8268b430 d bpf_verifier_vlog._entry
> ffffffff8282a958 d bpf_verifier_vlog._entry_ptr
> ffffffff82e12a1f d bpf_verifier_vlog.__already_done

Note that these symbols also exist non-LTO mode.

For example, with my particular config, I have


$ grep bpf_verifier_vlog System.map
ffffffff812f9370 T bpf_verifier_vlog
ffffffff82afa440 d bpf_verifier_vlog._entry
ffffffff83404218 d bpf_verifier_vlog._entry_ptr
$ grep bpf_output System.map
ffffffff81359c70 t perf_event_bpf_output
ffffffff821eeba0 t bpf_output
ffffffff82eec720 d bpf_output._entry
ffffffff83412c50 d bpf_output._entry_ptr
ffffffff84b0f334 d bpf_output.__already_done

bpf_output() is defined in net/core/lwt_bpf.c.

The original function:

static int bpf_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
         struct dst_entry *dst = skb_dst(skb);
         struct bpf_lwt *bpf;
         int ret;

         bpf = bpf_lwt_lwtunnel(dst->lwtstate);
         if (bpf->out.prog) {
                 ret = run_lwt_bpf(skb, &bpf->out, dst, NO_REDIRECT);
                 if (ret < 0)
                         return ret;
         }

         if (unlikely(!dst->lwtstate->orig_output)) {
                 pr_warn_once("orig_output not set on dst for prog %s\n",
                              bpf->out.name);
                 kfree_skb(skb);
                 return -EINVAL;
         }

         return dst->lwtstate->orig_output(net, sk, skb);
}


The function after preprocess:

static int bpf_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
  struct dst_entry *dst = skb_dst(skb);
  struct bpf_lwt *bpf;
  int ret;

  bpf = bpf_lwt_lwtunnel(dst->lwtstate);
  if (bpf->out.prog) {
   ret = run_lwt_bpf(skb, &bpf->out, dst, false);
   if (ret < 0)
    return ret;
  }

  if (__builtin_expect(!!(!dst->lwtstate->orig_output), 0)) {
   ({ bool __ret_do_once = !!(true); if (({ static bool 
__attribute__((__section__(".data.once"))) __already_done; bool 
__ret_cond = !!(__ret_do_once); bool __ret_once = false; if 
(__builtin_expect(!!(__ret_cond && !__already_done), 0)) { 
__already_done = true; __ret_once = true; } 
__builtin_expect(!!(__ret_once), 0); })) ({ do { if 
(__builtin_constant_p("\001" "4" "orig_output not set on dst for prog 
%s\n") && __builtin_constant_p(((void *)0))) { static const struct 
pi_entry _entry __attribute__((__used__)) = { .fmt = 
__builtin_constant_p("\001" "4" "orig_output not set on dst for prog 
%s\n") ? ("\001" "4" "orig_output not set on dst for prog %s\n") : 
((void *)0), .func = __func__, .file = "net/core/lwt_bpf.c", .line = 
154, .level = __builtin_constant_p(((void *)0)) ? (((void *)0)) : ((void 
*)0), .subsys_fmt_prefix = ((void *)0), }; static const struct pi_entry 
*_entry_ptr __attribute__((__used__)) 
__attribute__((__section__(".printk_index"))) = &_entry; } } while (0); 
_printk("\001" "4" "orig_output not set on dst for prog %s\n", 
bpf->out.name); }); __builtin_expect(!!(__ret_do_once), 0); });

   kfree_skb(skb);
   return -22;
  }

  return dst->lwtstate->orig_output(net, sk, skb);
}

In the above particular case, pr_warn_once() introduced
three static variables inside the funciton '__already_done',
'_entry' and '_entry_ptr'. To differentiate from
other potential static variables inside other functions,
these static variables are renamed to
<func_name>.<static_variable_name> in the above.

> 
> With existing code, compare_symbol_name() will match
> bpf_verifier_vlog to all these with CONFIG_LTO_CLANG.
> 
> We can probably teach compare_symbol_name() to not to match
> these suffix, as Zhen suggested.

This might not be a scalable solution unless there is a
way to capture usage of static variable inside the function
with some tools and feed them into an auto generated table
to be used by live patching.

Current comment in cleanup_symbol_name():

===
         /*
          * LLVM appends various suffixes for local functions and 
variables that
          * must be promoted to global scope as part of LTO.  This can break
          * hooking of static functions with kprobes. '.' is not a valid
          * character in an identifier in C. Suffixes observed:
          * - foo.llvm.[0-9a-f]+
          * - foo.[0-9a-f]+
          */
===

Based on my earlier study from llvm15 and llvm17, I only
observed 'foo.llvm.[0-9a-f]+'. Maybe earlier version of llvm
lto generates 'foo.[0-9a-f]+' format.

Note that CONFIG_CLANG_VERSION should be in the .config file
if the kernel is built with clang, which could be used
to further differentiate suffix format if necessary.

> 
> If this is not ideal, I am open to suggestions that can solve
> the problem.
> 
>> But if the suffix is different then kpatch has a problem.
>> kpatch would need to match symbols with different suffixes.
>> It would be easy for symbols which are unique after removing
>> the suffix. But it would be tricky for comparing symbols
>> which do not have an unique name. kpatch would need to find
>> which suffix in the original binary matches an other suffix
>> in the fixed binary. In this case, it might be easier
>> to use the stripped symbol names.
>>
>> And the suffix might be problematic also for source based
>> livepatches. They define struct klp_func in sources so
>> they would need to hardcode the suffix there. It might
>> be easy to keep using the stripped name and "old_sympos".
>>
>> I expect that this patch actually breaks the livepatch
>> selftests when the kernel is compiled with clang LTO.
> 
> Not really. This patch passes livepatch selftests.
> 
> Thanks,
> Song
>