Add the __mcount_loc section on ppc64le. It has pointers to all the
mcount calls. This will enable the ftrace hook to be used for patched
functions.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com> (rebased)
Some theoretically unchanged functions can have undesired changes if the
compiler decides to perform inlining in a different way (e.g. because of
newly added references). In such a case, it can be useful to discard
changes to functions that don't actually need modification.
Sadly, this currently doesn't work for functions missing the ftrace hook
(e.g. notrace code) as presence of the hook is checked before
identifying elements to ignore.
Look for functions/sections to ignore earlier.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
Starting with Linux 5.8, vmlinux-specific KLP relas are applied early,
before all the special section initializations are done.
This means that jump labels can now be supported for cases where the
corresponding static keys live in the core kernel (vmlinux).
It also means that paravirt patching and alternatives can also now be
supported without the need for the .klp.arch sections.
This simplifies things greatly for newer kernels. We just have to make
sure that module-specific KLP relas aren't created for special sections.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
We have recently encountered a situation when a patched function
had more than one jump label (static branches with the same static key
used to turn on/off some debugging feature). As it is often the case
with jump labels, their locations were far from obvious in the source
code, hidden in the chains of inline functions.
create-diff-object, however, exits after it has reported one jump label.
This is inconvenient, because, after one updates the patch to avoid
that jump label, the next build of the binary patch reveals another
one and fails again, and so on. It can be very time-consuming.
Let us report all jump labels first.
Before this commit the messages looked like this:
kpatch-build/create-diff-object: ERROR: dev.o:
kpatch_regenerate_special_section: 2084:
Found a jump label at ploop_req_state_process()+0x220, using key css_stacks_on.
Jump labels aren't currently supported. Use static_key_enabled() instead.
After:
dev.o: Found a jump label at ploop_req_state_process+0x220, key: css_stacks_on.
dev.o: Found a jump label at ploop_ioctl+0x2708, key: css_stacks_on.
kpatch-build/create-diff-object: ERROR: dev.o:
kpatch_regenerate_special_section: 2123:
Found 2 jump label(s) in the patched code.
Jump labels aren't currently supported. Use static_key_enabled() instead.
Signed-off-by: Evgenii Shatokhin <eshatokhin@virtuozzo.com>
The dynrela (aka .klp.rela) conversion logic is notoriously complex and
fragile. Simplify it and improve the comments.
This is mainly a cosmetic change. In theory it shouldn't change
functionality or break anything.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Reverse the if condition and use a 'continue' statement to reduce
indentation and improve readability.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
A symbol in the original object might get split in several sub-functions
in the patched object, which can themselves be bundled (and use a
separate rela section). References to local static variables from the
original function, might have been moved in one of the sub-functions
in the patched object.
Look for references to local static variables in the rela section
of child symbols in the patched object.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
Consider symbols containing .part. in their names as sub-function
of the symbols they are derived from (if such symbol still exists in the
object file).
Signed-off-by: Julien Thierry <jthierry@redhat.com>
A symbol associated to a function can be split into multiple
sub-functions. Currently, kpatch only supports one child per function.
Extend this to support an arbitrary number of sub-function per function.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
When a child symbol has changed, the parent symbol is only needed
in the output object if the child symbol is unpatchable on its own.
This is the case when the child symbol does not have its own profiling
call.
Only include unchanged parent symbols if their child has changed and
the child does not have a function profiling call.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
When a ppcle64 ".toc" section contains only constants, the compiler
might not (won't?) create a corresponding ".rela.toc" section.
In such cases, create-diff-object crashes, assuming ".rela.toc" exists
whenever .toc exists. Simply report that no rela are available when
looking up possible relocations in .toc.
Fixes#1078.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
Internal CI is reporting a SIGSEGV in create-diff-object when it
processes macro-callbacks.patch, starting with 19baa5b7c7
("create-diff-object: process debug sections last").
The problem is that, after changing the order between callback and debug
section inclusion, kpatch_include_debug_sections() now tries to include
the callback section symbols. But kpatch_include_callback_elements()
inadvertently un-includes the callback section symbols (e.g.,
".kpatch.callbacks.pre_patch") when it un-includes the callback struct
symbols (e.g., "kpatch_pre_patch_data").
So after kpatch_elf_teardown(kelf_patched), the callback section symbols
get freed even though there are DWARF .debug_info relocations which
reference them. Then kpatch_check_relocations() goes off into the weeds
when it accesses one of the freed symbols.
Fix it by refining the callback un-include logic so that it *only*
strips the struct object symbols.
Fixes: 19baa5b7c7 ("create-diff-object: process debug sections last")
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
On x86_64, GCC generates the following instruction to compute
'empty_zero_page - __START_KERNEL_map' (__phys_addr_nodebug(), used in
the implementation of ZERO_PAGE()):
48 ba 00 00 00 00 00 00 00 00 movabs $0x0,%rdx
R_X86_64_64 empty_zero_page+0x80000000
__START_KERNEL_map is 0xffffffff80000000.
However, the relocation addend becomes wrong in the patch module:
48 ba 00 00 00 00 00 00 00 00 movabs $0x0,%rdx
R_X86_64_64 empty_zero_page-0x80000000
Note the sign of the addend.
As a result, ZERO_PAGE(0) returns a wrong value in any function touched
by the patch, which may lead to memory corruption and difficult-to-debug
kernel crashes.
The cause is that 'struct rela' uses 'int' for the addend, which is not
enough to store such values. r_addend from Elf64_Rela is int64_t
(Elf64_Sxword) for that.
Let us use 'long' instead of 'int' for the addend in 'struct rela'.
v2:
* Moved 'addend' field after 'offset' in struct rela to facilitate
structure packing (suggested by Kamalesh Babulal).
Fixes https://github.com/dynup/kpatch/issues/1064.
Signed-off-by: Evgenii Shatokhin <eshatokhin@virtuozzo.com>
The current code to find the twin of a local static variable allows two
variables of the same name to be wrongly matched with the other's twin.
While there isn't a magic formula to avoid this, make stricter
requirements for twining static local from the original object with
a symbol from the patched object. This reduces the risk of erroneous
matches.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
Process the debug sections only after all the other inclusion logic has
finished, since it makes decisions based on what else has already been
included.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Simplify static local variable correlation and renaming code by using
the newly introduced helpers for section and symbol correlation.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
Change 935f199875 ('create-diff-object: simplify mangled function
correlation') simplified the way symbols are correlated and got rid of
symbol section renaming.
As a result a symbol/section can now have a CHANGED status, being
correlated to an element that doesn't have the exact same name. This
will cause lookups to the original object fail when creating the new
patch object.
So lets bring back the symbol/section renaming, but only once they
have actually been correlated.
Fixes: 935f199875 ('create-diff-object: simplify mangled function
correlation')
Signed-off-by: Julien Thierry <jthierry@redhat.com>
Elements from the original object and the patched object can be
correlated using their mangled names. In case an elements (section or
symbol) could be matched with more than one object through mangling,
make sure all elements related to a section are correlated with the
corresponding elements of the twin section.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
The RHEL powerpc kernel is compiled with -O3, which triggers some
"interesting" new optimizations. One of them, which seems to be
relatively common, is the replacing of a function with two separate
"constprop" functions.
Previously we only ever saw a single constprop clone, so we just renamed
the patched version of the function to match the original version. Now
that we can have two such clones, that no longer makes sense.
Instead of renaming functions, just improve the correlation logic such
that they can be correlated despite having slightly different symbol
names. The first clone in the original object is correlated with the
first clone in the patched object; the second clone is correlated with
the second clone; and so on.
This assumes that the order of the symbols and sections doesn't change,
which seems to be a reasonable assumption based on past experience with
the compiler. Otherwise it will just unnecessarily mark the cloned
constprop functions as changed, which is annoying but harmless, and
noticeable by a human anyway.
Fixes#935.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
If the symbol associated with a relocation does not have a section set,
nothing is done for that relocation.
Skip iterating through all the symbols of the ELF file in such a case.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
rela_insn() only retrieves information about an instruction and does not
modify sections or relocations.
Add const to make this explicit.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
There is no point inspecting through the symbols of the ELF files
(original and patched) when the ELF headers do not meet requirements.
Check ELF headers as soon as the files are mapped.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
We are seeing the following error on a real world patch:
unsupported reference to special section __barrier_nospec_fixup
The kpatch commit bb444c2168 ("create-diff-object: Check for *_fixup
sections changes") created this error because we were trying to be
future proof. However, that may have been overly paranoid, as it
doesn't seem likely that those fixup sections will need relocations
anytime soon, because the replacement instructions are manually
generated in code. And anyway that "future proof" commit breaks the
present.
Also we decided at LPC that we are going to remove .klp.arch sections
anyway, so once that happens we will be fully future-proof anyway.
This reverts commit bb444c2168.
Fixes#974.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
While static keys (jump labels) are currently broken in livepatch, a
broken dynamic debug static key is harmless since it just disables
dynamically enabled debug printks in the patched code.
Fixes: #1021
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
We saw the following panic on ppc64le when loading the macro-callbacks
integration test:
livepatch: enabling patch 'kpatch_macro_callbacks'
Oops: Exception in kernel mode, sig: 4 [#1]
LE SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: kpatch_macro_callbacks(OEK+) rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver nfs lockd grace fscache sunrpc sg pseries_rng xts vmx_crypto xfs libcrc32c sd_mod ibmvscsi scsi_transport_srp ibmveth dm_mirror dm_region_hash dm_log dm_mod [last unloaded: kpatch_gcc_static_local_var_6]
CPU: 2 PID: 17445 Comm: insmod Kdump: loaded Tainted: G OE K --------- - - 4.18.0-128.el8.ppc64le #1
NIP: d00000000bb708e0 LR: c0000000001fd610 CTR: d00000000bb708e0
REGS: c00000040e98f640 TRAP: 0700 Tainted: G OE K --------- - - (4.18.0-128.el8.ppc64le)
MSR: 800000000288b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 28008228 XER: 20040003
CFAR: c0000000001fd60c IRQMASK: 0
GPR00: c0000000001fd5c0 c00000040e98f8c0 c000000001662a00 c000000733525400
GPR04: 0000000000000800 0000000000000800 c0000000015e2c00 c0000007335254a8
GPR08: 0000000000000001 d00000000bb708e0 c0000007eeb68400 0000000000000000
GPR12: d00000000bb708e0 c000000007fad600 0000000000000001 aaaaaaaaaaaaaaab
GPR16: 000000000000ff20 000000000000fff1 000000000000fff2 d00000000bb90000
GPR20: 00000000000000a9 c00000040e98fc00 c000000000d8a728 c00000040e98fc00
GPR24: d00000000bb73f88 00000000006080c0 d00000000bb73a38 c000000733525400
GPR28: 0000000000000001 c000000733525400 ffffffffffffffed c0000007eeb60900
NIP [d00000000bb708e0] callback_info.isra.0+0x7c/0x66c [kpatch_macro_callbacks]
LR [c0000000001fd610] __klp_enable_patch+0x130/0x230
Call Trace:
[c00000040e98f8c0] [c0000000001fd5c0] __klp_enable_patch+0xe0/0x230 (unreliable)
[c00000040e98f940] [c0000000001fd7d8] klp_enable_patch+0xc8/0x100
[c00000040e98f980] [d00000000bb7079c] patch_init+0x460/0x4cc [kpatch_macro_callbacks]
[c00000040e98fa20] [c000000000010108] do_one_initcall+0x58/0x248
[c00000040e98fae0] [c00000000023b860] do_init_module+0x80/0x330
[c00000040e98fb70] [c0000000002416a4] load_module+0x3994/0x3d00
[c00000040e98fd30] [c000000000241cf4] sys_finit_module+0xc4/0x130
[c00000040e98fe30] [c00000000000b388] system_call+0x5c/0x70
Instruction dump:
7cea482a 48000235 e8410018 48000014 3c620000 e8638160 48000221 e8410018
38210060 e8010010 7c0803a6 4e800020 <0000ae18> 00000000 3c4c0001 3842ae18
The problem was introduced by a recent fix:
e8f7f2dfe8 ("create-diff-object/ppc64le: Fix replace_sections_syms() for bundled symbols")
We didn't notice the fact that there's a hack in
kpatch_include_callback_elements() which reverts the work of
kpatch_replace_sections_syms() for callback function symbols.
The problem is that that revert is only partial, causing the callback
pointers to point to the .TOC data which is located 8 bytes before the
start of the function code. This happens because
kpatch_include_callback_elements() makes the same assumption that
kpatch_replace_sections_syms() had previously made: that bundled symbols
are always located at the start of their corresponding sections.
kpatch_include_callback_elements() mysteriously strips references to the
callback function symbols, replacing them with section symbols. In this
case it replaced a 'pre_patch_callback' function reference with a
'.text.unlikely.pre_patch_callback' section reference. But it didn't
adjust the rela->addend accordingly.
Joe discovered the reasoning for why kpatch_include_callback_elements()
removes function symbol references in the commit log for 7dfad2fb76
("fix dynrela corruption in load/unload hooks"):
In the case of the hook functions, we strip the FUNC symbol to prevent
it from being added to the kpatch.funcs section as a patched function.
But that justification doesn't really make sense, at least not with the
current code. Callbacks aren't added to .kpatch.funcs anyway. They're
classifed as NEW. Only CHANGED functions are added to .kpatch.funcs.
So remove that hack, fixing this bug in the process.
This does have a side effect of showing the callback functions as new
functions, because their symbols are now included.
Before:
aio.o: found callback: post_unpatch_callback
aio.o: found callback: pre_patch_callback
aio.o: found callback: pre_unpatch_callback
aio.o: new function: callback_info.isra.0
After:
aio.o: found callback: post_unpatch_callback
aio.o: found callback: pre_patch_callback
aio.o: found callback: pre_unpatch_callback
aio.o: new function: callback_info.isra.0
aio.o: new function: pre_patch_callback
aio.o: new function: post_patch_callback
aio.o: new function: pre_unpatch_callback
aio.o: new function: post_unpatch_callback
But anyway they _are_ new functions, so the new output seems more
correct to me.
Fixes: e8f7f2dfe8 ("create-diff-object/ppc64le: Fix replace_sections_syms() for bundled symbols")
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
The existing comment is wrong. It confusingly conflates the function's
offset, which is 8 bytes from the beginning of the section, with the
function's localentry offset which is 8 bytes from the beginning of the
function.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
group_size variable is assigned right after we enter for loop without
ever being read so there is no need to initialize it to 0 beforehand.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
"funcs" in kpatch_create_patches_sections() and "entries" in
kpatch_create_kpatch_arch_section() were only used by sizeof, replaced
those with corresponding types.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
Make sure symtab section was found before dereferencing it.
Found by covscan, see issue #984 for full log.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
newdata variable is allocated through malloc call and requires a NULL
check.
Found by covscan, see issue #984 for full log.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
Since ORC_STRUCT_SIZE is used for division in
kpatch_regenerate_orc_sections() we need to make sure that it is
properly set.
Found by covscan, see issue #984 for full log.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
Make sure fixup section was found before dereferencing it.
Found by covscan, see issue #984 for full log.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
Make sure rela_toc(1|2) are not null before dereferencing them in
rela_equal().
Found by covscan, see issue #984 for full log.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
