* [PATCH] Livepatch: Support local/global symbols.
@ 2017-06-20 2:47 Konrad Rzeszutek Wilk
2017-06-20 2:47 ` [PATCH v1 1/3] livepatch: Add local and global symbol resolution Konrad Rzeszutek Wilk
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-06-20 2:47 UTC (permalink / raw)
To: xen-devel, ian.jackson, ross.lagerwall; +Cc: andrew.cooper3, jbeulich
Heya,
Please see the following two patches:
[PATCH v1 1/3] livepatch: Add local and global symbol resolution.
[PATCH v1 2/3] livepatch: Add xen_local_symbols test-case
which depend on "xen/test/Makefile: Fix clean target, broken by pattern rule"
to install properly.
There is also an OSSTEST test-case:
[PATCH v1 OSSTEST 3/3] ts-livepatch: Expand testcase to include global/local symbols
which expands the regression testing (and it is smart enough to not do the
test-case is the test-case file does not exist).
The Xen diff stat is as follow:
xen/common/livepatch.c | 21 +++++++++-----
xen/include/xen/livepatch.h | 3 +-
xen/include/xen/livepatch_elf.h | 7 +++++
xen/test/livepatch/Makefile | 10 ++++++-
xen/test/livepatch/xen_local_symbols.c | 52 ++++++++++++++++++++++++++++++++++
5 files changed, 84 insertions(+), 9 deletions(-)
Konrad Rzeszutek Wilk (2):
livepatch: Add local and global symbol resolution.
livepatch: Add xen_local_symbols test-case
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v1 1/3] livepatch: Add local and global symbol resolution.
2017-06-20 2:47 [PATCH] Livepatch: Support local/global symbols Konrad Rzeszutek Wilk
@ 2017-06-20 2:47 ` Konrad Rzeszutek Wilk
2017-06-20 7:51 ` Jan Beulich
2017-06-20 2:47 ` [PATCH v1 2/3] livepatch: Add xen_local_symbols test-case Konrad Rzeszutek Wilk
2017-06-20 2:47 ` [PATCH v1 OSSTEST 3/3] ts-livepatch: Expand testcase to include global/local symbols Konrad Rzeszutek Wilk
2 siblings, 1 reply; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-06-20 2:47 UTC (permalink / raw)
To: xen-devel, ian.jackson, ross.lagerwall
Cc: andrew.cooper3, jbeulich, Konrad Rzeszutek Wilk
This way we can load livepatches with symbol names that
are the same as long as they are local ('static').
The use case here is to replace an existing livepatch
with a newer one - and one which has the same local symbols.
Without this patch we get:
livepatch.c:819: livepatch: xen_local_symbols: duplicate new symbol: revert_hook
when loading the new livepatch (before doing the replace).
This due to livepatch assuming that all symbols are all
global. With this patch:
readelf --symbols xen_hello_world.livepatch
File: xen_hello_world.livepatch
Symbol table '.symtab' contains 55 entries:
Num: Value Size Type Bind Vis Ndx Name
..snip..
34: 0000000000000000 4 OBJECT LOCAL DEFAULT 25 cnt
35: 000000000000000a 8 OBJECT LOCAL DEFAULT 7 __func__.4654
36: 0000000000000065 23 FUNC LOCAL DEFAULT 2 revert_hook
37: 000000000000007c 23 FUNC LOCAL DEFAULT 2 apply_hook
38: 0000000000000093 54 FUNC LOCAL DEFAULT 2 check_fnc
..snip..
47: 0000000000000000 54 FUNC GLOBAL HIDDEN 2 xen_hello_world
48: 0000000000000000 0 NOTYPE GLOBAL HIDDEN UND xen_extra_version
..snip..
52: 0000000000000000 0 NOTYPE GLOBAL HIDDEN UND printk
53: 0000000000000000 64 OBJECT GLOBAL HIDDEN 23 livepatch_xen_hello_world
All the 'GLOBAL' have to be unique per livepatch. But the
'LOCAL' can all be the same which means the semantic of 'static'
on functions and data variables is the right one.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
xen/common/livepatch.c | 21 ++++++++++++++-------
xen/include/xen/livepatch.h | 3 ++-
xen/include/xen/livepatch_elf.h | 7 +++++++
3 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index df67a1a..2a86218 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -181,7 +181,10 @@ unsigned long livepatch_symbols_lookup_by_name(const char *symname)
if ( !data->symtab[i].new_symbol )
continue;
- if ( !strcmp(data->symtab[i].name, symname) )
+ if ( strcmp(data->symtab[i].name, symname) )
+ continue;
+
+ if ( data->symtab[i].global_symbol )
return data->symtab[i].value;
}
}
@@ -791,6 +794,7 @@ static int build_symbol_table(struct payload *payload,
symtab[nsyms].size = elf->sym[i].sym->st_size;
symtab[nsyms].value = elf->sym[i].sym->st_value;
symtab[nsyms].new_symbol = 0; /* May be overwritten below. */
+ symtab[nsyms].global_symbol = livepatch_sym_is_global(elf->sym[i].sym);
strtab_len += strlcpy(strtab + strtab_len, elf->sym[i].name,
KSYM_NAME_LEN) + 1;
nsyms++;
@@ -815,21 +819,24 @@ static int build_symbol_table(struct payload *payload,
if ( symbols_lookup_by_name(symtab[i].name) ||
livepatch_symbols_lookup_by_name(symtab[i].name) )
{
- dprintk(XENLOG_ERR, LIVEPATCH "%s: duplicate new symbol: %s\n",
- elf->name, symtab[i].name);
+ dprintk(XENLOG_ERR, LIVEPATCH "%s: duplicate new %s symbol: %s\n",
+ elf->name, symtab[i].global_symbol ? "global" : "local",
+ symtab[i].name);
xfree(symtab);
xfree(strtab);
return -EEXIST;
}
symtab[i].new_symbol = 1;
- dprintk(XENLOG_DEBUG, LIVEPATCH "%s: new symbol %s\n",
- elf->name, symtab[i].name);
+ dprintk(XENLOG_DEBUG, LIVEPATCH "%s: new %s symbol %s\n",
+ elf->name, symtab[i].global_symbol ? "global" : "local",
+ symtab[i].name);
}
else
{
/* new_symbol is not set. */
- dprintk(XENLOG_DEBUG, LIVEPATCH "%s: overriding symbol %s\n",
- elf->name, symtab[i].name);
+ dprintk(XENLOG_DEBUG, LIVEPATCH "%s: overriding %s symbol %s\n",
+ elf->name, symtab[i].global_symbol ? "global" : "local",
+ symtab[i].name);
}
}
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 98ec012..fccff94 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -38,7 +38,8 @@ struct livepatch_symbol {
const char *name;
unsigned long value;
unsigned int size;
- bool_t new_symbol;
+ unsigned int new_symbol:1;
+ unsigned int global_symbol:1;
};
int livepatch_op(struct xen_sysctl_livepatch_op *);
diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
index 9ad499e..870c4bc 100644
--- a/xen/include/xen/livepatch_elf.h
+++ b/xen/include/xen/livepatch_elf.h
@@ -50,6 +50,13 @@ static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec)
{
return !(sec->sh_flags & SHF_ALLOC) || sec->sh_size == 0;
}
+
+static inline bool livepatch_sym_is_global(const Elf_Sym *sym)
+{
+ return ((ELF_ST_BIND(sym->st_info) & STB_GLOBAL) &&
+ (sym->st_shndx != SHN_UNDEF));
+}
+
#endif /* __XEN_LIVEPATCH_ELF_H__ */
/*
--
2.9.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v1 2/3] livepatch: Add xen_local_symbols test-case
2017-06-20 2:47 [PATCH] Livepatch: Support local/global symbols Konrad Rzeszutek Wilk
2017-06-20 2:47 ` [PATCH v1 1/3] livepatch: Add local and global symbol resolution Konrad Rzeszutek Wilk
@ 2017-06-20 2:47 ` Konrad Rzeszutek Wilk
2017-06-20 2:47 ` [PATCH v1 OSSTEST 3/3] ts-livepatch: Expand testcase to include global/local symbols Konrad Rzeszutek Wilk
2 siblings, 0 replies; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-06-20 2:47 UTC (permalink / raw)
To: xen-devel, ian.jackson, ross.lagerwall
Cc: andrew.cooper3, jbeulich, Konrad Rzeszutek Wilk
To exercise the local/global visibility.
With "livepatch: Add local and global symbol resolution."
we can load both xen_hello_world and xen_local_symbols
without having to worry about:
-bash-4.1# xen-livepatch load xen_hello_world.livepatch
Uploading xen_hello_world.livepatch... completed
Applying xen_hello_world... completed
-bash-4.1# xen-livepatch list
ID | status
----------------------------------------+------------
xen_hello_world | APPLIED
-bash-4.1# xen-livepatch upload xen_local_symbols xen_local_symbols.livepatch
Uploading xen_local_symbols.livepatch... failed
(XEN) livepatch.c:819: livepatch: xen_local_symbols: duplicate new symbol: revert_hook
In fact you will see:
livepatch: xen_hello_world: new local symbol revert_hook
livepatch: xen_hello_world: new local symbol apply_hook
livepatch: xen_hello_world: new local symbol check_fnc
livepatch: xen_hello_world: new local symbol hello_world_patch_this_fnc
...
livepatch: xen_local_symbols: new local symbol revert_hook
livepatch: xen_local_symbols: new local symbol apply_hook
livepatch: xen_local_symbols: new local symbol hello_world_patch_this_fnc
..
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
xen/test/livepatch/Makefile | 10 ++++++-
xen/test/livepatch/xen_local_symbols.c | 52 ++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+), 1 deletion(-)
create mode 100644 xen/test/livepatch/xen_local_symbols.c
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 6831383d..b9ad291 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -19,11 +19,13 @@ LIVEPATCH := xen_hello_world.livepatch
LIVEPATCH_BYE := xen_bye_world.livepatch
LIVEPATCH_REPLACE := xen_replace_world.livepatch
LIVEPATCH_NOP := xen_nop.livepatch
+LIVEPATCH_LOCAL := xen_local_symbols.livepatch
LIVEPATCHES += $(LIVEPATCH)
LIVEPATCHES += $(LIVEPATCH_BYE)
LIVEPATCHES += $(LIVEPATCH_REPLACE)
LIVEPATCHES += $(LIVEPATCH_NOP)
+LIVEPATCHES += $(LIVEPATCH_LOCAL)
LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
@@ -107,5 +109,11 @@ xen_nop.o: config.h
$(LIVEPATCH_NOP): xen_nop.o note.o
$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^
+xen_local_symbols.o: config.h
+
+.PHONY: $(LIVEPATCH_LOCAL)
+$(LIVEPATCH_LOCAL): xen_hello_world_func.o xen_local_symbols.o note.o
+ $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_LOCAL) $^
+
.PHONY: livepatch
-livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP)
+livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_LOCAL)
diff --git a/xen/test/livepatch/xen_local_symbols.c b/xen/test/livepatch/xen_local_symbols.c
new file mode 100644
index 0000000..eb01b69
--- /dev/null
+++ b/xen/test/livepatch/xen_local_symbols.c
@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2017 Oracle and/or its affiliates. All rights reserved.
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+
+/* Same name as in xen_hello_world */
+static const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+/*
+ * The hooks are static here (LOCAL) and also in xen_hello_world.c
+ * and their name is exactly the same.
+ */
+static void apply_hook(void)
+{
+ printk(KERN_DEBUG "local_symbols: Hook executing.\n");
+}
+
+static void revert_hook(void)
+{
+ printk(KERN_DEBUG "local_symbols: Hook unloaded.\n");
+}
+
+LIVEPATCH_LOAD_HOOK(apply_hook);
+LIVEPATCH_UNLOAD_HOOK(revert_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_local_symbols = {
+ .version = LIVEPATCH_PAYLOAD_VERSION,
+ .name = hello_world_patch_this_fnc,
+ .new_addr = xen_hello_world,
+ .old_addr = xen_extra_version,
+ .new_size = NEW_CODE_SZ,
+ .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.9.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v1 OSSTEST 3/3] ts-livepatch: Expand testcase to include global/local symbols
2017-06-20 2:47 [PATCH] Livepatch: Support local/global symbols Konrad Rzeszutek Wilk
2017-06-20 2:47 ` [PATCH v1 1/3] livepatch: Add local and global symbol resolution Konrad Rzeszutek Wilk
2017-06-20 2:47 ` [PATCH v1 2/3] livepatch: Add xen_local_symbols test-case Konrad Rzeszutek Wilk
@ 2017-06-20 2:47 ` Konrad Rzeszutek Wilk
2017-06-20 15:18 ` Ian Jackson
2 siblings, 1 reply; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-06-20 2:47 UTC (permalink / raw)
To: xen-devel, ian.jackson, ross.lagerwall
Cc: andrew.cooper3, jbeulich, Konrad Rzeszutek Wilk
testing. The test is to verify that the local symbols
of payloads are ignored during loading.
See Xen's patch "livepatch: Add local and global symbol resolution."
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
ts-livepatch-run | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/ts-livepatch-run b/ts-livepatch-run
index f119458..1eaceff 100755
--- a/ts-livepatch-run
+++ b/ts-livepatch-run
@@ -27,6 +27,7 @@ my @livepatch_files = qw(xen_hello_world.livepatch
xen_bye_world.livepatch
xen_nop.livepatch);
+my $livepatch_dir="/usr/lib/debug/xen-livepatch";
my $xen_extra_info;
my $xen_minor_ver;
@@ -128,6 +129,24 @@ my @livepatch_tests = (
{ C => "xen-livepatch unload xen_nop" },
);
+my @livepatch_tests_symbols = (
+ { C => "xen-livepatch load xen_hello_world.livepatch" },
+ { C => "xen-livepatch list",
+ OutputCheck => sub { m/xen_hello_world/ } },
+ { C => "xl info",
+ OutputCheck => \&check_for_hello_world },
+ # This would normally fail as xen_local_symbols has the same local
+ # symbols as xen_hello_world.livepatch but with
+ # "livepatch: Add local and global symbol resolution." it works.
+ { C => "xen-livepatch upload xen_local_symbols xen_local_symbols.livepatch" },
+ { C => "xen-livepatch replace xen_local_symbols" },
+ { C => "xl info",
+ OutputCheck => \&check_for_hello_world },
+ { C => "xen-livepatch revert xen_local_symbols" },
+ { C => "xen-livepatch unload xen_hello_world" },
+ { C => "xen-livepatch unload xen_local_symbols" },
+ );
+
our $ho = selecthost('host');
sub livepatch_test () {
@@ -138,7 +157,7 @@ sub livepatch_test () {
foreach my $test (@livepatch_tests) {
# Default rc is zero.
my $expected_rc = defined($test->{rc}) ? $test->{rc} : 0;
- my $cmd = "(set -e;cd /usr/lib/debug/xen-livepatch;$test->{C})";
+ my $cmd = "(set -e;cd $livepatch_dir;$test->{C})";
my ($rc, $output) = target_cmd_output_root_status($ho, $cmd);
if ($rc != $expected_rc) {
@@ -158,10 +177,13 @@ sub livepatch_test () {
sub livepatch_check () {
foreach my $file (@livepatch_files) {
- if (!target_file_exists($ho, "/usr/lib/debug/xen-livepatch/$$file")) {
+ if (!target_file_exists($ho, "$$livepatch_dir/$$file")) {
die "$file is missing!\n";
}
}
+ if (target_file_exists($ho, "$livepatch_dir/xen_local_symbols.livepatch")) {
+ @livepatch_tests = (@livepatch_tests, @livepatch_tests_symbols);
+ }
}
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/3] livepatch: Add local and global symbol resolution.
2017-06-20 2:47 ` [PATCH v1 1/3] livepatch: Add local and global symbol resolution Konrad Rzeszutek Wilk
@ 2017-06-20 7:51 ` Jan Beulich
2017-06-20 15:59 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2017-06-20 7:51 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: andrew.cooper3, ross.lagerwall, xen-devel, ian.jackson
>>> On 20.06.17 at 04:47, <konrad.wilk@oracle.com> wrote:
> This way we can load livepatches with symbol names that
> are the same as long as they are local ('static').
>
> The use case here is to replace an existing livepatch
> with a newer one - and one which has the same local symbols.
>
> Without this patch we get:
> livepatch.c:819: livepatch: xen_local_symbols: duplicate new symbol:
> revert_hook
>
> when loading the new livepatch (before doing the replace).
>
> This due to livepatch assuming that all symbols are all
> global. With this patch:
>
> readelf --symbols xen_hello_world.livepatch
> File: xen_hello_world.livepatch
>
> Symbol table '.symtab' contains 55 entries:
> Num: Value Size Type Bind Vis Ndx Name
> ..snip..
> 34: 0000000000000000 4 OBJECT LOCAL DEFAULT 25 cnt
> 35: 000000000000000a 8 OBJECT LOCAL DEFAULT 7 __func__.4654
> 36: 0000000000000065 23 FUNC LOCAL DEFAULT 2 revert_hook
> 37: 000000000000007c 23 FUNC LOCAL DEFAULT 2 apply_hook
> 38: 0000000000000093 54 FUNC LOCAL DEFAULT 2 check_fnc
> ..snip..
>
> 47: 0000000000000000 54 FUNC GLOBAL HIDDEN 2 xen_hello_world
> 48: 0000000000000000 0 NOTYPE GLOBAL HIDDEN UND xen_extra_version
> ..snip..
> 52: 0000000000000000 0 NOTYPE GLOBAL HIDDEN UND printk
> 53: 0000000000000000 64 OBJECT GLOBAL HIDDEN 23 livepatch_xen_hello_world
>
> All the 'GLOBAL' have to be unique per livepatch. But the
> 'LOCAL' can all be the same which means the semantic of 'static'
> on functions and data variables is the right one.
I think this is wrong: Afaict your change results in main image and
patch local symbols to now be treated differently. While this may
indeed help patches which are meant to replace others, it is going
to get in the way if a patch wants to reference a local symbol
already altered (or newly introduced) by a prior one.
Question then is at what point in time name collisions should be
detected: In order for patch replacement to work, this obviously
shouldn't happen at the time the patch is being loaded, but in the
course of being applied. Otoh iirc "replace" doesn't use separate
old and new patch names, so the system must already be aware
of the correlation between them, and hence collision detection at
patch load time may still be possible (special casing the patch
being replaced).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 OSSTEST 3/3] ts-livepatch: Expand testcase to include global/local symbols
2017-06-20 2:47 ` [PATCH v1 OSSTEST 3/3] ts-livepatch: Expand testcase to include global/local symbols Konrad Rzeszutek Wilk
@ 2017-06-20 15:18 ` Ian Jackson
0 siblings, 0 replies; 8+ messages in thread
From: Ian Jackson @ 2017-06-20 15:18 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: xen-devel, andrew.cooper3, jbeulich, ross.lagerwall
Konrad Rzeszutek Wilk writes ("[PATCH v1 OSSTEST 3/3] ts-livepatch: Expand testcase to include global/local symbols"):
> testing. The test is to verify that the local symbols
> of payloads are ignored during loading.
Can we do this with substeps rather than a conditional test
execution ?
For example, if xen.git should suddenly stop producing this file, it
ought to be a test failure.
Maybe we should make each test into a separate testid ?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/3] livepatch: Add local and global symbol resolution.
2017-06-20 7:51 ` Jan Beulich
@ 2017-06-20 15:59 ` Konrad Rzeszutek Wilk
2017-06-20 16:18 ` Jan Beulich
0 siblings, 1 reply; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2017-06-20 15:59 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, ross.lagerwall, xen-devel, ian.jackson
On Tue, Jun 20, 2017 at 01:51:41AM -0600, Jan Beulich wrote:
> >>> On 20.06.17 at 04:47, <konrad.wilk@oracle.com> wrote:
> > This way we can load livepatches with symbol names that
> > are the same as long as they are local ('static').
> >
> > The use case here is to replace an existing livepatch
> > with a newer one - and one which has the same local symbols.
> >
> > Without this patch we get:
> > livepatch.c:819: livepatch: xen_local_symbols: duplicate new symbol:
> > revert_hook
> >
> > when loading the new livepatch (before doing the replace).
> >
> > This due to livepatch assuming that all symbols are all
> > global. With this patch:
> >
> > readelf --symbols xen_hello_world.livepatch
> > File: xen_hello_world.livepatch
> >
> > Symbol table '.symtab' contains 55 entries:
> > Num: Value Size Type Bind Vis Ndx Name
> > ..snip..
> > 34: 0000000000000000 4 OBJECT LOCAL DEFAULT 25 cnt
> > 35: 000000000000000a 8 OBJECT LOCAL DEFAULT 7 __func__.4654
> > 36: 0000000000000065 23 FUNC LOCAL DEFAULT 2 revert_hook
> > 37: 000000000000007c 23 FUNC LOCAL DEFAULT 2 apply_hook
> > 38: 0000000000000093 54 FUNC LOCAL DEFAULT 2 check_fnc
> > ..snip..
> >
> > 47: 0000000000000000 54 FUNC GLOBAL HIDDEN 2 xen_hello_world
> > 48: 0000000000000000 0 NOTYPE GLOBAL HIDDEN UND xen_extra_version
> > ..snip..
> > 52: 0000000000000000 0 NOTYPE GLOBAL HIDDEN UND printk
> > 53: 0000000000000000 64 OBJECT GLOBAL HIDDEN 23 livepatch_xen_hello_world
> >
> > All the 'GLOBAL' have to be unique per livepatch. But the
> > 'LOCAL' can all be the same which means the semantic of 'static'
> > on functions and data variables is the right one.
>
> I think this is wrong: Afaict your change results in main image and
> patch local symbols to now be treated differently. While this may
> indeed help patches which are meant to replace others, it is going
> to get in the way if a patch wants to reference a local symbol
> already altered (or newly introduced) by a prior one.
>
> Question then is at what point in time name collisions should be
> detected: In order for patch replacement to work, this obviously
> shouldn't happen at the time the patch is being loaded, but in the
> course of being applied. Otoh iirc "replace" doesn't use separate
> old and new patch names, so the system must already be aware
> of the correlation between them, and hence collision detection at
> patch load time may still be possible (special casing the patch
> being replaced).
This gets complicated, let me break it down to make sure I got it right.
I think what you are saying is that after a livepatch has been
applied the distinction of global/local should be lifted for
that livepatch.
But before we even get to that stage we have to deal with
the situation in which two (or more) livepatches have identical
local symbols ('revert_hook' for example). And there the
local symbols collision should not happen.
Now you mention an interesting case "if a patch wants to reference
a local symbol already altered (or newly introduced) by a prior one"
Let me break that apart, the "(or newly introduced)" meaning
we would have say two livepatches already applied.
The first one adds this new symbol and the second one
depends on the first livepatch (and uses the symbol).
We are trying to replace the second one with a newer version.
[I remember talking to Ross about this:
https://lists.xen.org/archives/html/xen-devel/2016-08/msg01756.html]
If the newly introduced symbol is local, the second livepatch
should _not_ be able to resolve it [But it does today, this patch
fixes this].
If the newly introduced symbol is global, the second livepatch
should be able to resolve it [and it does today]
If this symbol we want to reference is not within the live
patches, but in the main code - then:
If the symbol is local, we will still find it (symbols_lookup_by_name)
If the symbol is global, we will still find it (symbols_lookup_by_name)
Ah, so your point is that since the main code does not provide
_any_ semantics about local/global symbols, then by extension
the livepatches should neither.
While my patch does introduce this distinction?
Hmm.
Well, what I can say is that this issue (local symbol collision
in the livepatches) has been bitting us since November.
Our mechanism when deploying livepatches is to replace the loaded
livepatch with another one. Which means we only have on livepatch
applied and during the upgrade process have to load another one.
In November (XSA-204) we started shipping in the livepatches
a new local symbol (x86_emulate.c#_get_fpu).
livepatch: 617712b: new symbol x86_emulate.c#_get_fpu
Then when the next XSA came about and we made a new livepatch that
included everything prior and the new one, and we got:
livepatch.c:751: livepatch: fbab204: duplicate new symbol: x86_emulate.c#_get_fpu
which aborted the replacement of the livepatch.
The solution at hand was to rename the offending local symbol to
something more unique (the name of the livepatch):
livepatch: fbab204: new symbol x86_emulate.c#_get_fpu_fbab204
Which we are doing now for every livepatch. However I would
like a better mechanism, which is why I am proposing this patch.
P.S.
[Note to myself: Include this long explanation in the commit description]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/3] livepatch: Add local and global symbol resolution.
2017-06-20 15:59 ` Konrad Rzeszutek Wilk
@ 2017-06-20 16:18 ` Jan Beulich
0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2017-06-20 16:18 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: andrew.cooper3, ross.lagerwall, xen-devel, ian.jackson
>>> On 20.06.17 at 17:59, <konrad.wilk@oracle.com> wrote:
> Our mechanism when deploying livepatches is to replace the loaded
> livepatch with another one. Which means we only have on livepatch
> applied and during the upgrade process have to load another one.
I think this is the main problematic part here: You're trying to fix one
use case (single patch being replaced every time) while - afaict - you
break the other one (multiple stacked patches). For your case,
wouldn't it be sufficient to load the patch with some flag indicating
that all symbol handling (resolution as well as insertion) should only
consider the main image? Of course with that flag, a plain "apply"
would need to fail as long as there's any other patch loaded.
The downside of this is that it would special case things a little too
much for my taste. For example I'd expect to also be able to have
a couple of stacked patches and replace just the topmost one(s).
That would require more than a boolean flag to tell which symbols
to consider and which to ignore.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-20 16:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-20 2:47 [PATCH] Livepatch: Support local/global symbols Konrad Rzeszutek Wilk
2017-06-20 2:47 ` [PATCH v1 1/3] livepatch: Add local and global symbol resolution Konrad Rzeszutek Wilk
2017-06-20 7:51 ` Jan Beulich
2017-06-20 15:59 ` Konrad Rzeszutek Wilk
2017-06-20 16:18 ` Jan Beulich
2017-06-20 2:47 ` [PATCH v1 2/3] livepatch: Add xen_local_symbols test-case Konrad Rzeszutek Wilk
2017-06-20 2:47 ` [PATCH v1 OSSTEST 3/3] ts-livepatch: Expand testcase to include global/local symbols Konrad Rzeszutek Wilk
2017-06-20 15:18 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.