create-diff-object: Remove undefined function symbols

[sumanthkorikkar]

When building shadow-pid.patch on a debug kernel, it generates __bug_table, which contains an array of struct bug_entries.

.rela__bug_table contains references to bug address, line number and column.

create-diff-object identifies that .text.kernel_clone has changed and it includes .rela.text.kernel_clone rela section. Then later, it includes all symbols (in kpatch_include_symbols()) associated with it, which ends up including __bug_table and its rela section .rela__bug_table. Then, all the function symbols associated with .rela__bug_table is included irrespective of whether it's section is included or not.

This leads to the following modpost errors:
kernel/fork.o: changed function: kernel_clone
kernel/exit.o: changed function: do_exit
fs/proc/array.o: changed function: proc_pid_status make -C /root/linux M=/root/.kpatch/tmp/patch CFLAGS_MODULE='' make[1]: Entering directory '/root/linux'
make[2]: Entering directory '/root/.kpatch/tmp/patch'
LDS kpatch.lds
CC [M] patch-hook.o
LD [M] test-shadow-newpid.o
MODPOST Module.symvers
WARNING: modpost: missing MODULE_DESCRIPTION() in test-shadow-newpid.o
ERROR: modpost: "replace_mm_exe_file" [test-shadow-newpid.ko] undefined!
ERROR: modpost: "put_task_stack" [test-shadow-newpid.ko] undefined!
ERROR: modpost: "release_task" [test-shadow-newpid.ko] undefined!
ERROR: modpost: "set_mm_exe_file" [test-shadow-newpid.ko] undefined!

Examining the /root/.kpatch/patch/ directory reveals, these symbols are never referenced in any relas.

readelf -Ws output.o |
grep -E 'put_task_stack|replace_mm_exe_file|release_task|set_mm_exe_file'
27: 0000000000000000 0 SECTION LOCAL DEFAULT 36 .rodata.release_task.str1.2
45: 0000000000000000 0 SECTION LOCAL DEFAULT 55 .rodata.set_mm_exe_file.str1.2
47: 0000000000000000 0 SECTION LOCAL DEFAULT 57 .rodata.replace_mm_exe_file.str1.2
234: 0000000000000000 0 FUNC GLOBAL DEFAULT UND replace_mm_exe_file
254: 0000000000000000 0 FUNC GLOBAL DEFAULT UND put_task_stack
263: 0000000000000000 0 FUNC GLOBAL DEFAULT UND release_task
269: 0000000000000000 0 FUNC GLOBAL DEFAULT UND set_mm_exe_file

readelf -Wr output.o |
grep -E 'put_task_stack|replace_mm_exe_file|release_task|set_mm_exe_file'

Hence, exclude these unreferenced symbols to avoid modpost errors.

Fix:

• Identify all function symbols present in __bug_table and track their symbol indices.
• Exclude .rela__bug_table and .rela__mcount_loc, and for all other relocation sections, check whether any of these symbol indices are actually referenced.
• If a symbol index is never referenced in any relevant relocation section and the symbol’s section is not included in the patch, exclude the symbol from being added.

PATCH RFC v2:
Note: Skipped need_klp_reloc()/kpatch_create_intermediate_sections()
check for .rela__bug_table section.
Reason: The function symbols that were not referenced by any sections
other than .rela__bug_table were being initialized with include = 0 (via
rela->sym->include = 0). As a result, kpatch_migrate_included_elements()
did not migrate these function symbols into kelf_out. However, later in
kpatch_create_intermediate_sections(), when parsing the .rela__bug_table
relasec and evaluating each symbol in need_klp_reloc(), the
code ended up using the previous rela->sym reference (which had already
been torn down). Since that symbol had its include field set to 0, the
dereference led to a segmentation fault. To prevent this, the
.rela__bug_table section is excluded from consideration in
kpatch_migrate_included_elements(). Additionally, if a function is
modified, the assumption is that, it will be referenced by other
relasec.

[sumanthkorikkar]

The dynamic-debug-jump-label-issue-1253 test is failing with this PR. I will investigate the failing test. Let me know, if there are better approaches or suggestions to solve this problem. Thank you

[sumanthkorikkar]

Added RFC patch v2

[sumanthkorikkar]

Other possible fix/hack:

--- a/kpatch-build/create-diff-object.c
+++ b/kpatch-build/create-diff-object.c
@@ -1884,6 +1884,8 @@ static void kpatch_include_section(struct section *sec)
        if (!sec->rela)
                return;
        sec->rela->include = 1;
+       if (!strcmp(sec->rela->name, ".rela__bug_table"))
+               return;
        list_for_each_entry(rela, &sec->rela->relas, list)
                kpatch_include_symbol(rela->sym);
 }

Required rela symbols are included later in kpatch_regenerate_special_section() anyways.

/*
 * Copy all relas in the group.  It's possible that the relas
 * aren't sorted (e.g. .rela.fixup), so go through the entire
 * rela list each time.
 */
list_for_each_entry_safe(rela, safe, &relasec->relas, list) {
        if (rela->offset >= src_offset &&
            rela->offset < src_offset + group_size) {
                /* copy rela entry */
                list_del(&rela->list);
                list_add_tail(&rela->list, &newrelas);

                rela->offset -= src_offset - dest_offset;
                rela->rela.r_offset = rela->offset;

                rela->sym->include = 1;

                if (!strcmp(special->name, ".fixup"))
                        kpatch_update_ex_table_addend(kelf, special,
                                                      src_offset,
                                                      dest_offset,
                                                      group_size);
        }
}

So, my assumption is that it might be safe to unconditionally skip relocation symbols for .rela__bug_table during the initial pass in kpatch_include_section(). Later, kpatch_generate_special_section() will explicitly add the required symbols (though not recursively). And if any of the referenced rela->sym->sec entries are actually needed, they will still get included through other sections associated with changed functions.

[jpoimboe]

I saw this same problem with klp-build (the kpatch-build replacement has just been upstreamed for x86), see linux commit f2dba60339a6 ("objtool/klp: Fix bug table handling for __WARN_printf()").

The fix for klp-build was simple, but it might be harder here.

The problem is that WARN() has been converted to a static call, which passes a reference to its bug table entry (in __bug_table) to the static call, which looks like:

 205:   48 8d 3d 00 00 00 00    lea    0x0(%rip),%rdi        # 20c <ftrace_make_nop+0x5c>       208: R_X86_64_PC32      __bug_table+0x3c
 20c:   e8 00 00 00 00          call   211 <ftrace_make_nop+0x61>       20d: R_X86_64_PLT32     __SCT__WARN_trap-0x4

Notice the lea instruction's reference to the bug table. That direct reference from a function to a special section is new, and that's causing create-diff-object to incorrectly pull in the entire __bug_table. Any fix here will need to make sure that bug table reference still points to the same entry after kpatch_regenerate_special_section().

I haven't looked to see how hard that would be. kpatch-build for x86 is basically deprecated at this point.

[jpoimboe]

WRT to this PR in particular, I don't think we want to keep all those extra bug table entries.

[sumanthkorikkar]

I saw this same problem with klp-build (the kpatch-build replacement has just been upstreamed for x86), see linux commit f2dba60339a6 ("objtool/klp: Fix bug table handling for __WARN_printf()").

The fix for klp-build was simple, but it might be harder here.

The problem is that WARN() has been converted to a static call, which passes a reference to its bug table entry (in __bug_table) to the static call, which looks like:

 205:   48 8d 3d 00 00 00 00    lea    0x0(%rip),%rdi        # 20c <ftrace_make_nop+0x5c>       208: R_X86_64_PC32      __bug_table+0x3c
 20c:   e8 00 00 00 00          call   211 <ftrace_make_nop+0x61>       20d: R_X86_64_PLT32     __SCT__WARN_trap-0x4

Notice the lea instruction's reference to the bug table. That direct reference from a function to a special section is new, and that's causing create-diff-object to incorrectly pull in the entire __bug_table. Any fix here will need to make sure that bug table reference still points to the same entry after kpatch_regenerate_special_section().

Thanks for the inputs. I will recheck it.