Re: [Xen-devel] [PATCH v3 5/7] x86/alt: Support for automatic padding calculations

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/03/18 16:49, Jan Beulich wrote:
>>>> On 08.03.18 at 17:23, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 08/03/18 15:42, Jan Beulich wrote:
>>>>>> On 07.03.18 at 16:51, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> @@ -175,18 +175,47 @@ void init_or_livepatch apply_alternatives(const 
>>>> struct alt_instr *start,
>>>>       * So be careful if you want to change the scan order to any other
>>>>       * order.
>>>>       */
>>>> -    for ( a = start; a < end; a++ )
>>>> +    for ( a = base = start; a < end; a++ )
>>>>      {
>>>>          uint8_t *orig = ALT_ORIG_PTR(a);
>>>>          uint8_t *repl = ALT_REPL_PTR(a);
>>>>          uint8_t buf[MAX_PATCH_LEN];
>>>> +        unsigned int total_len = a->orig_len + a->pad_len;
>>>>  
>>>> -        BUG_ON(a->repl_len > a->orig_len);
>>>> -        BUG_ON(a->orig_len > sizeof(buf));
>>>> +        BUG_ON(a->repl_len > total_len);
>>>> +        BUG_ON(total_len > sizeof(buf));
>>>>          BUG_ON(a->cpuid >= NCAPINTS * 32);
>>>>  
>>>> +        /*
>>>> +         * Detect sequences of alt_instr's patching the same origin site, 
>>>> and
>>>> +         * keep base pointing at the first alt_instr entry.  This is so 
>>>> we can
>>>> +         * refer to a single ->priv field for patching decisions.
>>>> +         *
>>>> +         * ->priv being nonzero means that the origin site has already 
>>>> been
>>>> +         * modified, and we shouldn't try to optimise the nops again.
>>>> +         */
>>>> +        if ( ALT_ORIG_PTR(base) != orig )
>>>> +            base = a;
>>> I don't understand why you need the new "priv" field - have a
>>> boolean local variable which you reset instead of base here, and
>>> which you check/set instead of base->priv below.
>> That can break in a "corrupted instruction stream" kind of way if we
>> perform two passes over the same set of alt_instr's, e.g. after loading
>> microcode, finding some new features, and rerunning alternatives.
> Well, we don't do anything like that today (which first of all would
> require all that stuff to come out of .init.*). But yes, if you want
> the code be prepared for that, fine with me:
>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> But it would be nice if you called out that extra aspect in the
> description.

Sure - I'll update this comment in the code.

FWIW, I chose this specific example because alternatives handling for
livepatching Spectre mitigations was a particularly sore area for some
people.

~Andrew

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