Rewrite the static local variable correlation logic. The algorithm now
traverses all the static locals in the original object rather than the
patched object, ensuring that each symbol in the original object has a
twin. It adds a new restriction that static local variables can't be
removed.
This adds support for the following:
- Multiple static locals with the same name in the same function
- Two separate static locals which happen to have the same numbered
suffix
- Static locals which are referenced by data sections
- CSWTCH and other static locals which are sometimes unused due to
sharing of their data sections
Fixes: #514
It turns out this is a more general issue which exists for more than
just CSWTCH symbols. The new static local handling code will handle it.
This reverts commit fd0c1bbe9c.
create-diff-object now checks if the original functions have fentry calls.
If an original function to be affected by the patch does not have the
fentry call, it cannot be patched. Error is reported in that case.
kpatch_create_mcount_sections() now also takes into account if a changed
or a new function has fentry call. If it does, mcount record is
generated for it as before. If a changed or a new function has no fentry
call, it is not an error in this case.
All this fixes the following issues.
1. If an original function has no fentry call (e.g. a "notrace" function)
but the patched function has it, the original function can not be
patched, but it would only be detected when applying the patch.
2. kpatch_create_mcount_sections() crashed if a patched function had no
relocation at all.
I observed such crashes when experimenting with a modified version of
the patch "tcp_cubic: better follow cubic curve after idle period" in
CentOS 7 x64.
Besides that, for a function with the first instruction starting with
0x0f, it would be incorrectly detemined that the function had fentry call.
The first bytes of the function would be overwritten in that case.
3. create-diff-object output an error if a new (an added) function had
no fentry call. This restriction is not necessary.
v2:
* Moved the check for fentry calls after the call to
kpatch_compare_correlated_elements() and before info about the original
ELF file is destroyed. The original symbols are now checked there (via
sym->twin) rather than the patched ones.
* Removed an excessive error check.
Signed-off-by: Evgenii Shatokhin <eshatokhin@odin.com>
Hard-coding the special section group sizes is unreliable. Instead,
determine them dynamically by finding the related struct definitions in
the DWARF metadata.
Fixes#517.
Fixes#523.
kpatch_verify_patchability can detect the change of .bss or .data or
.init section, but it must be processed before verify num_changed.
Otherwise, for example, if only .init section changed, it will fail
with 'no changed functions were found', but not 'unsupported section
change(s)'.
With this patch,
for .init section: .init section will not a bundled section, so if
the section changed, not sync the function status, kpatch_verify_patchability
will give 'changed section <secname> not selected for inclusion' and
'unsupported section change(s)' error.
for .bss/.data section: kpatch_verify_patchability will ensure not
including .data or .bss section, otherwise it will give 'data section
<secname> selected for inclusion' and 'unsupported section change(s)'
error.
Signed-off-by: Li Bin <huawei.libin@huawei.com>
If a static variable is a pointer, it has rela section.
Example:
static int *p = &a;
changed to:
static int *p = &b;
so its rela section has changed.
Then this change of data should be found and report error.
But if we don't correlate its rela section, we won't
find this change.
Signed-off-by: Zhou ChengMing <zhouchengming1@outlook.com>
The current WARN detection logic catches the majority of cases, but
there are still a lot of outliers which it doesn't catch (thanks, gcc).
I looked at a much larger sample of WARN calls and came up with a more
generic algorithm.
The _rs variable is used for printk ratelimiting, similar to __warned,
which makes it a logical candidate to be "special": don't correlate it,
yet don't mark a function as changed just because it references it.
When patching a kernel module, if we can't find a needed dynrela symbol,
we currently assume it's exported. However, it's also possible that
it's provided by another .o in the patch module. Add support for that.
Fixes#445.
Currently unbundled section references are only replaced if the start of
the symbol is referenced. It's also useful to support replacement of
references which point to inside the symbol.
Improve the static local variable correlation logic, for the case where
a static local is used by multiple functions. For each usage of the
variable, look for a corresponding usage in the base object. If we find
at least one matching usage, consider it a twin.
Allow static locals to be used by two functions. This is possible if
the static's containing function is inlined. We only need to find one
of them to do the correlation.
The __func__ static local variable should be deemed "special", because
it doesn't need to be correlated and should be included when needed by
an include function.
I don't have a test case for F20, but this fixes the following types of
issues when doing a full-tree recompile on RHEL 7:
ERROR: cifssmb.o: object size mismatch: __func__.49322
ERROR: btmrvl_main.o: kpatch_correlate_static_local_variables: 982: static local variable __func__.44657 not used
ERROR: iwch_qp.o: .rodata.__func__.46024 section header details differ
When patching a shared header file, don't spam the user with hundreds of
lines of "no changed functions" messages. We expect the user to be
proactive with verifying that the right functions are being patched
anyway, so this message isn't strictly necessary.
The "descriptor" static local variables and their containing __verbose
section are used for dynamic debug printks. They should be considered
as special static local variable symbols because they have the same
requirements: they should never be correlated and they should only be
included if referenced by an included function.
The fixup_group_size() function assumes that all .fixup rela groups end
with a jmpq instruction. That assumption turns out to be false when you
take into account the ____kvm_handle_fault_on_reboot() macro which is
used by kvm.
This is a new, more reliable method. It turns out that each .fixup
group is referenced by the __ex_table section. The new algorithm goes
through the __ex_table relas to figure out the size of each .fixup
group.
Also the .fixup section is now processed before __ex_table, because it
needs to access the original __ex_table relas before the unused ones
have been stripped.
Fixes the following error:
ERROR: vmx.o: fixup_group_size: 1554: can't find jump instruction in .fixup section
Currently we're checking for several special cases when deciding whether
to convert unbundled section references to their corresponding symbol
references. We do it for all unbundled text sections as well as three
specific data sections.
There's no reason I can think of for why we shouldn't just do it for
_all_ unbundled sections.
There are two distinct usages of "objname" as a variable name:
- the parent object being patched (e.g. vmlinux)
- the child object being analyzed (e.g. meminfo.o)
The name of the global objname variable conflicts with several
functions' usage of a local objname variable, resulting in some error
messages of e.g., "ERROR: vmlinux:" instead of "ERROR: meminfo.o:".
Rename the global objname variable to childobj.
There's no need to process special sections if we're returning due to no
functions changing.
Also this means we don't have to deal with extra-special usage of the
.fixup section (here's looking at you arch/x86/lib/copy_user_64.S -- we
can't patch functions in .S files anyway).
The special sections should be processed after all the other inclusion
logic has run, so that should_keep_rela_group() can work properly.
Otherwise it might remove a needed rela group from a special section.
Fix the mangled function strcmp so that it compares all of the string
except for the numbered parts. foo.isra.35 should match foo.isra.1, but
not foo.isra.35.part.36.
Fixes#352.
It's possible for a static local variable's data section to have
a relocation which refers to the variable symbol itself. Fix the logic
which searches for the user of a static local variable by only looking
in text sections (i.e. functions).
Fixes#411.
This fixes a seg fault in the test suite caused by a debug section
referencing an un-included unbundled symbol (though its section was
included). The symbol was a __warned symbol and the section was
.data.unlikely.
For debug sections, there is no need to replace section references with
symbols because we don't compare debug sections.
Add support for the __key and __warned "special" static local variables.
I'm calling them that for lack of a better term, analagous to the
kernel's special sections that we have to deal with.
__warned: Used by WARN_ONCE et al as an indicator as to whether a
message has already been printed. I think it makes sense (and is much
easier) to reset this counter for a given function when replacing the
function, since the user may expect the new function to warn again.
__key: Used by lockdep as an identifier for a given lock initialization
code path (see http://lwn.net/Articles/185666/ for more info). I think
it makes sense (and is much easier) to create a new key for a given
function when replacing the function, because the locking semantics may
have changed, so it makes sense for lockdep to use a new key to validate
the new locking behavior.
So for both __warned and __key static variables, the new version of the
variable should be used when referenced by an included function.
Made the following changes to support these special variables:
- Ignore their suffixes when comparing them in rela_equal, so that gcc
renaming them will not result in a function being marked as changed
just because it referenced a renamed static local
- Don't ever correlate them, so that their new versions will be included
if a changed or new function uses their corresponding symbols
Fixes#402.
This adds support for shadow variables, which allow you to add new
"shadow" fields to existing data structures.
To allow patches to call the shadow functions in the core module, I had
to add a funky hack to use --warn-unresolved-symbols when linking, which
allows the patched vmlinux to link with the missing symbols. I also
added greps to the log file to ensure that only unresolved symbols to
kpatch_shadow_* are allowed. We can remove this hack once the core
module gets moved into the kernel tree.
Fixes#314.
In the case that a new global symbol is defined in a file but not used
by a changed function, the symbol will currently not be included.
However, since it is global, another file in the patch my reference it,
but it will not be there.
This commit includes new global symbols so that they may be referenced
by changes in other files within the same patch.
Signed-off-by: Seth Jennings <sjenning@redhat.com>
WARN_ON_ONCE places the __warned static local variable in the
.data.unlikely section, so it's not bundled (i.e. ignored by the
-fdata-sections gcc flag). There's no reason why we can't rename
unbundled symbols, so add support for them.
Fixes#394.
If a patch adds a new function in foo.c, and calls that function from
bar.c, currently it fails with something like:
kpatch_create_dynamic_rela_sections: 2115: lookup_global_symbol failed for tpe_allow_file, needed for .text.do_mmap_pgoff
This (crudely) fixes the issue by assuming that if we can't find the
global symbol in the original vmlinux, that it will be provided by
another object in the patch module. If that assumption is incorrect,
the module will fail to load due to the missing symbol dependency.
A (perhaps) better way to fix this is to search for the symbol in the
patched version of the vmlinux. But I think this approach is good
enough, for now at least.
Fixes#388.
The naming of variables in this function is confusing, and really threw
me for a loop: sec is first used as an iterator, then sec is reused to
point to the dynrela section, then sec2 is used as another iterator.
Instead make sec the iterator for both loops and dynsec the dynrela
section pointer.
When a function foo.isra.1 has a switch statement, it might have a
corresponding .rodata.foo.isra.1 section (in addition to its
.text.foo.isra.1 section). If so, rename that section too.
Otherwise kpatch-build will get confused when comparing the function's
relas which reference the .rodata section, and will mark the function's
rela section as changed because the rela symbol names differ.
I found this bug when trying to build the patch from upstream Linux
commit a3c54931. Unfortunately this issue is already fixed on F20 and I
wasn't able to come up with a similarly failing test case for the
integration test suite.
To reduce redundancy, remove/change the old_offset fields in the
kpatch_func and kpatch_patch_func structs to just old_addr. Since
old_offset is being used as a placeholder for old_addr, might as well
consolidate it to just one variable.
In kpatch_create_dynamic_rela_sections() the dest field is filled in
with either the function symbol or the section symbol that contains the
function depending on whether or not the sym field of the base section
is NULL or not (around line 2153).
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.
However we weren't unbundling the stripped symbol from the section.
This resulted in the sym field pointing to the null symbol (index 0),
corrupting the dynrelas rela section.
Before:
Relocation section [14] '.rela.kpatch.dynrelas' for section [13] '.kpatch.dynrelas' at offset 0x8b8 contains 6 entries:
Offset Type Value Addend Name
000000000000000000 X86_64_64 000000000000000000 +9
0x0000000000000018 X86_64_64 000000000000000000 +8 .kpatch.strings
0x0000000000000020 X86_64_64 000000000000000000 +0 .kpatch.strings
0x0000000000000030 X86_64_64 000000000000000000 +9
0x0000000000000048 X86_64_64 000000000000000000 +8 .kpatch.strings
0x0000000000000050 X86_64_64 000000000000000000 +0 .kpatch.strings
This commit unbundles the stripped symbol from the section so that the
section symbol is used in the dynrelas rela section.
After:
Relocation section [14] '.rela.kpatch.dynrelas' for section [13] '.kpatch.dynrelas' at offset 0x8b8 contains 6 entries:
Offset Type Value Addend Name
000000000000000000 X86_64_64 000000000000000000 +9 .text.kpatch_load_aio_max_nr
0x0000000000000018 X86_64_64 000000000000000000 +8 .kpatch.strings
0x0000000000000020 X86_64_64 000000000000000000 +0 .kpatch.strings
0x0000000000000030 X86_64_64 000000000000000000 +9 .text.kpatch_unload_aio_max_nr
0x0000000000000048 X86_64_64 000000000000000000 +8 .kpatch.strings
0x0000000000000050 X86_64_64 000000000000000000 +0 .kpatch.strings
Signed-off-by: Seth Jennings <sjenning@redhat.com>
GROUP section are rare and are a mechanism in the ELF to indicated that
certain groups of section must be included or excluded (stripped)
together.
It is valid to have more than one of these section with the same
".group" name. This currently messes up the section correlation code
with correlates based solely on name.
This commit adds additional correlation criteria for GROUP sections;
namely, the section content must be the same. Changing of groups
sections (i.e. reindexing of the section indexes the GROUP section
includes in their section data) is not supported and will result in a
"new/changed section not included" error.
Signed-off-by: Seth Jennings <sjenning@redhat.com>
Fixes the following error:
kpatch_correlate_static_local_variables: 850: found another static local variable matching __func__.49968 in patched .rela__verbose
Detect .rodata.* bundled sections so that .rodata.__func__.* relocation
references can be converted to refer to their corresponding object
symbols.
Fixes the following error:
kpatch_correlate_static_local_variables: 830: static local variable __func__.49968 not used
The __verbose section stores several static local structs named
"descriptor". These structs contain information related to dynamic
debugging printks and are specific to the patched object, so they
shouldn't be correlated with their base object counterparts.
Fixes the following error:
kpatch_correlate_static_local_variables: 830: static local variable descriptor.49967 not used
Right now, the test patch unnecessarily includes hrtimer_nanosleep()
because the call to do_nanosleep() generates a rela the references the
unbundled .sched.text section. This section symbol is not currently
replaced by kpatch_replace_sections_syms() as it only replaces bundled
sections symbols.
This commit adds logic to kpatch_replace_sections_syms() to replace
unbundled section symbols as well by scanning the symbol table for
symbols that start at the rela entry's offset within the matching
section.
This allows for properly rela section correlation when the functions
have moved from an unbundled section to a bundled section.
Signed-off-by: Seth Jennings <sjenning@redhat.com>
This macro is for ignoring sections that may change as a side effect of
another change or might be a non-bundlable section; that is one that
does not honor -ffunction-section and create a one-to-one relation from
function symbol to section.
Signed-off-by: Seth Jennings <sjenning@redhat.com>
Right now, in the case that the mcount sections have changed, we get a
"changed section not included" error on them. Since we rebuild them
from scratch, just mark them as SAME even if they are different so that
we don't cause an error.
Signed-off-by: Seth Jennings <sjenning@redhat.com>
This sh_link line currently has a bug with both operands being sec1; the
second should be sec2. However the bug is masking a logical flaw; that
is that the sh_link is the index of either the symtab or the strtab and
that can change if sections have been added or removed by the patch.
This commit removes the comparison.
Signed-off-by: Seth Jennings <sjenning@redhat.com>
gcc renames static local variables by appending a period and a number.
For example, __key could be renamed to __key.31452. Unfortunately this
number can arbitrarily change. Try to rename the patched version of the
symbol to match the base version and then correlate them.
Fixes#313.
