| Message ID | 20170620024759.32562-2-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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]
>>> 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
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__ */ /*
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(-)