| Message ID | 1474039754-25816-2-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 16.09.16 at 17:29, <konrad.wilk@oracle.com> wrote: > On general this is unhealthy - as the payload's .bss (definitly) > or .data (maybe) will be modified once the payload is running. > > Doing an revert and then re-applying the payload with a non-pristine > .bss or .data can lead to unforseen consequences (.bss are assumed > to always contain zero value but now they may have a different value). > > There is one exception - if the payload contains only one .data section > - the .livepatch.funcs, then it is OK to re-apply an revert. > > Suggested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> I think Ross is actually the primary person to be listed here. > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -52,6 +52,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. */ You nicely use "true" int the code below - please also use plain "bool" here. > @@ -381,7 +383,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++; Wouldn't it make sense to exclude zero-size sections here? Perhaps even worth skipping them together with !SHF_ALLOC ones (and then of course also in the other earlier loop)? The more that ISTR you mentioning that empty .data and .bss get created by the assembler even if there's no respective source contribution? Jan
On 09/16/2016 04:29 PM, Konrad Rzeszutek Wilk wrote: > On general this is unhealthy - as the payload's .bss (definitly) > or .data (maybe) will be modified once the payload is running. > > Doing an revert and then re-applying the payload with a non-pristine > .bss or .data can lead to unforseen consequences (.bss are assumed > to always contain zero value but now they may have a different value). > > There is one exception - if the payload contains only one .data section > - the .livepatch.funcs, then it is OK to re-apply an revert. > > Suggested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > This looks reasonable, with Jan's comments addressed.
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown index 89c1050..81f4fc9 100644 --- a/docs/misc/livepatch.markdown +++ b/docs/misc/livepatch.markdown @@ -1061,6 +1061,13 @@ depending on the current state of data. As such it should not be attempted. That said we should provide hook functions so that the existing data can be changed during payload application. +To guarantee safety we disallow re-applying an payload after it has been +reverted. This is because we cannot guarantee that the state of .bss +and .data to be exactly as it was during loading. Hence the administrator +MUST unload the payload and upload it again to apply it. + +There is an exception to this: if the payload only has .livepatch.funcs; +and no .data or .bss sections. ### Inline patching diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 23e4d51..967985c 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -52,6 +52,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. */ @@ -308,7 +310,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; @@ -381,7 +383,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; @@ -402,6 +408,10 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf) } } + /* No .bss and no .data, and only on RW - .livepatch.funcs section. */ + if ( rw_buf_cnt == 1 && + !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) ) + payload->safe_to_reapply = true; out: xfree(offset); @@ -1057,6 +1067,7 @@ static int revert_payload(struct payload *data) list_del_rcu(&data->applied_list); unregister_virtual_region(&data->region); + data->reverted = true; return 0; } @@ -1438,6 +1449,20 @@ 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 payload as the .data (or .bss) + * may not be in in pristine condition. Hence MUST unload and then + * apply patch again. Unless the payload has no .bss or only one + * RW section (.livepatch.funcs). + */ + 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;
On general this is unhealthy - as the payload's .bss (definitly) or .data (maybe) will be modified once the payload is running. Doing an revert and then re-applying the payload with a non-pristine .bss or .data can lead to unforseen consequences (.bss are assumed to always contain zero value but now they may have a different value). There is one exception - if the payload contains only one .data section - the .livepatch.funcs, then it is OK to re-apply an revert. Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Jan Beulich <jbeulich@suse.com> v6: New submission --- docs/misc/livepatch.markdown | 7 +++++++ xen/common/livepatch.c | 27 ++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-)