This adds support for emitting special sections such as initcall arrays,
PCI fixups and tracepoints as relative references rather than absolute
references. This reduces the size by 50% on 64-bit architectures, but
more importantly, it removes the need for carrying relocation metadata
for these sections in relocatables kernels (e.g., for KASLR) that need
to fix up these absolute references at boot time. On arm64, this reduces
the vmlinux footprint of such a reference by 8x (8 byte absolute reference
+ 24 byte RELA entry vs 4 byte relative reference)
Patch #2 was sent out before as a single patch. This series supersedes
the previous submission. This version makes relative ksymtab entries
dependent on the new Kconfig symbol HAVE_ARCH_PREL32_RELOCATIONS rather
than trying to infer from kbuild test robot replies for which architectures
it should be blacklisted.
Patch #1 introduces the new Kconfig symbol HAVE_ARCH_PREL32_RELOCATIONS,
and sets it for the main architectures that are expected to benefit the
most from this feature, i.e., 64-bit architectures or ones that use
runtime relocations.
Patches #3 - #5 implement relative references for initcalls, PCI fixups
and tracepoints, respectively, all of which produce sections with order
~1000 entries on an arm64 defconfig kernel with tracing enabled. This
means we save about 28 KB of vmlinux space for each of these patches.
Patches #6 - #8 have been added in v5, and implement relative references
in jump tables for arm64 and x86. On arm64, this results in significant
space savings (650+ KB on a typical distro kernel). On x86, the savings
are not as impressive, but still worthwhile. (Note that these patches
do not rely on CONFIG_HAVE_ARCH_PREL32_RELOCATIONS, given that the
inline asm that is emitted is already per-arch)
For the arm64 kernel, all patches combined reduce the memory footprint of
vmlinux by about 1.3 MB (using a config copied from Ubuntu that has KASLR
enabled), of which ~1 MB is the size reduction of the RELA section in .init,
and the remaining 300 KB is reduction of .text/.data.
Branch:
git://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git relative-special-sections-v6
Changes since v5:
- add missing jump_label prototypes to s390 jump_label.h (#6)
- fix inverted condition in call to jump_entry_is_module_init() (#6)
Changes since v4:
- add patches to convert x86 and arm64 to use relative references for jump
tables (#6 - #8)
- rename PCI patch and add Bjorn's ack (#4)
- rebase onto v4.15-rc5
Changes since v3:
- fix module unload issue in patch #5 reported by Jessica, by reusing the
updated routine for_each_tracepoint_range() for the quiescent check at
module unload time; this requires this routine to be moved before
tracepoint_module_going() in kernel/tracepoint.c
- add Jessica's ack to #2
- rebase onto v4.14-rc1
Changes since v2:
- Revert my slightly misguided attempt to appease checkpatch, which resulted
in needless churn and worse code. This v3 is based on v1 with a few tweaks
that were actually reasonable checkpatch warnings: unnecessary braces (as
pointed out by Ingo) and other minor whitespace misdemeanors.
Changes since v1:
- Remove checkpatch errors to the extent feasible: in some cases, this
involves moving extern declarations into C files, and switching to
struct definitions rather than typedefs. Some errors are impossible
to fix: please find the remaining ones after the diffstat.
- Used 'int' instead if 'signed int' for the various offset fields: there
is no ambiguity between architectures regarding its signedness (unlike
'char')
- Refactor the different patches to be more uniform in the way they define
the section entry type and accessors in the .h file, and avoid the need to
add #ifdefs to the C code.
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ralf Baechle <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Heiko Carstens <redacted>
Cc: Kees Cook <redacted>
Cc: Will Deacon <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Thomas Garnier <redacted>
Cc: Thomas Gleixner <redacted>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Paul Mackerras <redacted>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: James Morris <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nicolas Pitre <redacted>
Cc: Josh Poimboeuf <redacted>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Martin Schwidefsky <redacted>
Cc: Sergey Senozhatsky <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Jessica Yu <jeyu@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-s390@vger.kernel.org
Cc: sparclinux@vger.kernel.org
Cc: x86@kernel.org
Ard Biesheuvel (8):
arch: enable relative relocations for arm64, power, x86, s390 and x86
module: use relative references for __ksymtab entries
init: allow initcall tables to be emitted using relative references
PCI: Add support for relative addressing in quirk tables
kernel: tracepoints: add support for relative references
kernel/jump_label: abstract jump_entry member accessors
arm64/kernel: jump_label: use relative references
x86/kernel: jump_table: use relative references
arch/Kconfig | 10 ++++
arch/arm/include/asm/jump_label.h | 27 +++++++++
arch/arm64/Kconfig | 1 +
arch/arm64/include/asm/jump_label.h | 48 +++++++++++++---
arch/arm64/kernel/jump_label.c | 22 +++++++-
arch/arm64/kernel/vmlinux.lds.S | 2 +-
arch/mips/include/asm/jump_label.h | 27 +++++++++
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/jump_label.h | 27 +++++++++
arch/s390/Kconfig | 1 +
arch/s390/include/asm/jump_label.h | 20 +++++++
arch/sparc/include/asm/jump_label.h | 27 +++++++++
arch/tile/include/asm/jump_label.h | 27 +++++++++
arch/x86/Kconfig | 1 +
arch/x86/include/asm/Kbuild | 1 +
arch/x86/include/asm/export.h | 5 --
arch/x86/include/asm/jump_label.h | 56 +++++++++++++++----
arch/x86/kernel/jump_label.c | 59 ++++++++++++++------
drivers/pci/quirks.c | 13 ++++-
include/asm-generic/export.h | 12 +++-
include/linux/compiler.h | 11 ++++
include/linux/export.h | 46 +++++++++++----
include/linux/init.h | 44 +++++++++++----
include/linux/pci.h | 20 +++++++
include/linux/tracepoint.h | 19 +++++--
init/main.c | 32 +++++------
kernel/jump_label.c | 38 ++++++-------
kernel/module.c | 33 +++++++++--
kernel/printk/printk.c | 4 +-
kernel/tracepoint.c | 50 +++++++++--------
security/security.c | 4 +-
tools/objtool/special.c | 4 +-
32 files changed, 544 insertions(+), 148 deletions(-)
delete mode 100644 arch/x86/include/asm/export.h
--
2.11.0
Before updating certain subsystems to use place relative 32-bit
relocations in special sections, to save space and reduce the
number of absolute relocations that need to be processed at runtime
by relocatable kernels, introduce the Kconfig symbol and define it
for some architectures that should be able to support and benefit
from it.
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <redacted>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Martin Schwidefsky <redacted>
Cc: Heiko Carstens <redacted>
Cc: Thomas Gleixner <redacted>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/Kconfig | 10 ++++++++++
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/vmlinux.lds.S | 2 +-
arch/powerpc/Kconfig | 1 +
arch/s390/Kconfig | 1 +
arch/x86/Kconfig | 1 +
6 files changed, 15 insertions(+), 1 deletion(-)
Allow the initcall tables to be emitted using relative references that
are only half the size on 64-bit architectures and don't require fixups
at runtime on relocatable kernels.
Cc: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <redacted>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: James Morris <redacted>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
include/linux/init.h | 44 +++++++++++++++-----
init/main.c | 32 +++++++-------
kernel/printk/printk.c | 4 +-
security/security.c | 4 +-
4 files changed, 53 insertions(+), 31 deletions(-)
@@ -109,8 +109,24 @@typedefint(*initcall_t)(void);typedefvoid(*exitcall_t)(void);-externinitcall_t__con_initcall_start[],__con_initcall_end[];-externinitcall_t__security_initcall_start[],__security_initcall_end[];+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS+typedefsignedintinitcall_entry_t;++staticinlineinitcall_tinitcall_from_entry(initcall_entry_t*entry)+{+return(initcall_t)((unsignedlong)entry+*entry);+}+#else+typedefinitcall_tinitcall_entry_t;++staticinlineinitcall_tinitcall_from_entry(initcall_entry_t*entry)+{+return*entry;+}+#endif++externinitcall_entry_t__con_initcall_start[],__con_initcall_end[];+externinitcall_entry_t__security_initcall_start[],__security_initcall_end[];/* Used for contructor calls. */typedefvoid(*ctor_fn_t)(void);
Allow the PCI quirk tables to be emitted in a way that avoids absolute
references to the hook functions. This reduces the size of the entries,
and, more importantly, makes them invariant under runtime relocation
(e.g., for KASLR)
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
drivers/pci/quirks.c | 13 ++++++++++---
include/linux/pci.h | 20 ++++++++++++++++++++
2 files changed, 30 insertions(+), 3 deletions(-)
@@ -1792,7 +1792,11 @@ struct pci_fixup {u16device;/* You can use PCI_ANY_ID here of course */u32class;/* You can use PCI_ANY_ID here too */unsignedintclass_shift;/* should be 0, 8, 16 */+#ifdef CONFIG_HAVE_ARCH_PREL32_RELOCATIONS+signedinthook_offset;+#elsevoid(*hook)(structpci_dev*dev);+#endif};enumpci_fixup_pass{
To avoid the need for relocating absolute references to tracepoint
structures at boot time when running relocatable kernels (which may
take a disproportionate amount of space), add the option to emit
these tables as relative references instead.
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Ard Biesheuvel <redacted>
---
include/linux/tracepoint.h | 19 ++++++--
kernel/tracepoint.c | 50 +++++++++++---------
2 files changed, 42 insertions(+), 27 deletions(-)
On a randomly chosen distro kernel build for arm64, vmlinux.o shows the
following sections, containing jump label entries, and the associated
RELA relocation records, respectively:
...
[38088] __jump_table PROGBITS 0000000000000000 00e19f30
000000000002ea10 0000000000000000 WA 0 0 8
[38089] .rela__jump_table RELA 0000000000000000 01fd8bb0
000000000008be30 0000000000000018 I 38178 38088 8
...
In other words, we have 190 KB worth of 'struct jump_entry' instances,
and 573 KB worth of RELA entries to relocate each entry's code, target
and key members. This means the RELA section occupies 10% of the .init
segment, and the two sections combined represent 5% of vmlinux's entire
memory footprint.
So let's switch from 64-bit absolute references to 32-bit relative
references: this reduces the size of the __jump_table by 50%, and gets
rid of the RELA section entirely.
Note that this requires some extra care in the sorting routine, given
that the offsets change when entries are moved around in the jump_entry
table.
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/arm64/include/asm/jump_label.h | 27 ++++++++++++--------
arch/arm64/kernel/jump_label.c | 22 +++++++++++++---
2 files changed, 36 insertions(+), 13 deletions(-)
Similar to the arm64 case, 64-bit x86 can benefit from using 32-bit
relative references rather than 64-bit absolute ones when emitting
struct jump_entry instances. Not only does this reduce the memory
footprint of the entries themselves by 50%, it also removes the need
for carrying relocation metadata on relocatable builds (i.e., for KASLR)
which saves a fair chunk of .init space as well (although the savings
are not as dramatic as on arm64)
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/x86/include/asm/jump_label.h | 35 +++++++-----
arch/x86/kernel/jump_label.c | 59 ++++++++++++++------
tools/objtool/special.c | 4 +-
3 files changed, 65 insertions(+), 33 deletions(-)
An ordinary arm64 defconfig build has ~64 KB worth of __ksymtab
entries, each consisting of two 64-bit fields containing absolute
references, to the symbol itself and to a char array containing
its name, respectively.
When we build the same configuration with KASLR enabled, we end
up with an additional ~192 KB of relocations in the .init section,
i.e., one 24 byte entry for each absolute reference, which all need
to be processed at boot time.
Given how the struct kernel_symbol that describes each entry is
completely local to module.c (except for the references emitted
by EXPORT_SYMBOL() itself), we can easily modify it to contain
two 32-bit relative references instead. This reduces the size of
the __ksymtab section by 50% for all 64-bit architectures, and
gets rid of the runtime relocations entirely for architectures
implementing KASLR, either via standard PIE linking (arm64) or
using custom host tools (x86).
Note that the binary search involving __ksymtab contents relies
on each section being sorted by symbol name. This is implemented
based on the input section names, not the names in the ksymtab
entries, so this patch does not interfere with that.
Given that the use of place-relative relocations requires support
both in the toolchain and in the module loader, we cannot enable
this feature for all architectures. So make it dependent on whether
CONFIG_HAVE_ARCH_PREL32_RELOCATIONS is defined.
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Kees Cook <redacted>
Cc: Thomas Garnier <redacted>
Cc: Nicolas Pitre <redacted>
Acked-by: Jessica Yu <jeyu@kernel.org>
Signed-off-by: Ard Biesheuvel <redacted>
---
arch/x86/include/asm/Kbuild | 1 +
arch/x86/include/asm/export.h | 5 ---
include/asm-generic/export.h | 12 ++++-
include/linux/compiler.h | 11 +++++
include/linux/export.h | 46 +++++++++++++++-----
kernel/module.c | 33 +++++++++++---
6 files changed, 84 insertions(+), 24 deletions(-)
@@ -2228,10 +2247,12 @@ static int verify_export_symbols(struct module *mod)for(i=0;i<ARRAY_SIZE(arr);i++){for(s=arr[i].sym;s<arr[i].sym+arr[i].num;s++){-if(find_symbol(s->name,&owner,NULL,true,false)){+if(find_symbol(kernel_symbol_name(s),&owner,NULL,+true,false)){pr_err("%s: exports duplicate symbol %s"" (owned by %s)\n",-mod->name,s->name,module_name(owner));+mod->name,kernel_symbol_name(s),+module_name(owner));return-ENOEXEC;}}
@@ -2280,7 +2301,7 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)ksym=resolve_symbol_wait(mod,info,name);/* Ok if resolved. */if(ksym&&!IS_ERR(ksym)){-sym[i].st_value=ksym->value;+sym[i].st_value=kernel_symbol_value(ksym);break;}
@@ -2540,7 +2561,7 @@ static int is_exported(const char *name, unsigned long value,ks=lookup_symbol(name,__start___ksymtab,__stop___ksymtab);elseks=lookup_symbol(name,mod->syms,mod->syms+mod->num_syms);-returnks!=NULL&&ks->value==value;+returnks!=NULL&&kernel_symbol_value(ks)==value;}/* As per nm */
Weren't you supposed to explain this part in the commit message?
It isn't obvious why this is mixed up with the Kconfig changes, and
somebody already asked about it. The commit message only talks about
the Kconfig changes, and then suddenly there's that odd vmlinux.lds.S
change in there...
Linus
Weren't you supposed to explain this part in the commit message?
Oops. Apologies, I indeed forgot to update the commit log.
It isn't obvious why this is mixed up with the Kconfig changes, and
somebody already asked about it. The commit message only talks about
the Kconfig changes, and then suddenly there's that odd vmlinux.lds.S
change in there...
Yeah. It doesn't make sense to respin right away for just that, so I
will give people some time to respond, and respin in a week or so.
@@ -327,4 +327,15 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int scompiletime_assert(__native_word(t),\"Need native word sized stores/loads for atomicity.")+/*+*Forcethecompilertoemit'sym'asasymbol,sothatwecanreference+*itfrominlineassembler.Necessaryincase'sym'couldbeinlined+*otherwise,oreliminatedentirelyduetolackofreferencesthatare+*visibiletothecompiler.+*/+#define __ADDRESSABLE(sym) \+staticvoid*__attribute__((section(".discard.text"),used))\+__PASTE(__discard_##sym,__LINE__)(void)\+{return(void*)&sym;}\+#endif /* __LINUX_COMPILER_H */
Isn't this logically the point where you should add the arm64
vmlinux.lds.S change, and explain how ".discard.text" turns into
".init.discard.text" for some odd arm64 reason?
Linus
@@ -327,4 +327,15 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int scompiletime_assert(__native_word(t),\"Need native word sized stores/loads for atomicity.")+/*+*Forcethecompilertoemit'sym'asasymbol,sothatwecanreference+*itfrominlineassembler.Necessaryincase'sym'couldbeinlined+*otherwise,oreliminatedentirelyduetolackofreferencesthatare+*visibiletothecompiler.+*/+#define __ADDRESSABLE(sym) \+staticvoid*__attribute__((section(".discard.text"),used))\+__PASTE(__discard_##sym,__LINE__)(void)\+{return(void*)&sym;}\+#endif /* __LINUX_COMPILER_H */
Isn't this logically the point where you should add the arm64
vmlinux.lds.S change, and explain how ".discard.text" turns into
".init.discard.text" for some odd arm64 reason?
I tried to keep the generic patches generic, so perhaps I should just
put the arm64 vmlinux.lds.S change in a patch on its own?
On Wed, Dec 27, 2017 at 12:11 PM, Ard Biesheuvel
[off-list ref] wrote:
I tried to keep the generic patches generic, so perhaps I should just
put the arm64 vmlinux.lds.S change in a patch on its own?
I guess it doesn't matter, but regardless of where it gets introduced
I would like to see the explanation for where the heck that magical
".init.discard.text" comes from. It's definitely not obvious from the
patches, and is presumably some odd arm64 special case.
Linus
On 27 December 2017 at 20:13, Linus Torvalds
[off-list ref] wrote:
On Wed, Dec 27, 2017 at 12:11 PM, Ard Biesheuvel
[off-list ref] wrote:
quoted
I tried to keep the generic patches generic, so perhaps I should just
put the arm64 vmlinux.lds.S change in a patch on its own?
I guess it doesn't matter, but regardless of where it gets introduced
I would like to see the explanation for where the heck that magical
".init.discard.text" comes from. It's definitely not obvious from the
patches, and is presumably some odd arm64 special case.
This has to do with the EFI stub. x86 and ARM link it into the
decompressor, and so the code and data are not annotated as __init
(and doing so would involve modifying a lot of code). arm64 does not
have a decompressor, and so the EFI stub is linked into the kernel
proper. To make sure the code ends up in the .init segment, all
sections are prepended with .init at the object level, using objcopy.
Annoyingly, we need this because there is a single instance of a
special section that ends up in the EFI stub code: we build lib/sort.c
again as a EFI libstub object, and given that sort() is exported, we
end up with a ksymtab section in the EFI stub. The sort() thing has
caused issues before [0], so perhaps I should just clone sort.c into
drivers/firmware/efi/libstub and get rid of that hack.
[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=29f9007b3182ab3f328a31da13e6b1c9072f7a95
Annoyingly, we need this because there is a single instance of a
special section that ends up in the EFI stub code: we build lib/sort.c
again as a EFI libstub object, and given that sort() is exported, we
end up with a ksymtab section in the EFI stub. The sort() thing has
caused issues before [0], so perhaps I should just clone sort.c into
drivers/firmware/efi/libstub and get rid of that hack.
[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=29f9007b3182ab3f328a31da13e6b1c9072f7a95
If the root problem is early bootstrap code randomly using generic facility that
isn't __init, then we should definitely improve tooling to at least detect these
problems.
As bootstrap code gets improved (KASLR, more complex decompression, etc. etc.) we
keep using new bits of generic facilities...
So this should definitely not be hidden by open coding that function (which has
various other disadvantages as well), but should be turned from silent breakage
either into non-breakage (and do so not only for sort() but for other generic
functions as well), or should be turned into a build failure.
Thanks,
Ingo
On 28 December 2017 at 12:05, Ingo Molnar [off-list ref] wrote:
* Ard Biesheuvel [off-list ref] wrote:
quoted
Annoyingly, we need this because there is a single instance of a
special section that ends up in the EFI stub code: we build lib/sort.c
again as a EFI libstub object, and given that sort() is exported, we
end up with a ksymtab section in the EFI stub. The sort() thing has
caused issues before [0], so perhaps I should just clone sort.c into
drivers/firmware/efi/libstub and get rid of that hack.
[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=29f9007b3182ab3f328a31da13e6b1c9072f7a95
If the root problem is early bootstrap code randomly using generic facility that
isn't __init, then we should definitely improve tooling to at least detect these
problems.
As bootstrap code gets improved (KASLR, more complex decompression, etc. etc.) we
keep using new bits of generic facilities...
So this should definitely not be hidden by open coding that function (which has
various other disadvantages as well), but should be turned from silent breakage
either into non-breakage (and do so not only for sort() but for other generic
functions as well), or should be turned into a build failure.
We already have safeguards in place to ensure that the arm64 EFI stub
(which is essentially the same executable as the kernel proper) only
pulls in code that has been made available to it explicitly. That is
why sort.c is recompiled for the EFI stub, as well as all other C code
that is shared between the stub and the kernel. We also have a build
time check to ensure that the resulting code does not rely on absolute
symbol references, which will be invalid in the UEFI execution
context.
So the only problem is that unneeded ksymtab/kcrctab sections, which
affected ARM for obscure reasons; typically, they just take up some
space. On x86, the kaslr code deals with a similar issue by
#define'ing _LINUX_EXPORT_H before including linux/export.h, which
also gets rid of these sections, but I was a bit reluctant to copy
that pattern. Perhaps we should enhance linux/export.h for reasons
such as these by adding a macro that nops out EXPORT_SYMBOL()
declarations?
From: Steven Rostedt <rostedt@goodmis.org> Date: 2017-12-28 15:42:17
On Wed, 27 Dec 2017 08:50:30 +0000
Ard Biesheuvel [off-list ref] wrote:
To avoid the need for relocating absolute references to tracepoint
structures at boot time when running relocatable kernels (which may
take a disproportionate amount of space), add the option to emit
these tables as relative references instead.
I gave this patch a quick skim over. It appears to not modify anything
when CONFIG_HAVE_PREL32_RELOCATIONS is not defined. I haven't
thoroughly reviewed it or tested it. But if it doesn't break anything,
I'm fine giving you an ack.
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
I'm paranoid about doing arithmetic on abstract types. What happens in
the future if jump_label_t becomes a pointer? You will get a different
result.
Could we switch these calculations to something like:
return (jump_label_t)((long)&entrty->code + entry->code);
You have the functions already made before this patch. Perhaps we
should have a separate patch to use them (here and elsewhere) before
you make the conversion to using relative references. It will help out
in debugging and bisects. To know if the use of functions is an issue,
or the conversion of relative references is an issue.
I suggest splitting this into two patches.
-- Steve
quoted hunk
+ __LINE__);
} else {
/*
* ...otherwise expect an ideal_nop. Otherwise
* something went horribly wrong.
*/
- if (unlikely(memcmp((void *)entry->code, ideal_nop, 5)
- != 0))
- bug_at((void *)entry->code, __LINE__);
+ if (unlikely(memcmp((void *)jump_entry_code(entry),
+ ideal_nop, 5) != 0))
+ bug_at((void *)jump_entry_code(entry),
+ __LINE__);
}
code.jump = 0xe9;
- code.offset = entry->target -
- (entry->code + JUMP_LABEL_NOP_SIZE);
+ code.offset = jump_entry_target(entry) -
+ (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
} else {
/*
* We are disabling this jump label. If it is not what
@@ -76,14 +78,18 @@ static void __jump_label_transform(struct jump_entry *entry, * are converting the default nop to the ideal nop. */ if (init) {- if (unlikely(memcmp((void *)entry->code, default_nop, 5) != 0))- bug_at((void *)entry->code, __LINE__);+ if (unlikely(memcmp((void *)jump_entry_code(entry),+ default_nop, 5) != 0))+ bug_at((void *)jump_entry_code(entry),+ __LINE__); } else { code.jump = 0xe9;- code.offset = entry->target -- (entry->code + JUMP_LABEL_NOP_SIZE);- if (unlikely(memcmp((void *)entry->code, &code, 5) != 0))- bug_at((void *)entry->code, __LINE__);+ code.offset = jump_entry_target(entry) -+ (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);+ if (unlikely(memcmp((void *)jump_entry_code(entry),+ &code, 5) != 0))+ bug_at((void *)jump_entry_code(entry),+ __LINE__); } memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE); }
I'm paranoid about doing arithmetic on abstract types. What happens in
the future if jump_label_t becomes a pointer? You will get a different
result.
In general, I share your concern. In this case, however, jump_label_t
is typedef'd three lines up and is never used anywhere else.
Could we switch these calculations to something like:
return (jump_label_t)((long)&entrty->code + entry->code);
jump_label_t is local to this .h file, so it can be defined as u32 or
u64 depending on the word size. I don't mind adding the extra cast,
but I am not sure if your paranoia is justified in this particular
case. Perhaps we should just use 'unsigned long' throughout?
You have the functions already made before this patch. Perhaps we
should have a separate patch to use them (here and elsewhere) before
you make the conversion to using relative references. It will help out
in debugging and bisects. To know if the use of functions is an issue,
or the conversion of relative references is an issue.
I suggest splitting this into two patches.
Fair enough.
quoted
+ __LINE__);
} else {
/*
* ...otherwise expect an ideal_nop. Otherwise
* something went horribly wrong.
*/
- if (unlikely(memcmp((void *)entry->code, ideal_nop, 5)
- != 0))
- bug_at((void *)entry->code, __LINE__);
+ if (unlikely(memcmp((void *)jump_entry_code(entry),
+ ideal_nop, 5) != 0))
+ bug_at((void *)jump_entry_code(entry),
+ __LINE__);
}
code.jump = 0xe9;
- code.offset = entry->target -
- (entry->code + JUMP_LABEL_NOP_SIZE);
+ code.offset = jump_entry_target(entry) -
+ (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
} else {
/*
* We are disabling this jump label. If it is not what
@@ -76,14 +78,18 @@ static void __jump_label_transform(struct jump_entry *entry, * are converting the default nop to the ideal nop. */ if (init) {- if (unlikely(memcmp((void *)entry->code, default_nop, 5) != 0))- bug_at((void *)entry->code, __LINE__);+ if (unlikely(memcmp((void *)jump_entry_code(entry),+ default_nop, 5) != 0))+ bug_at((void *)jump_entry_code(entry),+ __LINE__); } else { code.jump = 0xe9;- code.offset = entry->target -- (entry->code + JUMP_LABEL_NOP_SIZE);- if (unlikely(memcmp((void *)entry->code, &code, 5) != 0))- bug_at((void *)entry->code, __LINE__);+ code.offset = jump_entry_target(entry) -+ (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);+ if (unlikely(memcmp((void *)jump_entry_code(entry),+ &code, 5) != 0))+ bug_at((void *)jump_entry_code(entry),+ __LINE__); } memcpy(&code, ideal_nops[NOP_ATOMIC5], JUMP_LABEL_NOP_SIZE); }
I'm paranoid about doing arithmetic on abstract types. What happens in
the future if jump_label_t becomes a pointer? You will get a different
result.
In general, I share your concern. In this case, however, jump_label_t
is typedef'd three lines up and is never used anywhere else.
I would agree if this was in a .c file, but it's in a header file,
which causes me to be more paranoid.
quoted
Could we switch these calculations to something like:
return (jump_label_t)((long)&entrty->code + entry->code);
jump_label_t is local to this .h file, so it can be defined as u32 or
u64 depending on the word size. I don't mind adding the extra cast,
but I am not sure if your paranoia is justified in this particular
case. Perhaps we should just use 'unsigned long' throughout?
Actually, that may be better. Have the return value be jump_label_t,
but the cast be "unsigned long". That way it should always work.
static inline jump_label_t jump_entry_code(...)
{
return (unsigned long)&entry->code + entry->code;
}
-- Steve
On 28 December 2017 at 15:42, Steven Rostedt [off-list ref] wrote:
On Wed, 27 Dec 2017 08:50:30 +0000
Ard Biesheuvel [off-list ref] wrote:
quoted
To avoid the need for relocating absolute references to tracepoint
structures at boot time when running relocatable kernels (which may
take a disproportionate amount of space), add the option to emit
these tables as relative references instead.
I gave this patch a quick skim over. It appears to not modify anything
when CONFIG_HAVE_PREL32_RELOCATIONS is not defined. I haven't
thoroughly reviewed it or tested it. But if it doesn't break anything,
I'm fine giving you an ack.
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Thank you Steven.
I should mention though (as you don't appear to recall) that an
earlier version of this patch triggered an issue for you
https://marc.info/?l=linux-arch&m=150584374820168&w=2
but I have never managed to reproduce it, neither at the time nor
currently with this v6.
ard@bezzzef:~/linux-2.6$ sudo tools/testing/selftests/ftrace/ftracetest
=== Ftrace unit tests ===
[1] Basic trace file check [PASS]
[2] Basic test for tracers [PASS]
[3] Basic trace clock test [PASS]
[4] Basic event tracing check [PASS]
[5] event tracing - enable/disable with event level files [PASS]
[6] event tracing - restricts events based on pid [PASS]
[7] event tracing - enable/disable with subsystem level files [PASS]
[8] event tracing - enable/disable with top level files [PASS]
[9] ftrace - function graph filters with stack tracer [PASS]
[10] ftrace - function graph filters [PASS]
[11] ftrace - test for function event triggers [PASS]
[12] ftrace - function glob filters [PASS]
[13] ftrace - function pid filters [PASS]
[14] ftrace - function profiler with function tracing [PASS]
[15] ftrace - test reading of set_ftrace_filter [PASS]
[16] ftrace - test for function traceon/off triggers [PASS]
[17] Test creation and deletion of trace instances while setting an event [PASS]
[18] Test creation and deletion of trace instances [PASS]
[19] Kprobe dynamic event - adding and removing [PASS]
[20] Kprobe dynamic event - busy event check [PASS]
[21] Kprobe dynamic event with arguments [PASS]
[22] Kprobes event arguments with types [PASS]
[23] Kprobe event auto/manual naming [PASS]
[24] Kprobe dynamic event with function tracer [PASS]
[25] Kprobe dynamic event - probing module [PASS]
[26] Kretprobe dynamic event with arguments [PASS]
[27] Kretprobe dynamic event with maxactive [PASS]
[28] Register/unregister many kprobe events [PASS]
[29] event trigger - test event enable/disable trigger [PASS]
[30] event trigger - test trigger filter [PASS]
[31] event trigger - test histogram modifiers [PASS]
[32] event trigger - test histogram trigger [PASS]
[33] event trigger - test multiple histogram triggers [PASS]
[34] event trigger - test snapshot-trigger [PASS]
[35] event trigger - test stacktrace-trigger [PASS]
[36] event trigger - test traceon/off trigger [PASS]
[37] (instance) Basic test for tracers [PASS]
[38] (instance) Basic trace clock test [PASS]
[39] (instance) event tracing - enable/disable with event level files [PASS]
[40] (instance) event tracing - restricts events based on pid [PASS]
[41] (instance) event tracing - enable/disable with subsystem level
files [PASS]
[42] (instance) ftrace - test for function event triggers [PASS]
[43] (instance) ftrace - test for function traceon/off triggers [PASS]
[44] (instance) event trigger - test event enable/disable trigger [PASS]
[45] (instance) event trigger - test trigger filter [PASS]
[46] (instance) event trigger - test histogram modifiers [PASS]
[47] (instance) event trigger - test histogram trigger [PASS]
[48] (instance) event trigger - test multiple histogram triggers [PASS]
# of passed: 48
# of failed: 0
# of unresolved: 0
# of untested: 0
# of unsupported: 0
# of xfailed: 0
# of undefined(test bug): 0