Re: [Xen-devel] [PATCH v6 1/6] livepatch: Disallow applying after an revert

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.09.16 at 17:29, <konrad.wilk@xxxxxxxxxx> 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@xxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>

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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel