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.

[sumanthkorikkar]

Added v3:

• kpatch_skip_symbol_inclusion_from_relasec() prevents the initial symbol
  inclusion for .rela__bug_table. The relocations in this section are instead
  handled later in kpatch_regenerate_special_section().
• kpatch_regenerate_special_section() includes only the bug table group entries
  corresponding to changed function symbols and skips unnecessary bug table
  entries.
• recalculate_bug_table_rela_addend() adjusts the __bug_table + offset addends
  in all relocation sections that reference a valid bug table entry, ensuring
  they point to the updated bug entry offset.

[Tomcat-42]

/packit test

[joe-lawrence]

@sumanthkorikkar : I haven't been following this PR closely, is this only relevant for debug kernels? Just wondering if this would be needed as a bug-fix type change in light of Josh's recent maintenance-only mode announcement (#1499). I was thinking of tagging a v0.9.12 to capture the last round of fixes and enhancements since the last version, and didn't know if this might qualify as well.

[joe-lawrence]

@Tomcat-42 : any idea why testing-farm:RHEL-8.10.0-Nightly-ppc64le:integration-rhel failed? The other tests (save for the known Fedora infra issues) all passed, so I'm assuming something intermittent?

[Tomcat-42]

@joe-lawrence Yep, I guess it was some transient failure on infra, I've re-runned the job and it passed.

The Fedora integration-upstream jobs are fixed in master (except ppc64le), so maybe @sumanthkorikkar wants to do a rebase? (I've noticed that push by maintainers is disabled in his fork)

[sumanthkorikkar]

@sumanthkorikkar : I haven't been following this PR closely, is this only relevant for debug kernels? Just wondering if this would be needed as a bug-fix type change in light of Josh's recent maintenance-only mode announcement (#1499). I was thinking of tagging a v0.9.12 to capture the last round of fixes and enhancements since the last version, and didn't know if this might qualify as well.

Thanks for the information. It would be nice to have this PR as bug fix in the kpatch. This is relevant for only debug kernel and problem occurs in x86 and s390

Relevant commit:
x86 - kernel version v6.19 - 5b472b6e5bd9 ("x86_64/bug: Implement __WARN_printf()") - v6.19
s390 - 04dabb4261c3 ("s390/bug: Implement __WARN_printf()")

[sumanthkorikkar]

@joe-lawrence Yep, I guess it was some transient failure on infra, I've re-runned the job and it passed.

The Fedora integration-upstream jobs are fixed in master (except ppc64le), so maybe @sumanthkorikkar wants to do a rebase? (I've noticed that push by maintainers is disabled in his fork)

@Tomcat-42 , no - i have already done the rebasing for v3 here. Let me know, if I have to do anything else with my branch. Thank you.