It was observed by Evgenii Shatokhin in PR#755, that when the RCU
callback was called on the patched function, from unloaded livepatch
module triggered a kernel crash.
This patch implements the approach on PowerPC outlined in PR#755.
With -mcmodel=large, like any other data, function pointers are also
loaded relative to the current TOC base and are populated as
relocation entries in .toc section. Every function passing a function
pointer as the argument need to load the function address through
.toc section + offset. Convert such .toc + offset relocation into
a dynamic rela, which resolves to original function address, during
module load.
Also move the comment related to nested function check, into
may_need_dynrela().
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Evgenii Shatokhin <eshatokhin@virtuozzo.com>
Cc: Joe Lawrence <jdl1291@gmail.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
A kernel crash happened in __do_softirq() in very rare cases when the
binary patch created from mainline commit be82485fbcbb
("netlink: fix an use-after-free issue for nlk groups") was unloaded.
Investigation has shown that the kernel tried to execute an RCU
callback, deferred_put_nlk_sk(), defined in the patch module after the
module had been unloaded.
The callback was set by the patched variant of netlink_release() and
the address of the patched deferred_put_nlk_sk() was used, rather than
the address of the original function.
Similar problems occur with workqueue functions as well.
As suggested in
https://github.com/dynup/kpatch/pull/755#issuecomment-344135224,
create-diff-object was modified so that the addresses of the original
functions were used in such situations, at least for x86 systems. A
similar fix for PowerPC was added as well.
Changes in v4:
* '#ifdef __x86_64__' was removed. It is not actually needed right now
because the constants for relocation types are different on different
architectures.
Changes in v3:
* Minor refactoring and a comment explaining what this all is about.
Quite lengthy, but the dynrela-related code is really far from obvious.
Changes in v2:
* Handle the nested functions the same way as before, because they are
unlikely to be used as asynchronous callbacks. Example: cmp() in
bch_cache_show() from drivers/md/bcache/sysfs.c in the kernel 4.4.
As the nested functions are local to the functions they are defined in,
the compiler names them in a similar way to static locals: <name>.<number>.
Currently, we filter out all functions with '.' in their names. If there
are any asynchronous callbacks in the kernel that have a dot in their
names too, they could be handled in the future patches. It is unclear
though, if the callbacks with such names can appear in the kernel.
Signed-off-by: Evgenii Shatokhin <eshatokhin@virtuozzo.com>
for-next branch of kbuild repo contains a "kbuild: rename built-in.o to
built-in.a" which renames all built-in.o instances. Filter on both .o
and .a in kpatch-gcc/kpatch-build to be prepared for this change.
Fixes#800.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
With kernel commit b1fca27d384 ("kernel debug: support resetting
WARN*_ONCE") the *_ONCE warnings are placed .data.once section.
Including .data.once section is valid, so add an check in
kpatch_verify_patchability() while checking for included invalid
sections.
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Symbols with R_PPC64_REL24 relocation type are functions and it's
currently assumed that all functions are replaced with their respective
section symbols.
There are function whose reference are not straight forward section
symbol but section + offset. These function replacement should be
handled more like bundled sections. Remove the check, which imposes
the inital assumption.
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
.toc section entries are mostly place holder for relocation entries,
specified in .rela.toc section. Sometimes, .toc section may have
constants as entries. These constants are not reference to any symbols,
but plain instructions mostly due to some arthimetics in the functions
referring them.
They are referred by the functions like normal .toc entries, these
entries can not be resolved to any symbols. This patch creates a list
of constants if available for .toc sections and compares them in
rela_equal() to ensure their is no mismatch in the generated constants
for original and patched .o files.
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
GCC 7.2.1 complains about the usage of the PPC64_LOCAL_ENTRY_OFFSET
macro:
create-diff-object.c: In function ‘is_gcc6_localentry_bundled_sym’:
create-diff-object.c:119:83: error: ‘<<’ in boolean context, did you mean ‘<’ ? [-Werror=int-in-bool-context]
(((1 << (((other) & STO_PPC64_LOCAL_MASK) >> STO_PPC64_LOCAL_BIT)) >> 2) << 2)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
create-diff-object.c:140:10: note: in expansion of macro ‘PPC64_LOCAL_ENTRY_OFFSET’
return (PPC64_LOCAL_ENTRY_OFFSET(sym->sym.st_other) &&
^~~~~~~~~~~~~~~~~~~~~~~~
Fix it by explicitly treating the macro as an integer instead of a bool.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Sometimes git doesn't see that the patches have been reverted, if that
happens during ./scripts/setlocalversion call the resulting patch module
is built with a wrong vermagic because the tree is still considered
dirty.
Fix by moving git update-index call into remove_patches function so that
it is called every time the patches are reverted, not only on cleanup.
Signed-off-by: Artem Savkov <asavkov@redhat.com>
Found in the scope of https://github.com/dynup/kpatch/pull/755 but not
related to the main problem discussed there.
kpatch_create_patches_sections() and kpatch_create_intermediate_sections()
used 'hint' in error messages.
However, the string 'hint' refers to is owned by 'kelf_base' and is
freed before kpatch_create_*_sections() are called. As a result, if
these functions try to output errors and print 'hint',
create-diff-object will crash.
As suggested in the mentioned PR, 'hint' is actually no longer needed at
that stage, so I have removed it from kpatch_create_*_sections().
By specifying -d, --debug multiple times, the following additional
debug modes can be enabled:
-d -d: Writes everything that is written to the logfile also to
stdout.
-d -d -d: Same as '-d -d' plus sets 'xtrace' in kpatch-build.
-d -d -d -d: Same as '-d -d -d' plus sets 'xtrace' in kpatch-gcc.
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
Add a logger funcition that can be used to log to both stdout and the
logfile or only to the logfile. This is needed for subsequent patches
where we introduce an alternate debug mode.
Since we're piping to a logger now, we need to set 'pipefail' otherwise
the return status of such a pipeline is always 0 (the exit status of the
logger) and we won't catch any errors.
From the bash manpage:
The return status of a pipeline is the exit status of the last command,
unless the pipefail option is enabled
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
This is in response to an upstream discussion for the following patch:
https://lkml.kernel.org/r/1508217523-18885-1-git-send-email-kamalesh@linux.vnet.ibm.com
This should hopefully make it a lot easier for the ppc64le kernel module
code to support klp relocations.
The gcc-common.h and gcc-generate-rtl-pass.h header files are copied
from the upstream Linux source tree.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
When searching for 'Linux version ...' in vmlinux, stop after the first
match so that we don't keep reading a potentially huge file.
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
The current checks never fail, because the first grep in the pipeline
doesn't write anything to stdout.
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
This can be used for building a kpatch module for a non-running
kernel. Note that the correct kernel and debug packages still need
to be installed.
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
When creating .kpatch.relocations, there's no reason to convert the
relocation destinations to symbols. In fact, it's actively harmful
because it makes it harder for create-klp-module to deal with the GCC 6+
8-byte localentry gap.
This also fixes a regression which was introduced in 5888f316e6, which
broke ppc64le relocations.
Fixes#754.
Fixes: 5888f316e6 ("create-klp-module: support unbundled symbols")
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
kpatch_relocation's 'dest' addend and 'offset' fields are redundant. In
fact, the 'offset' field isn't always accurate because it doesn't have a
relocation, so its value doesn't adjust when multiple .o files are
combined. Just use the 'dest' addend instead.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
kpatch_replace_sections_syms() assumes that all bundled symbols start at
section offset zero. With ppc64le and GCC 6+, that assumption is no
longer accurate. When replacing a rela symbol section with its
corresponding symbol, adjust the addend as necessary.
Also, with this fix in place, the workaround in
create_klp_relasecs_and_syms() can be removed.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
On ppc64le, adding a printk to total_mapping_size() caused it to change
from non-localentry to localentry, presumably because it was no longer a
leaf function. With GCC 6, a localentry function is offset by 8 in the
section, so different st_values are ok.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
The STT_FUNC and SHN_UNDEF checks aren't needed because they're already
implied by the localentry check.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
is_localentry_sym() isn't quite the right name, because it also checks
for the 8-byte gap introduced by GCC 6, and also checks that the
function is otherwise at the beginning of the section.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Replace stray spaces with tabs, except in the usage output where tabs
don't make much sense.
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
This was introduced in commit 5352d8b01a ('build objects in separate
directory to fix caching') but is no longer necessary.
Fixes: 2e99d6b7a4 ('kpatch-build: build the kernel in ~/.kpatch/src again')
Signed-off-by: Juerg Haefliger <juerg.haefliger@canonical.com>
Fix the version checks for when we enable CONFIG_LIVEPATCH on RHEL. It
will be based on the latest upstream code.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
The paravirt_patch_site struct has 12 bytes of data and 4 bytes of
padding, for a total of 16 bytes. However, when laying out the structs
in the .parainstructions section, the vmlinux script only aligns before
each struct's data, not after. So the last entry doesn't have the
4-byte padding, which breaks kpatch_regenerate_special_section()'s
assumption of a 16-byte struct, resulting in a memcpy past the end of
the section.
Fixes#747.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Replace find * with find ./* to prevent treating files with dashes as
options. The leading ./ is later used in comparisons and thus must be
removed before that.
Found by shellcheck.
gcc --version varies too much for sane comparisons with vmlinux's
.comment section. Therefore compile a test file and compare its .comment
section.
Also fix gcc 4.8 check which used a lexicographically comparison which
will break for gcc versions >= 10. Instead check for the required
compiler options.
Closes#565.
- Replace grep | wc -l with grep -c.
- Use find -print0 and xargs -0 to handle non-alphanumeric filenames
(shouldn't be an issue for us but it's good practice).
- Replace expr with $(( )).
Found by shellcheck.
The double quotes are confusing as they don't quote $UBUNTU_ABI and thus
have no real effect. As $UBUNTU_ABI is a number simply remove them and
put $UBUNTU_ABI into the surrounding quotes.
Found by shellcheck.
- Replace echo $(cmd) with just cmd.
- Replace $@ inside quotes with $*.
- Always die if cd fails.
- Ensure rm -rf "$TEMPDIR"/* never expands to rm -rf /*.
Found by shellcheck.
Without proper quoting kpatch fails if the argument contains spaces, the
other scripts might be affected as well.
Not all new quotes are strictly necessary but they were added for
consistency with the existing code and to prevent copy & paste errors in
the future.
There's one conversion which is not straight-forward:
- grepname=$grepname\\\.o
+ grepname="$grepname\.o"
There are different quoting rules with and without the double quotes.
Valgrind complains about an uninitialized variable in
create-klp-module.c:
==4412== Conditional jump or move depends on uninitialised value(s)
==4412== at 0x402846: main (create-klp-module.c:497)
This warning refers to main()'s struct arguments stack variable,
precisely its .no_klp_arch member. Initialize the entire structure to
zero to avoid complaint.
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
Valgrind complains about uninitialized bytes passed to pwrite64(buf)
from kpatch_write_output_elf()'s call to elf_update():
==32378== Syscall param pwrite64(buf) points to uninitialised byte(s)
==32378== at 0x5141A03: __pwrite_nocancel (in /usr/lib64/libc-2.23.so)
==32378== by 0x4E46846: ??? (in /usr/lib64/libelf-0.168.so)
==32378== by 0x4E42B88: elf_update (in /usr/lib64/libelf-0.168.so)
==32378== by 0x40C57A: kpatch_write_output_elf (kpatch-elf.c:895)
==32378== by 0x40926F: main (create-diff-object.c:2851)
==32378== Address 0x28d52300 is 0 bytes inside a block of size 56 alloc'd
==32378== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
==32378== by 0x40B86A: create_section_pair (kpatch-elf.c:707)
==32378== by 0x406CAE: kpatch_create_patches_sections (create-diff-object.c:2109)
==32378== by 0x4090C5: main (create-diff-object.c:2815)
These are fields which we don't need to populate (like a
funcs[index].new_addr value that will be filled by relocation). The
easiest way to appease valgrind and not clutter the code is to just
zero-out this entire buffer on allocation.
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
kpatch-elf.c is used by binaries other than create-diff-object, but
create-diff-object is the only one that cares about "bundling". Move
the bundling to create-diff-object.
Fixes#700.
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
The create_klp_relasecs_and_syms() function assumes that all dest
symbols are bundled, i.e. each symbol is located at offset 0 in its own
section.
However that may not always be the case. Unbundled symbols can occur,
for example, when combining two .o files which have the same bundled
symbol. They will be combined into the same section and will no longer
be considered "bundled".
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
The create_dynamic_rela_sections() function assumes that all dest
symbols are bundled, i.e. each symbol is located at offset 0 in its own
section.
However that may not always be the case. Unbundled symbols can occur,
for example, when combining two .o files which have the same bundled
symbol. They will be combined into the same section and will no longer
be considered "bundled".
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Use kpatch-<modname>.ko or livepatch-<modname>.ko depending on the type
of module we're building.
Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
