On Mon, Sep 12, 2016 at 01:49:51AM -0600, Jan Beulich wrote: > >>> On 11.09.16 at 17:48, <konrad.w...@oracle.com> wrote: > > --- a/docs/misc/livepatch.markdown > > +++ b/docs/misc/livepatch.markdown > > @@ -875,6 +875,12 @@ section and the new function will reference the new > > string in the new > > > > This is implemented in the Xen Project hypervisor. > > > > +Note that the .bss section is only cleared when the ELF payload is > > uploaded. > > +Subsequent apply/revert/apply operation do no clear the .bss (or reset the > > +.data to what it was when loaded). Hence it is the responsibility of the > > +creator of the payload to reset these values to known good state if they > > +depend on them having certain values at apply/revert states. > > Was it, as an alternative, considered to disallow re-applying a > reverted patch without re-uploading?
I was thinking about it but not exactly sure of the ramifications. That is if you go this route - revert a patch - we could go the next step and just unload it. But that puts some question on how to schedule that without corruption - say we have payload A,B and we are replacing A,B with C. A,B should be reverted and unloaded.. That means some form of list .. Ugh. We could do a simpler way (which is what I think inline with your suggestion). This does the trick (and needs to be split up obviously) and also handles the case where you only have .text changes (like for NOPs). Interestingly, the linker always adds an .bss and .data section even if the code does not produce it. diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index d9eeab1..4a1abb2 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -53,6 +53,8 @@ struct livepatch_build_id { struct payload { uint32_t state; /* One of the LIVEPATCH_STATE_*. */ int32_t rc; /* 0 or -XEN_EXX. */ + bool_t reverted; /* Whether it was reverted. */ + bool_t safe_to_reapply; /* Can apply safely after revert. */ struct list_head list; /* Linked to 'payload_list'. */ const void *text_addr; /* Virtual address of .text. */ size_t text_size; /* .. and its size. */ @@ -313,7 +315,7 @@ static void calc_section(const struct livepatch_elf_sec *sec, size_t *size, static int move_payload(struct payload *payload, struct livepatch_elf *elf) { void *text_buf, *ro_buf, *rw_buf; - unsigned int i; + unsigned int i, rw_buf_sec, rw_buf_cnt = 0; size_t size = 0; unsigned int *offset; int rc = 0; @@ -386,7 +388,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) if ( elf->sec[i].sec->sh_flags & SHF_EXECINSTR ) buf = text_buf; else if ( elf->sec[i].sec->sh_flags & SHF_WRITE ) + { buf = rw_buf; + rw_buf_sec = i; + rw_buf_cnt++; + } else buf = ro_buf; @@ -407,6 +413,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) } } + if ( rw_buf_cnt == 1 ) + { + if ( !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) ) + payload->safe_to_reapply = true; + } out: xfree(offset); @@ -1120,6 +1131,7 @@ static int revert_payload(struct payload *data) list_del_rcu(&data->applied_list); unregister_virtual_region(&data->region); + data->reverted = true; return 0; } @@ -1501,6 +1513,19 @@ static int livepatch_action(xen_sysctl_livepatch_action_t *action) case LIVEPATCH_ACTION_APPLY: if ( data->state == LIVEPATCH_STATE_CHECKED ) { + /* + * It is unsafe to apply an reverted as the .data may not be in + * in pristine condition. Hence MUST unload and then apply patch + * again. + */ + if ( data->reverted && !data->safe_to_reapply ) + { + dprintk(XENLOG_ERR, "%s%s: can't revert as payload has .data. Please unload!\n", + LIVEPATCH, data->name); + data->rc = -EINVAL; + break; + } + rc = build_id_dep(data, !!list_empty(&applied_list)); if ( rc ) break; diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile index 69b0cdd..e03a26d 100644 --- a/xen/test/livepatch/Makefile +++ b/xen/test/livepatch/Makefile @@ -72,7 +72,7 @@ $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o note.o: $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin $(OBJCOPY) $(OBJCOPY_MAGIC) \ - --rename-section=.data=.livepatch.depends -S $@.bin $@ + --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@ rm -f $@.bin # @@ -83,7 +83,7 @@ note.o: hello_world_note.o: $(LIVEPATCH) $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) $@.bin $(OBJCOPY) $(OBJCOPY_MAGIC) \ - --rename-section=.data=.livepatch.depends -S $@.bin $@ + --rename-section=.data=.livepatch.depends,,alloc,load,readonly,data,contents -S $@.bin $@ rm -f $@.bin xen_bye_world.o: config.h @@ -103,6 +103,8 @@ xen_nop.o: config.h .PHONY: $(LIVEPATCH_NOP) $(LIVEPATCH_NOP): xen_nop.o note.o $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^ + $(OBJCOPY) --remove-section=.bss --remove-section=.data $@ + $(OBJCOPY) --strip-unneeded $@ .PHONY: livepatch livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel