Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kmod: maintain syscall metadata sections in kpatch syscall macros #1376

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

joe-lawrence
Copy link
Contributor

The KPATCH_SYSCALL_DEFINEn macros in kpatch-syscall.h do not provide the same syscall metadata (saved in the __syscalls_metadata and _ftrace_events ELF sections) as the kernel. These same macros also instruct kpatch-build to ignore changes to these sections. This works fine as long as there are other unmodified syscalls present in the object file. However, if not, the kpatch syscall macros may result in either metadata ELF sections not appearing in the patched object file. The create-diff-object program expects to encounter any ELF section that has been marked by KPATCH_IGNORE_SECTION in the patched object file.

To avoid this limitation, create dummy __syscalls_metadata and _ftrace_events entries for the kpatch-modified syscall. The specific values shouldn't matter since their sections will still be marked with KPATCH_IGNORE_SECTION and now their presence will be guarenteed for create-diff-object.

Closes: #1375 ("kpatch-build error when modifying an object file's only syscall")

Copy link
Member

@jpoimboe jpoimboe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than the trailing semicolon

static struct trace_event_call __used \
__section("_ftrace_events") \
*__event_enter_##sname = NULL; \
KPATCH_IGNORE_SECTION("_ftrace_events");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove trailing semicolon

The KPATCH_SYSCALL_DEFINEn macros in kpatch-syscall.h do not provide the
same syscall metadata (saved in the __syscalls_metadata and
_ftrace_events ELF sections) as the kernel.  These same macros also
instruct kpatch-build to ignore changes to these sections.  This works
fine as long as there are other unmodified syscalls present in the
object file.  However, if not, the kpatch syscall macros may result in
either metadata ELF sections not appearing in the patched object file.
The create-diff-object program expects to encounter any ELF section that
has been marked by KPATCH_IGNORE_SECTION in the patched object file.

To avoid this limitation, create dummy __syscalls_metadata and
_ftrace_events entries for the kpatch-modified syscall.  The specific
values shouldn't matter since their sections will still be marked with
KPATCH_IGNORE_SECTION and now their presence will be guarenteed for
create-diff-object.

Closes: dynup#1375 ("kpatch-build error when modifying an object file's only syscall")
Signed-off-by: Joe Lawrence <[email protected]>
@joe-lawrence
Copy link
Contributor Author

A few follow ups:

  • Since the kernel's include/linux/syscalls.h only defines SYSCALL_METADATA inside an #ifdef CONFIG_FTRACE_SYSCALLS block, should we do the same for KPATCH_SYSCALL_METADATA, too?
  • In my limited testing, at least on ppc64le, I have observed that the dummy entries do ensure a __syscalls_metadata section, but not an associated rela section. This can SEGFAULT create-diff-object's __kpatch_correlate_section() code as it's passed a NULL sec_patched pointer. Should we assign the dummy metadata entries a pointer to something real (like &jiffies) to force the rela section, too?

@jpoimboe
Copy link
Member

jpoimboe commented Mar 2, 2024

Since the kernel's include/linux/syscalls.h only defines SYSCALL_METADATA inside an #ifdef CONFIG_FTRACE_SYSCALLS block, should we do the same for KPATCH_SYSCALL_METADATA, too?

I think either way is fine.

In my limited testing, at least on ppc64le, I have observed that the dummy entries do ensure a __syscalls_metadata section, but not an associated rela section. This can SEGFAULT create-diff-object's __kpatch_correlate_section() code as it's passed a NULL sec_patched pointer. Should we assign the dummy metadata entries a pointer to something real (like &jiffies) to force the rela section, too?

Hm, how does it get passed a NULL sec_patched pointer?

@joe-lawrence
Copy link
Contributor Author

In my limited testing, at least on ppc64le, I have observed that the dummy entries do ensure a __syscalls_metadata section, but not an associated rela section. This can SEGFAULT create-diff-object's __kpatch_correlate_section() code as it's passed a NULL sec_patched pointer. Should we assign the dummy metadata entries a pointer to something real (like &jiffies) to force the rela section, too?

Hm, how does it get passed a NULL sec_patched pointer?

Program received signal SIGSEGV, Segmentation fault.
0x0000000010004cf8 in __kpatch_correlate_section (sec_orig=0x10057060, sec_patched=0x0) at create-diff-object.c:1005
1005            CORRELATE_ELEMENT(sec_orig, sec_patched, "section");

(gdb) bt
#0  0x0000000010004cf8 in __kpatch_correlate_section (sec_orig=0x10057060, sec_patched=0x0) at create-diff-object.c:1005
#1  0x000000001000523c in kpatch_correlate_section (sec_orig=0x10056fc0, sec_patched=0x10111cf0) at create-diff-object.c:1032
#2  0x000000001000541c in kpatch_correlate_sections (seclist_orig=0x10054100, seclist_patched=0x1010f3f0) at create-diff-object.c:1071
#3  0x0000000010006d20 in kpatch_correlate_elfs (kelf_orig=0x100540f0, kelf_patched=0x1010f3e0) at create-diff-object.c:1505
#4  0x000000001000f6d4 in main (argc=8, argv=0x7fffffffec48) at create-diff-object.c:3992

(gdb) up
#1  0x000000001000523c in kpatch_correlate_section (sec_orig=0x10056fc0, sec_patched=0x10111cf0) at create-diff-object.c:1032
1032                    __kpatch_correlate_section(sec_orig->rela, sec_patched->rela);

(gdb) list
1027            if (is_rela_section(sec_orig)) {
1028                    __kpatch_correlate_section(sec_orig->base, sec_patched->base);
1029                    sec_orig = sec_orig->base;
1030                    sec_patched = sec_patched->base;
1031            } else if (sec_orig->rela) {
1032                    __kpatch_correlate_section(sec_orig->rela, sec_patched->rela);
1033            }
1034
1035            if (sec_orig->secsym)
1036                    kpatch_correlate_symbol(sec_orig->secsym, sec_patched->secsym);

(gdb) p sec_orig->name
$1 = 0x7ffff7c5ff75 "__syscalls_metadata"
(gdb) p sec_orig->rela
$2 = (struct section *) 0x10057060
(gdb) p sec_patched->name
$3 = 0x7ffff7bef861 "__syscalls_metadata"
(gdb) p sec_patched->rela
$4 = (struct section *) 0x0

@jpoimboe
Copy link
Member

jpoimboe commented Mar 4, 2024

Your version of the code seems to be missing the fix for that: 9fac261 ?

@joe-lawrence
Copy link
Contributor Author

Your version of the code seems to be missing the fix for that: 9fac261 ?

Ah yes, good find, my older kpatch-build was indeed missing that commit (which fixes the problem).

Anything more to tweak for this PR?

@joe-lawrence joe-lawrence merged commit 4077d87 into dynup:master Mar 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kpatch-build error when modifying an object file's only syscall
2 participants