| Message ID | 20170912003726.368-8-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote: > This surfaced due to "xen/livepatch/x86/arm32: Force > .livepatch.depends section to be uint32_t aligned." which switched > to a different way of including the build-id. > > Each livepatch ends with a global: > > 30: 00000000 1 OBJECT GLOBAL HIDDEN 7 note_depends > > which will cause collision when loading. > > One attempted solution was to add in the Makefile stanza: > @sed -i '/unsigned/static unsinged/' $@ > > But that resulted in the note_depends being omitted from the livepatch > (as it was static and not used) which meant we would not have an > .livepatch_depends section which we require. Did you consider using objcopy's --localize-symbol instead? Jan
On Tue, Sep 12, 2017 at 08:48:33AM -0600, Jan Beulich wrote: > >>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote: > > This surfaced due to "xen/livepatch/x86/arm32: Force > > .livepatch.depends section to be uint32_t aligned." which switched > > to a different way of including the build-id. > > > > Each livepatch ends with a global: > > > > 30: 00000000 1 OBJECT GLOBAL HIDDEN 7 note_depends > > > > which will cause collision when loading. > > > > One attempted solution was to add in the Makefile stanza: > > @sed -i '/unsigned/static unsinged/' $@ > > > > But that resulted in the note_depends being omitted from the livepatch > > (as it was static and not used) which meant we would not have an > > .livepatch_depends section which we require. > > Did you consider using objcopy's --localize-symbol instead? Yes, so that note_depends is not globally visible. But that won't help as hypervisor treats both local and global symbols as global when resolving them. That is each of the livepatch has the node_depends in it, and we can't load xen_hello_world, followed by xen_replace_world test-case (so stacking them on top of each other) - as both have the same local symbol. (This is fixed in "livepatch: Add local and global symbol resolution." on which you said: > 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. (https://www.mail-archive.com/xen-devel@lists.xen.org/msg111710.html) > > Jan >
>>> On 13.09.17 at 01:46, <konrad@kernel.org> wrote: > On Tue, Sep 12, 2017 at 08:48:33AM -0600, Jan Beulich wrote: >> >>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote: >> > This surfaced due to "xen/livepatch/x86/arm32: Force >> > .livepatch.depends section to be uint32_t aligned." which switched >> > to a different way of including the build-id. >> > >> > Each livepatch ends with a global: >> > >> > 30: 00000000 1 OBJECT GLOBAL HIDDEN 7 note_depends >> > >> > which will cause collision when loading. >> > >> > One attempted solution was to add in the Makefile stanza: >> > @sed -i '/unsigned/static unsinged/' $@ >> > >> > But that resulted in the note_depends being omitted from the livepatch >> > (as it was static and not used) which meant we would not have an >> > .livepatch_depends section which we require. >> >> Did you consider using objcopy's --localize-symbol instead? > > Yes, so that note_depends is not globally visible. But that won't help > as hypervisor treats both local and global symbols as global when resolving > them. > > That is each of the livepatch has the node_depends in it, and we can't > load xen_hello_world, followed by xen_replace_world test-case (so > stacking them on top of each other) - as both have the same local > symbol. Oh, right. Then perhaps stripping the symbol is as good or as bad as deriving the symbol name from e.g. the patch name, or putting some randomized tag on it. > (This is fixed in "livepatch: Add local and global symbol resolution." > on which you said: > > > 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. > > (https://www.mail-archive.com/xen-devel@lists.xen.org/msg111710.html) Right, this is a basically unresolvable ambiguity, I'm afraid. We'd need a 3rd class of symbols. It may be worth considering to (ab)use e.g. STV_INTERNAL for this purpose. Jan
On Wed, Sep 13, 2017 at 02:51:41AM -0600, Jan Beulich wrote: > >>> On 13.09.17 at 01:46, <konrad@kernel.org> wrote: > > On Tue, Sep 12, 2017 at 08:48:33AM -0600, Jan Beulich wrote: > >> >>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote: > >> > This surfaced due to "xen/livepatch/x86/arm32: Force > >> > .livepatch.depends section to be uint32_t aligned." which switched > >> > to a different way of including the build-id. > >> > > >> > Each livepatch ends with a global: > >> > > >> > 30: 00000000 1 OBJECT GLOBAL HIDDEN 7 note_depends > >> > > >> > which will cause collision when loading. > >> > > >> > One attempted solution was to add in the Makefile stanza: > >> > @sed -i '/unsigned/static unsinged/' $@ > >> > > >> > But that resulted in the note_depends being omitted from the livepatch > >> > (as it was static and not used) which meant we would not have an > >> > .livepatch_depends section which we require. > >> > >> Did you consider using objcopy's --localize-symbol instead? > > > > Yes, so that note_depends is not globally visible. But that won't help > > as hypervisor treats both local and global symbols as global when resolving > > them. > > > > That is each of the livepatch has the node_depends in it, and we can't > > load xen_hello_world, followed by xen_replace_world test-case (so > > stacking them on top of each other) - as both have the same local > > symbol. > > Oh, right. Then perhaps stripping the symbol is as good or as bad as > deriving the symbol name from e.g. the patch name, or putting some > randomized tag on it. Yes, and I had in mind changing the name of it (to prefix it with the livepatch name) using --redefine-sym. But then figured it may be just easier to ditch the symbol altogether. Let me try it out - I do wonder if that would remove the need for stripping the debug symbols or if that would still trip the issue I keep on having - which is that the debug section would reference the original symbol. > > > (This is fixed in "livepatch: Add local and global symbol resolution." > > on which you said: > > > > > 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. > > > > (https://www.mail-archive.com/xen-devel@lists.xen.org/msg111710.html) > > Right, this is a basically unresolvable ambiguity, I'm afraid. We'd > need a 3rd class of symbols. It may be worth considering to (ab)use > e.g. STV_INTERNAL for this purpose. Oooooh. Let me look at that. Thank you! > > Jan >
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile index 89ad89dfd5..9e73861732 100644 --- a/xen/test/livepatch/Makefile +++ b/xen/test/livepatch/Makefile @@ -53,6 +53,7 @@ xen_hello_world.o: config.h livepatch_depends.h .PHONY: $(LIVEPATCH) $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^ + $(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@ # # This target is only accessible if CONFIG_LIVEPATCH is defined, which @@ -88,18 +89,21 @@ xen_bye_world.o: config.h hello_world_livepatch_depends.h .PHONY: $(LIVEPATCH_BYE) $(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_BYE) $^ + $(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@ xen_replace_world.o: config.h livepatch_depends.h .PHONY: $(LIVEPATCH_REPLACE) $(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^ + $(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@ xen_nop.o: config.h livepatch_depends.h .PHONY: $(LIVEPATCH_NOP) $(LIVEPATCH_NOP): xen_nop.o $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^ + $(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@ .PHONY: livepatch livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP)
This surfaced due to "xen/livepatch/x86/arm32: Force .livepatch.depends section to be uint32_t aligned." which switched to a different way of including the build-id. Each livepatch ends with a global: 30: 00000000 1 OBJECT GLOBAL HIDDEN 7 note_depends which will cause collision when loading. One attempted solution was to add in the Makefile stanza: @sed -i '/unsigned/static unsinged/' $@ But that resulted in the note_depends being omitted from the livepatch (as it was static and not used) which meant we would not have an .livepatch_depends section which we require. The solution to this is to remove the symbol via the --strip-symbols after generating the livepatch. However that fails as note_depends is in use by .rel.debug_info: Relocation section '.rel.debug_info' at offset 0x151c contains 113 entries: Offset Info Type Sym.Value Sym. Name .. 00000625 00001e02 R_ARM_ABS32 00000000 note_depends And the solution to that is to also slap on --strip-debug which removes various .debug* sections (which livepatch ignores anyhow): .debug_aranges, .debug_info, .debug_abbrev, .debug_line, .debug_frame, .debug_str, and their .rel.* sections. And that will remove that. Alternatively we could also use --localize-symbol so that note_depends is not globally visible. But that won't help as hypervisor treats both local and global symbols as global when resolving them. (This is fixed in "livepatch: Add local and global symbol resolution." but that patch is stuck in limbo). Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- v3: First version --- xen/test/livepatch/Makefile | 4 ++++ 1 file changed, 4 insertions(+)