On powerpc, "readelf -s" of vmlinux shows both .dynsym and .symtab.
.dynsym is just a subset of .symtab, so skip it to avoid duplicates.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
symtab_read() is quite fragile because it relies on the fact that the
first and second loops have the exact same conditions.
Instead just change the first loop to count all the lines in the file,
to get an upper bound for allocation. It's ok to over-allocate
slightly.
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>
Skip building insn/* on x86 and gcc-plugin on Power with -Wconversion,
-Wno-sign-converion flags.
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Add -Wconversion and -Wno-sign-conversion to CFLAGS. The first flag
should catch any implicit conversions like the one seen with #1065 and
the second flag suppress the warnings between signed and unsigned
integers.
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.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>
When patching kernel module dm-persistent-data, I found
that the KOBJFILE_NAME is incorrectly replaced to
dm_persistent-data while the module name in kernel is
dm_persistent_data.
Signed-off-by: Zhipeng Xie <xiezhipeng1@huawei.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>
When freeing a kpatch_elf, another object might have symbols and
sections twined with elements that are getting freed.
Clear the twin references, so if they are used after the object they
reference is freed, the program will crash.
Signed-off-by: Julien Thierry <jthierry@redhat.com>
Kernel commit cb9b55d21fe0 modpost: add support for symbol namespaces
adds a new namespace column to Module.symvers file which can be blank.
fscanf is no longer a viable solution to parse that. Switch to the way
scripts/mod/modpost.c handles this and try to support both versions with
and without namespace column.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
Currently, only rela section get freed. This seems like a simple
scope mistake.
Free all sections regardless of their nature in kpatch_elf_teardown()
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>
Building a kpatch for a module with this Makefile:
The Makefile is as follow:
obj-m += m_hello.o
m_hello-y = hello.o
default:
$(MAKE) -C /lib/modules/4.4.21-69-default/build M=$(shell pwd) modules
clean:
$(MAKE) -C /lib/modules/4.4.21-69-default/build M=$(shell pwd) clean
results in kpatch-build "ERROR: two parent matches for hello.o".
The problem is that find_parent_obj() looks for filenames like so:
% grep -l hello.o ./.*.cmd | grep -Fv hello.o
.m_hello.ko.cmd
.m_hello.o.cmd
where .m_hello.ko.cmd is the parant for m_hello.o, and .m_hello.o.cmd is the
parant for hello.o, but because the "hello.o" is a substring of "m_hello.o",
it will cause "m_hello.o" to be matched for the "hello.o" as well.
Fix this by using grep's -w|--word-regexp option to force it to match
whole words instead of substrings.
Signed-off-by: chenzefeng <chenzefeng2@huawei.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>
Somewhere starting with 5.3 (probably with 9f69a496f100 "kbuild: split
out *.mod out of {single,multi}-used-m rules", but that is not
confirmed) .mod and correcponding .mod.cmd files started showing up
during module builds throwing off kpatch-build's find_parent_obj() func.
Filter out any files ending with .mod.cmd as they are definitely not the
parent.
Signed-off-by: Artem Savkov <artem.savkov@gmail.com>
Not every distro out there supports /etc/os-release file.
This file is useful for obtaining given distro defaults, but not
essential for the script to work (when all parameters are passed
on a command line).
To avoid warnings or unwanted errors, make sourcing of this file
conditional.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Run the input patch(es) through lsdiff and then verify that no obviously
unsupported files are directly modified (e.g. assembly .S files).
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Recent distros don't require you to set 'ulimit -c unlimited'. Instead
they place core files in a distro-specific location. Update the SIGSEGV
error message accordingly.
Fixes: #1025
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>
Josh Poimboeuf
Julien Thierry
Kamalesh Babulal
Evgenii Shatokhin
Zhipeng Xie
Artem Savkov
chenzefeng
Artem Savkov
Joe Lawrence
Pawel Wieczorkiewicz