create-diff-object: __patchable_function_entries section support #1415

joe-lawrence · 2024-10-09T19:02:23Z

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 some kernel configurations, 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 these configurations, use __patchable_function_entries as it is what is expected.

The x86_64 arch is special (of course). Unlike other arches (ppc64le and aarch64) a x86_64 kernel built with -fpatchable-function-entry will generate nops AND create rela__patchable_function_entries for functions even marked as notrace. For this arch, always create __mount_loc sections and rely on fentry relocations to indicate ftrace call sites.

Note: this patch is a non-arch translation of original code by Pete Swain swine@google.com for aarch64. At the same time, this version squashes several follow up commits from him and zimao zimao@microsoft.com. The intent is minimize the eventual changeset for aarch64 support now that other arches are making use of __patchable_function_entries sections.

joe-lawrence · 2024-11-04T20:49:05Z

Tested internally (beaker job #10108322) on s390x and ppc64le running early rhel-10 kernel. Also tested by Amazon on top of the rest of the aarch64 arch support code.

kpatch-build/create-diff-object.c

jpoimboe · 2024-11-14T17:49:11Z

kpatch-build/create-diff-object.c

+	 * sections for each symbols.
+	 */
+	kelf->has_pfe = true;
+	sec = find_section_by_name(&kelf->sections, "__patchable_function_entries");


This has no purpose apparently?

Also since this function is very similar to kpatch_create_mcount_sections() it would be a good idea to mirror that logic as much as possible so it's easier to review.

Oops, looks like sec and relasec assignments are just overwritten here.

With respect to kpatch_create_mcount_sections() - yes, ideally there will be a single function, but I found it much easier to debug and review a standalone copy, at least to start with. Once the pfe functionality is set (and the commonality determined), I can merge the two functions back together.

They don't necessarily need to be combined into a single function, if it's easier to keep them apart. I was just looking for more symmetry between the two functions so it's easier to see they're correct.

The latest version combines them into a single kpatch_create_ftrace_callsite_sections() with only two if (pfe) conditional blocks.

kpatch-build/create-diff-object.c

jpoimboe · 2024-11-14T17:52:51Z

kpatch-build/create-diff-object.c

+		}
+
+		/* Allocate __patchable_function_entries for symbol */
+		sec = create_section_pair(kelf, "__patchable_function_entries", sizeof(void *), 1);


Not sure why "1" instead of "nr"? Also this should be up higher (so it mirrors the mcount version of the function).

Unlike the combined mcount section, we're creating a pfe section per function, so there's a new 1 entry pair created every time through the loop.

Ah... is that really necessary though? can they just be placed in a single section?

I'm not sure, but I figured I would try to mimic a kernel build object as closely as possible to avoid any toolchain surprises.

kpatch-build/create-diff-object.c

kpatch-build/kpatch-elf.c

joe-lawrence · 2024-11-19T15:16:00Z

v2:

add a comment above kpatch_set_pfe_link() explaining why the function iterates over multiple pft sections
remove unneeded x86_64 code from kpatch_create_pfe_sections()
kpatch_create_ftrace_callsite_sections() and main() rename create_pfe to has_pfe -- just a slight change in meaning since kpatch_create_ftrace_callsite_sections() decides what to do about x86_64 objects that "have pfe" sections... the decision to "create pfe" sections lives within the function.
- assuming v2 looks to be mostly solid, I can still double back and see about folding kpatch_create_pfe_sections() back into kpatch_create_mcount_sections(). The code is about 90% the same, I'd just like to avoid having half a dozen "if pfe" blocks for the two sections.
remove unneeded find_nth_section_by_name() and revert to original find_section_by_name() implementation

kpatch-build/kpatch-elf.h

kpatch-build/create-diff-object.c

jpoimboe · 2024-11-20T19:11:52Z

kpatch-build/create-diff-object.c

+	 * We will create separate __patchable_function_entries
+	 * sections for each symbols.
+	 */
+	kelf->has_pfe = true;


I still think it's simpler (and easier to converge with the mcount stuff) to just make a single section. It should be harmless.

kpatch-build/create-diff-object.c

jpoimboe · 2024-11-20T19:40:08Z

kpatch-build/create-diff-object.c

+
+		/* __patchable_function_entries relocates off the section symbol */
+		section_sym = find_symbol_by_name(&kelf->symbols, sym->sec->name);
+		pfe_rela->sym = section_sym;


I'm pretty sure there's no need to use the section symbol, either way it points to the same thing in the code. Regardless the find_symbol_by_name() isn't needed, the section symbol is already in sym->sec.

This is another instance of matching the input kernel objects (as reported by readelf --relocs).

jpoimboe · 2024-11-20T19:40:29Z

kpatch-build/create-diff-object.c

+		 * Create a .rela__patchable_function_entries entry which also points to it.
+		 */
+		ALLOC_LINK(pfe_rela, &relasec->relas);
+		struct symbol *section_sym;


Variables should be defined at the beginning of the block

Good catch.

jpoimboe · 2024-11-20T19:55:28Z

kpatch-build/kpatch-elf.c

+			if (sym->sec->pfe) {
+				sym->sec->pfe->sh.sh_link = sym->sec->index;
+				if (sym->sec->pfe->rela)
+					sym->sec->pfe->rela->sh.sh_info = sym->sec->index;


This last part seems wrong, .rela__patchable_function_entries sh_info should point to its __patchable_function_entries section.

And anyway this gets overwritten by the code in main() which does that, right after calling this function.

Since it's overwritten, I'll just remove it.

kpatch-build/create-diff-object.c

kpatch-build/kpatch-elf.c

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 some kernel configurations, 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 these configurations, use __patchable_function_entries as it is what is expected. The x86_64 arch is special (of course). Unlike other arches (ppc64le and aarch64) a x86_64 kernel built with -fpatchable-function-entry will generate nops AND create rela__patchable_function_entries for functions even marked as notrace. For this arch, always create __mount_loc sections and rely on __fentry__ relocations to indicate ftrace call sites. Note: this patch is a refactoring of original code by Pete Swain <swine@google.com> for aarch64. At the same time, this version squashes several follow up commits from him and zimao <zimao@microsoft.com>. The intent is minimize the eventual changeset for aarch64 support now that other arches are making use of __patchable_function_entries sections. Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>

joe-lawrence

(I think my latest force pushes may have screwed up my github replies to your earlier comments. I'll try to find those and summarize in another comment that summarizes the latest updates. Sorry about that.)

kpatch-build/create-diff-object.c

joe-lawrence · 2024-11-22T20:04:32Z

kpatch-build/create-diff-object.c

+		sym->sec->pfe = sec;
+
+		/*
+		 * 'rela' points to the patchable function entry


OK, shall I update this in a follow up commit that separates pfe code from cleanup?

joe-lawrence · 2024-11-22T20:04:41Z

kpatch-build/create-diff-object.c

+		 * Create a .rela__patchable_function_entries entry which also points to it.
+		 */
+		ALLOC_LINK(pfe_rela, &relasec->relas);
+		struct symbol *section_sym;


Good catch.

kpatch-build/create-diff-object.c

joe-lawrence · 2024-11-22T20:09:54Z

kpatch-build/kpatch-elf.c

+			if (sym->sec->pfe) {
+				sym->sec->pfe->sh.sh_link = sym->sec->index;
+				if (sym->sec->pfe->rela)
+					sym->sec->pfe->rela->sh.sh_info = sym->sec->index;


Since it's overwritten, I'll just remove it.

kpatch-build/kpatch-elf.c

kpatch-build/kpatch-elf.h

joe-lawrence · 2024-11-26T20:31:36Z

kpatch-build/create-diff-object.c

+				list_for_each_entry(rela, &sym->sec->rela->relas, list) {
+					if (!strcmp(rela->sym->name, "_mcount")) {
+						sym->has_func_profiling = 1;
+						break;


Looking at a ppc64le object file, I don't think it's the first rela:

Relocation section '.rela.text.meminfo_proc_open' at offset 0x1698 contains 6 entries: Offset Info Type Symbol's Value Symbol's Name + Addend 0000000000000000 000000220000002c R_PPC64_REL64 0000000000000000 .TOC. - 8 0000000000000008 0000000000000076 R_PPC64_ENTRY 0 0000000000000014 000000330000000a R_PPC64_REL24 0000000000000000 _mcount + 0 0000000000000028 0000000800000032 R_PPC64_TOC16_HA 0000000000000000 .toc + 10 0000000000000030 0000000800000040 R_PPC64_TOC16_LO_DS 0000000000000000 .toc + 10 0000000000000034 000000320000000a R_PPC64_REL24 0000000000000000 single_open + 0

Though maybe some other assumption could be made (considering global/local entry and TOC relas)?

joe-lawrence · 2024-11-26T20:57:56Z

v3:

Implement all of the code cleanup suggestions
Re: "I don't think it matters whether the original object had pfe"- it's used when determining if the original function had an ftrace site (ie, what to look for, an mcount relocation or pfe nop site)
Re: "Ah... is that [a new pfe section per function] really necessary though? can they just be placed in a single section?" - technically I think they could (it seems to be separated for garbage collection purposes), it is just easier to debug when the create-diff-object output.o has similar layout to its kernel build created counterpart
Re: "I'm pretty sure there's no need to use the section symbol [in pfe sections], either way it points to the same thing in the code" - true, but again, it is easier to debug when the comparing readelf from the original kernel object to the create-diff-object generated output
Re: "Shouldn't it [ppc64le mcount relocation check in kpatch_find_func_profiling_calls()] be the first rela in the section?" - no, sample ppc64le object files have a TOC and entry relocation before their mcount relocations, unless I am misreading the question

Sorry again for making the review a hash. GitHub hides/buries earlier comments when a subsequent push removes the associated code.

jpoimboe · 2024-11-27T05:47:45Z

Re: "Ah... is that [a new pfe section per function] really necessary though? can they just be placed in a single section?" - technically I think they could (it seems to be separated for garbage collection purposes), it is just easier to debug when the create-diff-object output.o has similar layout to its kernel build created counterpart

Re: "I'm pretty sure there's no need to use the section symbol [in pfe sections], either way it points to the same thing in the code" - true, but again, it is easier to debug when the comparing readelf from the original kernel object to the create-diff-object generated output

I'd disagree with both points, pfe/mcount_loc are quite simple and should be bug-free at this point, especially with mostly shared code. More robust/readable code is more important than hypothetical (and rare for this code IMO) debugging. I don't see how that would help much with debugging anyway, as the layouts of these sections are dirt simple. But I defer to you.

Sorry again for making the review a hash. GitHub hides/buries earlier comments when a subsequent push removes the associated code.

No worries, it's one of github's quirks and the comments can be shown by unhiding the "resolved" comments. The above summary is actually pretty helpful, but not necessary.

BTW I think you forgot to move 'pfe' to struct symbol?

joe-lawrence · 2024-12-02T16:16:16Z

BTW I think you forgot to move 'pfe' to struct symbol?
Oops, I meant to double back and do that last. I assume you mean to associate it with the text section symbol?

joe-lawrence mentioned this pull request Oct 9, 2024

Rebase of aarch64 PR #1414

Open

joe-lawrence force-pushed the patchable_function_entries branch 2 times, most recently from 422b699 to a527949 Compare October 9, 2024 20:02

joe-lawrence mentioned this pull request Oct 10, 2024

C99 code vs. gcc defaults? #1416

Closed

joe-lawrence force-pushed the patchable_function_entries branch from a527949 to 14c03c4 Compare November 4, 2024 20:32

joe-lawrence changed the title ~~Draft: create-diff-object: __patchable_function_entries section support~~ create-diff-object: __patchable_function_entries section support Nov 4, 2024

joe-lawrence mentioned this pull request Nov 8, 2024

Add LoongArch support for kpatch #1424

Open

joe-lawrence commented Nov 12, 2024

View reviewed changes

kpatch-build/create-diff-object.c Outdated Show resolved Hide resolved

joe-lawrence force-pushed the patchable_function_entries branch from 14c03c4 to 6dc0961 Compare November 13, 2024 14:22

jpoimboe reviewed Nov 14, 2024

View reviewed changes

joe-lawrence force-pushed the patchable_function_entries branch from 6dc0961 to c1472d5 Compare November 19, 2024 15:04

jpoimboe reviewed Nov 20, 2024

View reviewed changes

joe-lawrence force-pushed the patchable_function_entries branch from 948f93e to d8b1708 Compare November 26, 2024 20:25

joe-lawrence commented Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create-diff-object: __patchable_function_entries section support #1415

create-diff-object: __patchable_function_entries section support #1415

joe-lawrence commented Oct 9, 2024 •

edited

Loading

joe-lawrence commented Nov 4, 2024

jpoimboe Nov 14, 2024

joe-lawrence Nov 15, 2024

jpoimboe Nov 15, 2024

joe-lawrence Nov 26, 2024

jpoimboe Nov 14, 2024

joe-lawrence Nov 15, 2024

jpoimboe Nov 15, 2024

joe-lawrence Nov 18, 2024

joe-lawrence commented Nov 19, 2024

jpoimboe Nov 20, 2024

jpoimboe Nov 20, 2024

joe-lawrence Nov 22, 2024

jpoimboe Nov 20, 2024

joe-lawrence Nov 22, 2024

jpoimboe Nov 20, 2024

joe-lawrence Nov 22, 2024

joe-lawrence left a comment

joe-lawrence Nov 22, 2024

joe-lawrence Nov 22, 2024

joe-lawrence Nov 22, 2024

joe-lawrence Nov 26, 2024

joe-lawrence commented Nov 26, 2024

jpoimboe commented Nov 27, 2024 •

edited

Loading

joe-lawrence commented Dec 2, 2024

create-diff-object: __patchable_function_entries section support #1415

Are you sure you want to change the base?

create-diff-object: __patchable_function_entries section support #1415

Conversation

joe-lawrence commented Oct 9, 2024 • edited Loading

joe-lawrence commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joe-lawrence commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joe-lawrence left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joe-lawrence commented Nov 26, 2024

jpoimboe commented Nov 27, 2024 • edited Loading

joe-lawrence commented Dec 2, 2024

joe-lawrence commented Oct 9, 2024 •

edited

Loading

jpoimboe commented Nov 27, 2024 •

edited

Loading