Re: [Xen-devel] [PATCH 2/5] xen/alternatives: Plumb a 'live' parameter through apply_alternatives()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/01/18 11:05, Julien Grall wrote:
> Hi Andrew,
>
> On 29/01/18 15:38, Andrew Cooper wrote:
>> On x86, we would like to alter how we patch based on whether there is
>> any
>> chance of the code being patched being concurrently executed.
>>
>> prepare_payload() passes false (as the livepatch definitely isn't
>> live at this
>> point), whereas the boot-time alternatives application passes true.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxx>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
>> ---
>>   xen/arch/arm/alternative.c        | 10 ++++++----
>>   xen/arch/x86/alternative.c        |  5 +++--
>>   xen/common/livepatch.c            |  2 +-
>>   xen/include/asm-arm/alternative.h |  6 ++++--
>>   xen/include/asm-x86/alternative.h |  3 ++-
>>   5 files changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
>> index 99112e1..078b259 100644
>> --- a/xen/arch/arm/alternative.c
>> +++ b/xen/arch/arm/alternative.c
>> @@ -98,7 +98,8 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>>    * The region patched should be read-write to allow
>> __apply_alternatives
>>    * to replacing the instructions when necessary.
>>    */
>> -static void __apply_alternatives(const struct alt_region *region)
>> +static void __apply_alternatives(const struct alt_region *region,
>> +                                 bool live)
>>   {
>>       const struct alt_instr *alt;
>>       const u32 *replptr;
>> @@ -193,7 +194,7 @@ static int __init
>> __apply_alternatives_multi_stop(void *unused)
>>           region.begin = (void *)__alt_instructions - (void *)_start
>> + xenmap;
>>           region.end = (void *)__alt_instructions_end - (void
>> *)_start + xenmap;
>>   -        __apply_alternatives(&region);
>> +        __apply_alternatives(&region, true);
>>             unregister_virtual_region(&patch_region);
>>   @@ -224,14 +225,15 @@ void __init apply_alternatives_all(void)
>>   }
>>     void apply_alternatives(const struct alt_instr *start,
>> -                        const struct alt_instr *end)
>> +                        const struct alt_instr *end,
>> +                        bool live)
>
> This function is not able to deal with "live" code, so I think at
> least need an ASSERT(!live) to prevent mis-usage of the code.

This passes straight through into __apply_alternatives(), just like
__apply_alternatives_multi_stop does, and multi_stop is used on live code.

Either both are unsafe (although all evidence to the contrary), or both
are safe, but I don't think that an assert here is appropriate.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel