On 6/21/23 12:18 PM, Song Liu wrote:
On Jun 21, 2023, at 1:52 AM, Petr Mladek <pmladek@xxxxxxxx> wrote:
On Tue 2023-06-20 22:36:14, Song Liu wrote:
On Jun 19, 2023, at 4:32 AM, Petr Mladek <pmladek@xxxxxxxx> 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@xxxxxxxxxx> 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
