* -fpatchable-function-entry should set SHF_WRITE and create one __patchable_function_entries per function @ 2020-01-07 6:06 Fangrui Song 2020-01-07 7:26 ` Fangrui Song 0 siblings, 1 reply; 6+ messages in thread From: Fangrui Song @ 2020-01-07 6:06 UTC (permalink / raw) To: gcc Cc: Martin Liška, Alexander Monakov, Torsten Duwe, Maxim Kuvyrkov, Nick Clifton The addresses of NOPs are collected in a section named __patchable_function_entries. A __patchable_function_entries entry is relocated by a symbolic relocation (e.g. R_X86_64_64, R_AARCH64_ABS64, R_PPC64_ADDR64). In -shared or -pie mode, the linker will create a dynamic relocation (non-preemptible: relative relocation (e.g. R_X86_64_RELATIVE); preemptible: symbolic relocation (e.g. R_X86_64_64)). In either case, the section contents will be modified at runtime. Thus, the section should have the SHF_WRITE flag to avoid text relocations (DF_TEXTREL). When -ffunction-sections is used, ideally GCC should emit one __patchable_function_entries (SHF_LINK_ORDER) per .text.foo . If the corresponding .text.foo is discarded (--gc-sections, COMDAT, /DISCARD/), the linker can discard the associated __patchable_function_entries. This can be seen as a lightweight COMDAT section group. (A section group adds an extra section and costs 3 words) Currently lld (LLVM linker) has implemented such SHF_LINK_ORDER collecting features. GNU ld and gold don't have the features. I have summarized the feature requests in this post https://sourceware.org/ml/binutils/2019-11/msg00266.html gcc -fpatchable-function-entry=2 -ffunction-sections -c a.c [ 4] .text.f0 PROGBITS 0000000000000000 000040 000009 00 AX 0 0 1 ### No W flag ### One __patchable_function_entries instead of 3. [ 5] __patchable_function_entries PROGBITS 0000000000000000 000049 000018 00 A 0 0 1 [ 6] .rela__patchable_function_entries RELA 0000000000000000 000280 000048 18 I 13 5 8 [ 7] .text.f1 PROGBITS 0000000000000000 000061 000009 00 AX 0 0 1 [ 8] .text.f2 PROGBITS 0000000000000000 00006a 000009 00 AX 0 0 1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: -fpatchable-function-entry should set SHF_WRITE and create one __patchable_function_entries per function 2020-01-07 6:06 -fpatchable-function-entry should set SHF_WRITE and create one __patchable_function_entries per function Fangrui Song @ 2020-01-07 7:26 ` Fangrui Song 2020-01-07 10:32 ` Szabolcs Nagy 0 siblings, 1 reply; 6+ messages in thread From: Fangrui Song @ 2020-01-07 7:26 UTC (permalink / raw) To: gcc Cc: Martin Liška, Alexander Monakov, Torsten Duwe, Maxim Kuvyrkov, Nick Clifton On 2020-01-06, Fangrui Song wrote: >The addresses of NOPs are collected in a section named __patchable_function_entries. >A __patchable_function_entries entry is relocated by a symbolic relocation (e.g. R_X86_64_64, R_AARCH64_ABS64, R_PPC64_ADDR64). >In -shared or -pie mode, the linker will create a dynamic relocation (non-preemptible: relative relocation (e.g. R_X86_64_RELATIVE); preemptible: symbolic relocation (e.g. R_X86_64_64)). > >In either case, the section contents will be modified at runtime. >Thus, the section should have the SHF_WRITE flag to avoid text relocations (DF_TEXTREL). > > > >When -ffunction-sections is used, ideally GCC should emit one __patchable_function_entries (SHF_LINK_ORDER) per .text.foo . >If the corresponding .text.foo is discarded (--gc-sections, COMDAT, /DISCARD/), the linker can discard the associated __patchable_function_entries. This can be seen as a lightweight COMDAT section group. (A section group adds an extra section and costs 3 words) >Currently lld (LLVM linker) has implemented such SHF_LINK_ORDER collecting features. GNU ld and gold don't have the features. > >I have summarized the feature requests in this post https://sourceware.org/ml/binutils/2019-11/msg00266.html > >gcc -fpatchable-function-entry=2 -ffunction-sections -c a.c > > [ 4] .text.f0 PROGBITS 0000000000000000 000040 000009 00 AX 0 0 1 > ### No W flag > ### One __patchable_function_entries instead of 3. > [ 5] __patchable_function_entries PROGBITS 0000000000000000 000049 000018 00 A 0 0 1 > [ 6] .rela__patchable_function_entries RELA 0000000000000000 000280 000048 18 I 13 5 8 > [ 7] .text.f1 PROGBITS 0000000000000000 000061 000009 00 AX 0 0 1 > [ 8] .text.f2 PROGBITS 0000000000000000 00006a 000009 00 AX 0 0 1 Emitting a __patchable_function_entries for each function may waste object file sizes (64 bytes per function on ELF64). If zeros are allowed, emitting a single __patchable_function_entries should be fine. If we do want to emit unique sections, the condition should be either -ffunction-sections or COMDAT is used. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: -fpatchable-function-entry should set SHF_WRITE and create one __patchable_function_entries per function 2020-01-07 7:26 ` Fangrui Song @ 2020-01-07 10:32 ` Szabolcs Nagy 2020-01-08 9:16 ` __patchable_function_entries is flawed Fangrui Song 0 siblings, 1 reply; 6+ messages in thread From: Szabolcs Nagy @ 2020-01-07 10:32 UTC (permalink / raw) To: Fangrui Song, gcc Cc: nd, Martin Liška, Alexander Monakov, Torsten Duwe, Maxim Kuvyrkov, nickc On 07/01/2020 07:25, Fangrui Song wrote: > On 2020-01-06, Fangrui Song wrote: >> The addresses of NOPs are collected in a section named __patchable_function_entries. >> A __patchable_function_entries entry is relocated by a symbolic relocation (e.g. R_X86_64_64, R_AARCH64_ABS64, R_PPC64_ADDR64). >> In -shared or -pie mode, the linker will create a dynamic relocation (non-preemptible: relative relocation (e.g. R_X86_64_RELATIVE); >> preemptible: symbolic relocation (e.g. R_X86_64_64)). >> >> In either case, the section contents will be modified at runtime. >> Thus, the section should have the SHF_WRITE flag to avoid text relocations (DF_TEXTREL). pie/pic should either imply writable __patchable_function_entries, or __patchable_function_entries should be documented to be offsets from some base address in the module: the users of it have to modify .text and do lowlevel hacks so they should be able to handle such arithmetics. i think it's worth opening a gcc bug report. >> When -ffunction-sections is used, ideally GCC should emit one __patchable_function_entries (SHF_LINK_ORDER) per .text.foo . >> If the corresponding .text.foo is discarded (--gc-sections, COMDAT, /DISCARD/), the linker can discard the associated >> __patchable_function_entries. This can be seen as a lightweight COMDAT section group. (A section group adds an extra section and costs 3 words) >> Currently lld (LLVM linker) has implemented such SHF_LINK_ORDER collecting features. GNU ld and gold don't have the features. >> >> I have summarized the feature requests in this post https://sourceware.org/ml/binutils/2019-11/msg00266.html >> >> gcc -fpatchable-function-entry=2 -ffunction-sections -c a.c >> >> [ 4] .text.f0 PROGBITS 0000000000000000 000040 000009 00 AX 0 0 1 >> ### No W flag >> ### One __patchable_function_entries instead of 3. >> [ 5] __patchable_function_entries PROGBITS 0000000000000000 000049 000018 00 A 0 0 1 >> [ 6] .rela__patchable_function_entries RELA 0000000000000000 000280 000048 18 I 13 5 8 >> [ 7] .text.f1 PROGBITS 0000000000000000 000061 000009 00 AX 0 0 1 >> [ 8] .text.f2 PROGBITS 0000000000000000 00006a 000009 00 AX 0 0 1 > > Emitting a __patchable_function_entries for each function may waste > object file sizes (64 bytes per function on ELF64). If zeros are > allowed, emitting a single __patchable_function_entries should be fine. > > If we do want to emit unique sections, the condition should be either > -ffunction-sections or COMDAT is used. again it's worth raising a gcc bug i think. there is another known issue: aarch64 -mbranch-protect=bti (and presumably x86_64 -fcf-protection=branch) has to add landing pad at the begining of each indirectly called function so the patchable nops can only come after that. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92424 no matter how this gets resolved i think this will require documentation changes too. ^ permalink raw reply [flat|nested] 6+ messages in thread
* __patchable_function_entries is flawed 2020-01-07 10:32 ` Szabolcs Nagy @ 2020-01-08 9:16 ` Fangrui Song 2020-01-10 3:53 ` Fangrui Song 0 siblings, 1 reply; 6+ messages in thread From: Fangrui Song @ 2020-01-08 9:16 UTC (permalink / raw) To: gcc Cc: nd, Szabolcs Nagy, Martin Liška, Alexander Monakov, Torsten Duwe, Maxim Kuvyrkov, nickc On 2020-01-07, Szabolcs Nagy wrote: >On 07/01/2020 07:25, Fangrui Song wrote: >> On 2020-01-06, Fangrui Song wrote: >>> The addresses of NOPs are collected in a section named __patchable_function_entries. >>> A __patchable_function_entries entry is relocated by a symbolic relocation (e.g. R_X86_64_64, R_AARCH64_ABS64, R_PPC64_ADDR64). >>> In -shared or -pie mode, the linker will create a dynamic relocation (non-preemptible: relative relocation (e.g. R_X86_64_RELATIVE); >>> preemptible: symbolic relocation (e.g. R_X86_64_64)). >>> >>> In either case, the section contents will be modified at runtime. >>> Thus, the section should have the SHF_WRITE flag to avoid text relocations (DF_TEXTREL). > >pie/pic should either imply writable __patchable_function_entries, >or __patchable_function_entries should be documented to be offsets >from some base address in the module: the users of it have to modify >.text and do lowlevel hacks so they should be able to handle such >arithmetics. > >i think it's worth opening a gcc bug report. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93194 patch posted to gcc-patches. PC-relative relocation types (R_X86_64_64, R_AARCH64_PREL64, R_PPC64_REL64) can avoid dynamic relocations, and avoid the need for SHF_WRITE. Unfortunately, it seems we cannot re-interpret __patchable_function_entries. I have found 2 other problems with the current __patchable_function_entries. Perhaps we need a new design. * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93197 __patchable_function_entries will always be collected by --gc-sections (.tmp_vmlinux1 is not linked with --gc-sections, so it appears that the Linux kernel is not affected.) I think the solution requires a GNU ld and gold side fix: https://sourceware.org/bugzilla/show_bug.cgi?id=24526 If you clang>=8, you can play with clang -fstack-size-section -c a.cc Basically I think __patchable_function_entry has to behave like .stack_sizes . * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93195 __patchable_function_entries should consider comdat groups. It can cause linker failures (GNU ld, gold and lld). It is fairly easy to trigger with C++ inline. (OK, the Linux kernel does not speak C++.) (Clang's function multi-versioning implementation uses comdat, even in C. I don't have an example with GCC. My GCC __attribute__((target(..))) is broken.) The two issues make -fpatchable-function-entry useless for user applications. I still hope we can make the solution more general, not restricted to the Linux kernel. We've already implemented enough GCC options for dynamic ftrace (with regs) and live patching: -mfentry -mnop-mcount -mrecord-mcount -mhotpatch -fpatchable-function-entry. FWIW I am working on a clang/llvm patch https://reviews.llvm.org/D72215 I have tried resolving these problems. ("o" (SHF_LINK_ORDER+sh_link) is not recognized by GNU as.) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: __patchable_function_entries is flawed 2020-01-08 9:16 ` __patchable_function_entries is flawed Fangrui Song @ 2020-01-10 3:53 ` Fangrui Song 2020-01-10 6:28 ` Fangrui Song 0 siblings, 1 reply; 6+ messages in thread From: Fangrui Song @ 2020-01-10 3:53 UTC (permalink / raw) To: gcc Cc: nd, Szabolcs Nagy, Martin Liška, Alexander Monakov, Torsten Duwe, Maxim Kuvyrkov, nickc On 2020-01-08, Fangrui Song wrote: >On 2020-01-07, Szabolcs Nagy wrote: >>On 07/01/2020 07:25, Fangrui Song wrote: >>>On 2020-01-06, Fangrui Song wrote: >>>>The addresses of NOPs are collected in a section named __patchable_function_entries. >>>>A __patchable_function_entries entry is relocated by a symbolic relocation (e.g. R_X86_64_64, R_AARCH64_ABS64, R_PPC64_ADDR64). >>>>In -shared or -pie mode, the linker will create a dynamic relocation (non-preemptible: relative relocation (e.g. R_X86_64_RELATIVE); >>>>preemptible: symbolic relocation (e.g. R_X86_64_64)). >>>> >>>>In either case, the section contents will be modified at runtime. >>>>Thus, the section should have the SHF_WRITE flag to avoid text relocations (DF_TEXTREL). >> >>pie/pic should either imply writable __patchable_function_entries, >>or __patchable_function_entries should be documented to be offsets >>from some base address in the module: the users of it have to modify >>.text and do lowlevel hacks so they should be able to handle such >>arithmetics. >> >>i think it's worth opening a gcc bug report. > >https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93194 patch posted to gcc-patches. > >PC-relative relocation types (R_X86_64_64, R_AARCH64_PREL64, >R_PPC64_REL64) can avoid dynamic relocations, and avoid the need for >SHF_WRITE. Unfortunately, it seems we cannot re-interpret >__patchable_function_entries. I have found 2 other problems with the >current __patchable_function_entries. Perhaps we need a new design. > > >* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93197 > __patchable_function_entries will always be collected by --gc-sections > (.tmp_vmlinux1 is not linked with --gc-sections, so it appears that > the Linux kernel is not affected.) > I think the solution requires a GNU ld and gold side fix: > https://sourceware.org/bugzilla/show_bug.cgi?id=24526 > > If you clang>=8, you can play with clang -fstack-size-section -c a.cc > Basically I think __patchable_function_entry has to behave like .stack_sizes . > >* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93195 > __patchable_function_entries should consider comdat groups. > > It can cause linker failures (GNU ld, gold and lld). It is fairly > easy to trigger with C++ inline. > (OK, the Linux kernel does not speak C++.) > (Clang's function multi-versioning implementation uses comdat, even in > C. I don't have an example with GCC. My GCC __attribute__((target(..))) is broken.) > >The two issues make -fpatchable-function-entry useless for user >applications. I still hope we can make the solution more general, not >restricted to the Linux kernel. We've already implemented enough GCC >options for dynamic ftrace (with regs) and live patching: -mfentry >-mnop-mcount -mrecord-mcount -mhotpatch -fpatchable-function-entry. > >FWIW I am working on a clang/llvm patch https://reviews.llvm.org/D72215 >I have tried resolving these problems. >("o" (SHF_LINK_ORDER+sh_link) is not recognized by GNU as.) The LLVM 10.0 release branch is scheduled next week. I want to contribute my clang -fpatchable-function-entry patches before that (already approved), so that clang built Linux people can play with it with Clang 10.0 . Can we agree on the syntax of the new option before 2020-01-15? We can reuse the option name, just overload the value by adding the third argument, "pcrel": -fpatchable-function-entry=2,0,pcrel If "pcrel" is seen, GCC should emit the __patchable_function_entries section which contains PC-relative addresses, instead of absolute addresses. This is backward compatible. % stage1-gcc/xgcc -B stage1-gcc -fpatchable-function-entry=1,a -xc /dev/null cc1: error: invalid arguments for ‘-fpatchable_function_entry’ (OK, another bug: incorrect option name in the diagnostic.) I can teach Clang to only recognize the pcrel form. The function attribute does not need any change: __attribute__((patchable_function_entry(N,M))) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: __patchable_function_entries is flawed 2020-01-10 3:53 ` Fangrui Song @ 2020-01-10 6:28 ` Fangrui Song 0 siblings, 0 replies; 6+ messages in thread From: Fangrui Song @ 2020-01-10 6:28 UTC (permalink / raw) To: gcc Cc: nd, Szabolcs Nagy, Martin Liška, Alexander Monakov, Torsten Duwe, Maxim Kuvyrkov, nickc On 2020-01-09, Fangrui Song wrote: >On 2020-01-08, Fangrui Song wrote: >>On 2020-01-07, Szabolcs Nagy wrote: >>>On 07/01/2020 07:25, Fangrui Song wrote: >>>>On 2020-01-06, Fangrui Song wrote: >>>>>The addresses of NOPs are collected in a section named __patchable_function_entries. >>>>>A __patchable_function_entries entry is relocated by a symbolic relocation (e.g. R_X86_64_64, R_AARCH64_ABS64, R_PPC64_ADDR64). >>>>>In -shared or -pie mode, the linker will create a dynamic relocation (non-preemptible: relative relocation (e.g. R_X86_64_RELATIVE); >>>>>preemptible: symbolic relocation (e.g. R_X86_64_64)). >>>>> >>>>>In either case, the section contents will be modified at runtime. >>>>>Thus, the section should have the SHF_WRITE flag to avoid text relocations (DF_TEXTREL). >>> >>>pie/pic should either imply writable __patchable_function_entries, >>>or __patchable_function_entries should be documented to be offsets >>>from some base address in the module: the users of it have to modify >>>.text and do lowlevel hacks so they should be able to handle such >>>arithmetics. >>> >>>i think it's worth opening a gcc bug report. >> >>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93194 patch posted to gcc-patches. >> >>PC-relative relocation types (R_X86_64_64, R_AARCH64_PREL64, >>R_PPC64_REL64) can avoid dynamic relocations, and avoid the need for >>SHF_WRITE. Unfortunately, it seems we cannot re-interpret >>__patchable_function_entries. I have found 2 other problems with the >>current __patchable_function_entries. Perhaps we need a new design. >> >> >>* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93197 >> __patchable_function_entries will always be collected by --gc-sections >> (.tmp_vmlinux1 is not linked with --gc-sections, so it appears that >> the Linux kernel is not affected.) >> I think the solution requires a GNU ld and gold side fix: >> https://sourceware.org/bugzilla/show_bug.cgi?id=24526 >> >> If you clang>=8, you can play with clang -fstack-size-section -c a.cc >> Basically I think __patchable_function_entry has to behave like .stack_sizes . >> >>* https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93195 >> __patchable_function_entries should consider comdat groups. >> >> It can cause linker failures (GNU ld, gold and lld). It is fairly >> easy to trigger with C++ inline. >> (OK, the Linux kernel does not speak C++.) >> (Clang's function multi-versioning implementation uses comdat, even in >> C. I don't have an example with GCC. My GCC __attribute__((target(..))) is broken.) >> >>The two issues make -fpatchable-function-entry useless for user >>applications. I still hope we can make the solution more general, not >>restricted to the Linux kernel. We've already implemented enough GCC >>options for dynamic ftrace (with regs) and live patching: -mfentry >>-mnop-mcount -mrecord-mcount -mhotpatch -fpatchable-function-entry. >> >>FWIW I am working on a clang/llvm patch https://reviews.llvm.org/D72215 >>I have tried resolving these problems. >>("o" (SHF_LINK_ORDER+sh_link) is not recognized by GNU as.) > >The LLVM 10.0 release branch is scheduled next week. I want to >contribute my clang -fpatchable-function-entry patches before that >(already approved), so that clang built Linux people can play with it >with Clang 10.0 . > >Can we agree on the syntax of the new option before 2020-01-15? > >We can reuse the option name, just overload the value by adding the >third argument, "pcrel": > >-fpatchable-function-entry=2,0,pcrel > >If "pcrel" is seen, GCC should emit the __patchable_function_entries >section which contains PC-relative addresses, instead of absolute >addresses. This is backward compatible. > > % stage1-gcc/xgcc -B stage1-gcc -fpatchable-function-entry=1,a -xc /dev/null > cc1: error: invalid arguments for ‘-fpatchable_function_entry’ > >(OK, another bug: incorrect option name in the diagnostic.) > >I can teach Clang to only recognize the pcrel form. > > >The function attribute does not need any change: __attribute__((patchable_function_entry(N,M))) Some architectures do not have a PC-relative relocation type of pointer size, e.g. MIPS has R_MIPS_PC32 (GNU extension) but not R_MIPS_PC64, and .quad foo-. is not representable. Making -fpatchable-function-entry=2,0 absolute by default is fine (I will teach Clang to emit such __patchable_function_entries table). Just wanted to mention that on most other architectures PC-relative is better. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-10 6:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-07 6:06 -fpatchable-function-entry should set SHF_WRITE and create one __patchable_function_entries per function Fangrui Song 2020-01-07 7:26 ` Fangrui Song 2020-01-07 10:32 ` Szabolcs Nagy 2020-01-08 9:16 ` __patchable_function_entries is flawed Fangrui Song 2020-01-10 3:53 ` Fangrui Song 2020-01-10 6:28 ` Fangrui Song
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).