> 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. 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. 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 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. 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
