Add Arm64 Support #1236
Add Arm64 Support #1236
Conversation
kpatch-build/create-diff-object.c
Outdated
| @@ -214,6 +216,22 @@ static struct rela *toc_rela(const struct rela *rela) | |||
| (unsigned int)rela->addend); | |||
| } | |||
|
|
|||
| #ifdef __aarch64__ | |||
| static int kpatch_is_mapping_symbol(struct symbol *sym) | |||
There was a problem hiding this comment.
This could use a comment to describe what a mapping symbol is and why we wouldn't correlate it.
There was a problem hiding this comment.
Also, probably best to make it return a bool so the return code semantics are clear.
kpatch-build/create-diff-object.c
Outdated
| rela->type == R_X86_64_32S && | ||
| #elif defined(__aarch64__) | ||
| rela->type == R_AARCH64_ABS64 && | ||
| #endif |
There was a problem hiding this comment.
No need to '#ifdef' this, the defines exist in /usr/include/elf.h regardless of arch.
There was a problem hiding this comment.
I'm not sure I understand this comment. The ifdef isn't trying to get around it being defined or not, it's trying to change the if condition.
There was a problem hiding this comment.
kpatch-build/create-diff-object.c
Outdated
| log_normal("function %s has no fentry/mcount call, unable to patch\n", | ||
| sym->name); | ||
| #endif |
There was a problem hiding this comment.
Another unneeded '#ifdef', instead the existing message can be made arch-independent, e.g. "function %s doesn't have patchable function entry".
kpatch-build/create-diff-object.c
Outdated
| rela->sym->sec) { | ||
| found = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Why is this needed? The check to ensure the symbol is patchable should have already been done already in kpatch_check_func_profiling_calls(). So I could be missing something, but I don't see the need to preserve patchable_sec or to do this check here.
I do notice that rela->addend is needed below for the insn_offset calculation, but if the offset isn't always at the same location, can the function entry point be discovered by just looking for it at the beginning of the function? If so, I'd much rather do that because it seems less complex than the hacks needed to preserve the contents of the table.
There was a problem hiding this comment.
Excellent point re kpatch_check_func_profiling_calls().
The offset isn't always the same because there can be an additional branch target identification instruction added to function entry by gcc which precedes the patchable function entry. However this can be identified and accounted for.
kpatch-build/create-diff-object.c
Outdated
| @@ -3421,7 +3486,11 @@ static void kpatch_create_mcount_sections(struct kpatch_elf *kelf) | |||
| nr++; | |||
|
|
|||
| /* create text/rela section pair */ | |||
| #ifdef __aarch64__ | |||
| sec = create_section_pair(kelf, "__patchable_function_entries", sizeof(void*), nr); | |||
There was a problem hiding this comment.
When trying to kpatch-build a change to meminfo_proc_show(), I got the following error:
make[1]: Entering directory '/root/linux'
LDS /root/.kpatch/tmp/patch/kpatch.lds
CC [M] /root/.kpatch/tmp/patch/patch-hook.o
LD [M] /root/.kpatch/tmp/patch/livepatch-a.o
ld: __patchable_function_entries has both ordered [`__patchable_function_entries' in /root/.kpatch/tmp/patch/patch-hook.o] and unordered [`__patchable_function_entries' in /root/.kpatch/tmp/patch/output.o] sections
ld: final link failed: bad value
make[2]: *** [scripts/Makefile.build:431: /root/.kpatch/tmp/patch/livepatch-a.o] Error 1
The problem is that the compiler-created version of the section has the SHF_LINK_ORDER flag set. I "fixed" it with the following hack. sh_link just points to a random section but maybe that's ok, AFAICT its value doesn't matter for the kernel's use of __patchable_function_entries.
diff --git a/kpatch-build/create-diff-object.c b/kpatch-build/create-diff-object.c
index 11a5f94..bd34764 100644
--- a/kpatch-build/create-diff-object.c
+++ b/kpatch-build/create-diff-object.c
@@ -3488,6 +3488,8 @@ static void kpatch_create_mcount_sections(struct kpatch_elf *kelf,
/* create text/rela section pair */
#ifdef __aarch64__
sec = create_section_pair(kelf, "__patchable_function_entries", sizeof(void*), nr);
+ sec->sh.sh_flags |= SHF_LINK_ORDER;
+ sec->sh.sh_link = 1;
#else /* !__aarch64__ */
sec = create_section_pair(kelf, "__mcount_loc", sizeof(void*), nr);
#endif
There was a problem hiding this comment.
This won't affect how the kernel uses this section and so will be fine. I didn't see the same error so I assume that's down to the linker being used.
There was a problem hiding this comment.
Ok so I have the opposite problem in that patch-hook.o is unordered and output.o becomes ordered with the change you suggested and the linking fails.
Which version of gcc are you using?
There was a problem hiding this comment.
I've seen this on both rhel9 and fedora34 (gcc version 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC)). We might want to copy sh_flags/sh_link from __patchable_function_entries of patched objectfile.
There was a problem hiding this comment.
Yeah that sounds like the best option
kpatch-build/create-diff-object.c
Outdated
| } | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
This feels like a major hack and I'm not really convinced it's needed. I have a more detailed comment below.
| patchable_sec = patchable_sec->rela; | ||
| list_del(&patchable_sec->list); | ||
| } | ||
|
|
There was a problem hiding this comment.
As described above, I don't think this is needed.
kpatch-build/kpatch-elf.c
Outdated
| */ | ||
| if (!sec || !sec->rela) | ||
| return; | ||
| patchable_sec = sec->rela; |
There was a problem hiding this comment.
The patchable_sec variable is confusingly named, since it's really the relocation section for the patchable sec. I don't think you need a separate variable for it anyway, it can just continue to be referred to by sec->rela.
There was a problem hiding this comment.
This was an optimisation to avoid searching for the "__patchable_function_entries" section each time. Although I'm sure it makes almost zero difference to the run time of the script.
There was a problem hiding this comment.
Oh, I see what you mean. Maybe do the find_section_by_name() before the loop, and give the rela sec a more descriptive name like patchable_rela_sec.
kpatch-build/kpatch-elf.c
Outdated
| patchable_sec = sec->rela; | ||
| } | ||
| list_for_each_entry(rela, &patchable_sec->relas, | ||
| list) { |
There was a problem hiding this comment.
No need for the line break here.
| [[ "$CONFIG_PRINTK_INDEX" -eq 0 ]] && AWK_OPTIONS="$AWK_OPTIONS -vskip_i=1" | ||
|
|
||
| SPECIAL_VARS="$(readelf -wi "$VMLINUX" | | ||
| gawk --non-decimal-data $AWK_OPTIONS ' |
There was a problem hiding this comment.
This line breaks shellcheck, which wants everything to be doublequoted, but in this case doublequoting is skipped intentionally so we need to tell shellcheck about this:
| gawk --non-decimal-data $AWK_OPTIONS ' | |
| # shellcheck disable=SC2086 | |
| gawk --non-decimal-data $AWK_OPTIONS ' |
| @@ -1536,7 +1564,7 @@ static void kpatch_replace_sections_syms(struct kpatch_elf *kelf) | |||
| continue; | |||
| } | |||
|
|
|||
| #ifdef __powerpc64__ | |||
| #if defined(__powerpc64__) || defined(__aarch64__) | |||
There was a problem hiding this comment.
Just curious, why add_off is 0 in aarch64?
I see different handling in x86 below.
:)
The "has_function_profiling" support field in the symbol struct is used to show that a function symbol is able to be patched. This is necessary to check that functions which need to be patched are able to be. On arm64 this means the presence of 2 NOP instructions at function entry which are patched by ftrace to call the ftrace handling code. These 2 NOPs are inserted by the compiler and the location of them is recorded in a section called "__patchable_function_entries". Check whether a symbol has a corresponding entry in the "__patchable_function_entries" section and if so mark it as "has_func_profiling". Signed-off-by: Suraj Jitindar Singh <[email protected]> --- V1->V2: - Make error message standard across architectures when no patchable entry - Don't store __patchable_function_entries section in kpatch_find_func_profiling_calls(), instead find it each time
…d populate The function kpatch_create_mcount_sections() allocates the __mcount_loc section and then populates it with functions which have a patchable entry. The following patch will add aarch64 support to this function where the allocation will have to be done before the kelf_patched is torn down. Thus split this function so that the allocation can be performed earlier and the populating as before. No intended functional change. Signed-off-by: Suraj Jitindar Singh <[email protected]> --- V1->V2: - Add patch to series
…arch64 The __mcount_loc section contains the addresses of patchable ftrace sites which is used by the ftrace infrastructure in the kernel to create a list of tracable functions and to know where to patch to enable tracing of them. On aarch64 this section is called __patchable_function_entries and is generated by the compiler. Either of __mcount_loc or __patchable_function_entries is recognised by the kernel but for aarch64 use __patchable_function_entries as it is what is expected. Add aarch64 support to kpatch_alloc_mcount_sections(). The SHF_LINK_ORDER section flag must be copied to ensure that it matches to avoid the following: ld: __patchable_function_entries has both ordered [...] and unordered [...] sections Add aarch64 support to kpatch_populate_mcount_sections(). Check for the 2 required NOP instructions on function entry, which may be preceded by a BTI C instruction depending on whether the function is a leaf function. This determines the offset of the patch site. Signed-off-by: Suraj Jitindar Singh <[email protected]> --- V1->V2: - Don't preserve the __patchable_function_entries section from the patched elf as this is already verified by kpatch_check_func_profiling_calls() - Instead get the patch entry offset by checking for a preceding BTI C instr - Copy the section flags for __patchable_function_entries
Add the final support required for aarch64 and enable building on that arch. Signed-off-by: Suraj Jitindar Singh <[email protected]> --- V1->V2: - Add # shellcheck disable=SC2086 - Add comment to kpatch_is_mapping_symbol()
|
Updated with V2 addressing review comments |
|
Hi @surajjs95 , @keiya-nobuta : some of my recent merges to the master branch introduced a bunch of merge conflicts for this patchset, mainly those where I removed |
|
When building a small patch test1.tar.gz, I notice a modpost warning: And see that there are multiple sections in the livepatch.ko file with that name (the second one missing the flags modpost complains about): Q: can the modpost warning be ignored? gcc: gcc version 11.2.1 20220127 (Red Hat 11.2.1-9) (GCC) |
| */ | ||
| tmp = find_section_by_name(&kelf->sections, "__patchable_function_entries"); | ||
| sec->sh.sh_flags |= (tmp->sh.sh_flags & SHF_LINK_ORDER); | ||
| sec->sh.sh_flags = 1; |
There was a problem hiding this comment.
sec->sh.sh_link = 1 ?
There was a problem hiding this comment.
Good catch, this resolves the issue @joe-lawrence reported above. I also thought we decided to copy it over from patched objectfile instead of always setting it.
Hi @joe-lawrence, thanks your work. It has appeared in other patches, but it is mostly summarized for the above reasons. And warn-detect-FAIL.patch was successfully built, I think this is because line_macro_change_only for aarch64 is not implemented. |
Thanks for testing! By the way, what is your gcc / kernel version combo that you tested with? I was hoping to try the PR on a 5.14-based kernel (aka, rhel-9), but I only saw @surajjs95 's tree: amazon-arm-livepatch-v5.10, do you have a kernel branch is closer to up to date?
Great, thanks for those fixups, I'll incorporate those too.
Yep, that is still on the TODO list for the arm implementation. |
You can use mine https://github.com/sm00th/linux/tree/kpatch/aarch64 it is v13 with dropped STACK_VALIDATION dependency on top of v5.17-rc4 |
I'm using:
Thanks, I'll try it. |
|
Some more incremental work here: https://github.com/joe-lawrence/kpatch/tree/arm64-rebase
Finally, if I try this change: @@ -3567,7 +3567,7 @@ static void kpatch_alloc_mcount_sections(struct kpatch_elf *kelf, struct kpatch_
*/
tmp = find_section_by_name(&kelf->sections, "__patchable_function_entries");
sec->sh.sh_flags |= (tmp->sh.sh_flags & SHF_LINK_ORDER);
- sec->sh.sh_link = 1;
+ sec->sh.sh_link = tmp->sh.sh_link;
break;
}
case PPC64:The integration tests fail with "nm: gcc-static-local-var-2.OUTPUT.o: sh_link [2] in section `__patchable_function_entries' is incorrect". Did I implement the suggestion correctly? |
I checked by cross-compile on x86 using gcc-linaro-11.2.1, and I got same error. On the PATCHED.o, patched/mm/mmap.o output/mm/mmap.o In most cases |
|
Re: sh_link: I don't know what sh_link should be either. It seems that ordinary module builds end up with sh_link set to the first non-zero length .text section. Perhaps for now, we should just leave it set to 1? Re: integration tests: I built the integration-tests-kernel-5.14.0-17.el9 tests and almost all succeeded, save for macro-printk.patch, due to diff context shifts (from 5.14 to 5.17), and gcc-mangled-3.patch: Interestingly both percpu_ref_put_many() and memcg_slab_free_hook() are defined in header files as static inlines, the compiler gives them their own .text. sections, but they don't have the usual I modified the patch to only add nops to get_slabinfo() and the build succeeds, reporting expected Also, I remember reporting a similar problem trying to build for PR #1244. Maybe this is new gcc-11 behavior? |
|
Some more interesting behavior to report. Using a rhel-9 kernel and gcc-mangled-3.tar.gz: In this case, the entire __list_add function has moved from Yet their compiled code is exactly the same: I googled a bit and found gcc Bug 61958 - functions arbitrarily placed in .text.unlikely section, from @jpoimboe way back in 2014. The gcc-managled-3.patch is similarly adding a printk and functions are moving from .text to .text.likely, however I don't believe that __list_add() is called by get_slabinfo(), nor is __list_add() inlined into its calling functions. For an additional data point, with the rhel-9 kernel, I see the same section change for |
|
Sorry for late reply. I couldn't prepare RHELL9, so I tried it with fedora 35 aarch64. I didn't get the same result but I get similar result that static inline functions were changed which independent of the patched functions. It seems that arm64 gcc11 has optimization features that I don't understand well. I didn't get this result with defconfig based config, so I tried turning off some configs from fedora's config. When CONFIG_DEBUG_SECTION_MISMATCH=n was set, these static inline function did not change. CONFIG_DEBUG_SECTION_MISMATCH is an option to add -fno-inline-functions-called-once to KBUILD_CFLAGS. I'm not sure what this is used for. It looks like it is solved, but the non-patched functions seems to be changed :( |
|
I posted this question to GCC Bugzilla. |
Thanks for raising the question to the gcc folks. Maybe resurrecting #1045 to at least dump out some of the IPA inlining results could help us cope. (Ideally we could feed such data back to the compiler to essentially repeat the same decisions, but I would assume that is simply not possible.) |
|
I think the Arm64 specific gcc issue needs to be resolved separately, but I don't think it's this pull-request issue. |
|
I've rebased your arm64 support into v0.9.6 and addressed a few issues which clang-built linux-5.15 throws up, and it builds working patches for most test cases. I'll clean up and post my tree shortly, after merging with the recent s/390 changes, which cause much incidental merge conflict, as they both add to the switch(kelf->arch) cases, and special_sections[] entries I think I've solved the problem with recent clang, where a separate __patchable_function_entries section is created for each patchable function, and it's required that create-diff-object can correctly read this different arrangement, and write_elf with the same many-sections format. One remaining issue I haven't yet solved with 5.15 is insmod failure logging "overflow in relocation type 261 val 4" where clang (or kpatch) is using type 261 (R_AARCH64_PREL32) in a way that's now deprecated due to increased security |
|
keiya-nobuta says,
The most difficult part is finding time during the day to work through it :) But yes, now with the initial s390x code out of the way, let's try continuing with arm64. :) swine says,
Perfect. Apologies for the code churn. We refactored a lot of the architecture-specific code from #ifdef to switch statements. Hopefully it's a little more obvious to fit in new arches and then run cross-arch unit testing. Rebasing would be a good restart for reviewing... I don't know if you can directly push that here, or if @surajjs95 would have to manually import those changes to his branch that the PR was open for.
Was the many __patchable_function_entries sections issue limited to clang or did it affect gcc, too?
Can you elaborate on that? Also, is this no longer an issue on newer kernels? Thanks. |
|
Hello, is anyone currently working on this? To see the current status, I also rebased the code to current master : this branch
I didn't test clang build which @swine mentioned. On the other hand, I believe kernel side needs to/will be updated. I'm not familiar with kernel side at the moment and try to understand the current situation as well. Thanks. |
|
thanks @t-msn, I wish I'd pushed my arm64 branch earlier to save you duplicated effort, but was hunting clang issues. your rebase is possibly cleaner (certainly smaller, as it didn't need to address the clang-vs-gcc issues that I've explored in #1302). BTW, the "overflow in relocation" issue I mentioned earlier is a non-issue, typo in earlier version of the code. |
|
@swine Thank you very much for sharing your reabase code. It looks like the clang support needs some effort... Maybe is it better to support(merge) gcc only part for the starting point? Anyway, I will try to look the code.
That's good. Thanks. |
|
This PR has been open for 60 days with no activity and no assignee. It will be closed in 7 days unless a comment is added. |
|
This PR was closed because it was inactive for 7 days after being marked stale. |
Implement support for building livepatches for arm64
Kernel Support:
With the above kernel tree and the changes in this pull request I have been able to build and apply patches using kpatch.