Re: [PATCH v6 16/24] xsplice: Add support for alternatives

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/04/16 04:49, Konrad Rzeszutek Wilk wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
>
> Add support for applying alternative sections within xsplice payload.
> At payload load time, apply an alternative sections that are found.
>
> Also we add an test-case exercising a rather useless alternative
> (patching a NOP with a NOP) - but it does exercise the code-path.
>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>
> v2: Make a new alternative function that does not ASSERT on IRQs and
>     don't disable IRQs in the code when loading payload.
> v4: Include test-case
>     Include check for size of alternatives and that it is not a 0 size
>     section.
> v6: Add #define INIT to preserve __initness on alternative code.
>     Double check that alt_instr are only patching payload code.
> ---
> ---
>  xen/arch/x86/Makefile                    |  2 +-
>  xen/arch/x86/alternative.c               | 35 ++++++++++++++++++++++----------
>  xen/arch/x86/test/xen_hello_world_func.c |  5 +++++
>  xen/common/xsplice.c                     | 34 +++++++++++++++++++++++++++++++
>  xen/include/asm-x86/alternative.h        |  4 ++++
>  5 files changed, 68 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index d85287d..08a7b68 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -6,7 +6,7 @@ subdir-y += mm
>  subdir-$(CONFIG_XENOPROF) += oprofile
>  subdir-y += x86_64
>  
> -obj-bin-y += alternative.init.o
> +obj-bin-y += alternative.o

You want to keep this as alternative.init.o if not CONFIG_XSPLICE

> @@ -142,15 +144,13 @@ static void *__init text_poke_early(void *addr, const void *opcode, size_t len)
>   * APs have less capabilities than the boot processor are not handled.
>   * Tough. Make sure you disable such features by hand.
>   */
> -static void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
> +void __INIT apply_alternatives_nocheck(struct alt_instr *start, struct alt_instr *end)
>  {
>      struct alt_instr *a;
>      u8 *instr, *replacement;
>      u8 insnbuf[MAX_PATCH_LEN];
>      unsigned long cr0 = read_cr0();

The cr0 adjustment of WP should also move from the nocheck() to the
plain variant.  This avoid temporisingly disabling WP on the CPU setting
up the patch, before the payload is "secured".

>  
> -    ASSERT(!local_irq_is_enabled());
> -
>      printk(KERN_INFO "alt table %p -> %p\n", start, end);
>  
>      /* Disable WP to allow application of alternatives to read-only pages. */
> @@ -183,13 +183,26 @@ static void __init apply_alternatives(struct alt_instr *start, struct alt_instr
>  
>          add_nops(insnbuf + a->replacementlen,
>                   a->instrlen - a->replacementlen);
> -        text_poke_early(instr, insnbuf, a->instrlen);
> +        text_poke(instr, insnbuf, a->instrlen);
>      }
>  
>      /* Reinstate WP. */
>      write_cr0(cr0);
>  }
>  
> +#undef __INIT
> +#undef __INITCONST
> +#undef __INITDATA

Why these undefs?

> +/*
> + * This routine is called with local interrupt disabled and used during
> + * bootup.
> + */
> +void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
> +{
> +    ASSERT(!local_irq_is_enabled());
> +    apply_alternatives_nocheck(start, end);
> +}
> +
>  void __init alternative_instructions(void)
>  {
>      nmi_callback_t *saved_nmi_callback;
> diff --git a/xen/arch/x86/test/xen_hello_world_func.c b/xen/arch/x86/test/xen_hello_world_func.c
> index 7e239ca..55e84ac 100644
> --- a/xen/arch/x86/test/xen_hello_world_func.c
> +++ b/xen/arch/x86/test/xen_hello_world_func.c
> @@ -3,6 +3,9 @@
>   *
>   */
>  
> +#include <asm/alternative.h>
> +#include <asm/nops.h>
> +#include <asm/uaccess.h>
>  #include <xen/types.h>
>  
>  static unsigned long *non_canonical_addr = (unsigned long *)(1UL<<48);
> @@ -12,6 +15,8 @@ const char *xen_hello_world(void)
>  {
>      unsigned long tmp = 0xdeadbeef;
>      int rc;
> +
> +    alternative(ASM_NOP1, ASM_NOP1, X86_FEATURE_NX);

NX is very definitely not always available.  Use LM instead, and leave a
comment saying that LM will always be available on Xen.

With these fixed, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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