Re: [PATCH] xen/arm: alternative: Make it possible to patch outside of the hypervisor

[Julien Grall <julien.grall@arm.com>]

Hi Konrad,

On 06/09/2016 20:25, Konrad Rzeszutek Wilk wrote:
> On Thu, Sep 01, 2016 at 08:56:39AM +0100, Julien Grall wrote:
>> With livepatch the alternatives that should be patched are outside of
>> the Xen hypervisor _start -> _end. The current code is assuming that
>> only Xen could be patched and therefore will explode when a payload
>> contains alternatives.
>>
>> Given that alt_instr contains a relative offset, the function
>> __apply_alternatives could directly take in parameter the virtual
>                                              ^- the    ^- for
>> address of the alt_instr set of the re-mapped region. So we can mandate
>> the callers of __apply_alternatives to ensure the region has read-write
>
> s/ensure the region/provide us with a region that/
>> access beforehand.
>
> How do we mandate it?

I don't have any idea how to write the check here. I think the only way 
would be a comment on __apply_alternatives as you suggested below.

>
> Should the __apply_alternatives have an comment about it?
>
>>
>> The only caller that will patch directly the Xen binary is the function
>> __apply_alternatives_multi_stop. The other caller apply_alternatives
>> will work on the payload which will still have read-write access at that
>> time.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>>     This is an alternative of the patch suggested by Konrad [1] to fix
>>     alternatives patching with livepatching.
>>
>>     [1] https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02880.html
>
> You also did
>
>  s/_stext/_start/

And s/text_mfn/xen_mfn/

I didn't write a separate patch because the code has been reworked quite 
a lot. But I am fine to move this in a separate patch if necessary.

>
>>
>> ---
>>  xen/arch/arm/alternative.c | 58 +++++++++++++++++++++++++---------------------
>>  1 file changed, 31 insertions(+), 27 deletions(-)
>>
>> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
>> index 8ee5a11..db4bd0f 100644
>> --- a/xen/arch/arm/alternative.c
>> +++ b/xen/arch/arm/alternative.c
>> @@ -97,26 +97,11 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>>  static int __apply_alternatives(const struct alt_region *region)
>>  {
>>      const struct alt_instr *alt;
>> -    const u32 *origptr, *replptr;
>> -    u32 *writeptr, *writemap;
>> -    mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
>> -    unsigned int text_order = get_order_from_bytes(_end - _start);
>> +    const u32 *replptr;
>> +    u32 *origptr;
>>
>>      printk(XENLOG_INFO "alternatives: Patching kernel code\n");
>>
>> -    /*
>> -     * The text section is read-only. So re-map Xen to be able to patch
>> -     * the code.
>> -     */
>> -    writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
>> -                      VMAP_DEFAULT);
>> -    if ( !writemap )
>> -    {
>> -        printk(XENLOG_ERR "alternatives: Unable to map the text section (size %u)\n",
>> -               1 << text_order);
>> -        return -ENOMEM;
>> -    }
>> -
>>      for ( alt = region->begin; alt < region->end; alt++ )
>>      {
>>          u32 insn;
>> @@ -128,7 +113,6 @@ static int __apply_alternatives(const struct alt_region *region)
>>          BUG_ON(alt->alt_len != alt->orig_len);
>>
>>          origptr = ALT_ORIG_PTR(alt);
>> -        writeptr = origptr - (u32 *)_start + writemap;
>>          replptr = ALT_REPL_PTR(alt);
>>
>>          nr_inst = alt->alt_len / sizeof(insn);
>> @@ -136,19 +120,17 @@ static int __apply_alternatives(const struct alt_region *region)
>>          for ( i = 0; i < nr_inst; i++ )
>>          {
>>              insn = get_alt_insn(alt, origptr + i, replptr + i);
>> -            *(writeptr + i) = cpu_to_le32(insn);
>> +            *(origptr + i) = cpu_to_le32(insn);
>>          }
>>
>>          /* Ensure the new instructions reached the memory and nuke */
>> -        clean_and_invalidate_dcache_va_range(writeptr,
>> -                                             (sizeof (*writeptr) * nr_inst));
>> +        clean_and_invalidate_dcache_va_range(origptr,
>> +                                             (sizeof (*origptr) * nr_inst));
>>      }
>>
>>      /* Nuke the instruction cache */
>>      invalidate_icache();
>>
>> -    vunmap(writemap);
>> -
>>      return 0;
>>  }
>>
>> @@ -159,10 +141,6 @@ static int __apply_alternatives(const struct alt_region *region)
>>  static int __apply_alternatives_multi_stop(void *unused)
>>  {
>>      static int patched = 0;
>> -    const struct alt_region region = {
>> -        .begin = __alt_instructions,
>> -        .end = __alt_instructions_end,
>> -    };
>>
>>      /* We always have a CPU 0 at this point (__init) */
>>      if ( smp_processor_id() )
>> @@ -174,12 +152,38 @@ static int __apply_alternatives_multi_stop(void *unused)
>>      else
>>      {
>>          int ret;
>> +        struct alt_region region;
>> +        mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
>> +        unsigned int xen_order = get_order_from_bytes(_end - _start);
>> +        char *xenmap;
>
> Could this be 'void *'?
>>
>>          BUG_ON(patched);
>> +
>> +        /*
>> +         * The text and inittext section is read-only. So re-map Xen to
>> +         * be able to patch the code.
>> +         */
>> +        xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
>> +                        VMAP_DEFAULT);
>> +        /* Re-mapping Xen is not expected to fail during boot. */
>> +        BUG_ON(!xenmap);
>> +
>> +        /*
>> +         * Find the virtual address of the alternative region in the new
>> +         * mapping.
>> +         * alt_instr contains relative offset, so the function
>> +         * __apply_alternatives will patch in the re-mapped version of
>> +         * Xen.
>> +         */
>> +        region.begin = (void *)((char *)__alt_instructions - _start + xenmap);
>> +        region.end = (void *)((char *)__alt_instructions_end - _start + xenmap);
>
> Which I think would make this have less casts?
>
> Oh wait. _start is a char *, so we would have to have:
>
>   region.begin = (void *)__alt_instructions - (void *)_start + xenmap;
>   region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;
>
> Julien, you are the maintainer, so your call.

Your suggestion seems to be clearer.

>
>> +
>>          ret = __apply_alternatives(&region);
>>          /* The patching is not expected to fail during boot. */
>>          BUG_ON(ret != 0);
>>
>> +        vunmap(xenmap);
>> +
>>          /* Barriers provided by the cache flushing */
>>          write_atomic(&patched, 1);
>>      }
>> --
>> 1.9.1
>>
>

Regards,

-- 
Julien Grall

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