Re: [Xen-devel] [PATCH v3 1/9] x86: infrastructure to allow converting certain indirect calls to direct ones

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.09.18 at 12:49, <wei.liu2@xxxxxxxxxx> wrote:
> On Tue, Sep 11, 2018 at 07:32:04AM -0600, Jan Beulich wrote:
>> @@ -218,6 +219,13 @@ void init_or_livepatch apply_alternative
> 
> I think you need to fix the comment before this if statement. At the
> very least you're now using two ->priv to make decision on patching.

I've been considering this, but even a very close look didn't turn up
anything I could do to this comment to improve it. Suggestions
welcome.

> Also I wonder why you keep base, since ...
> 
>>          if ( ALT_ORIG_PTR(base) != orig )
>>              base = a;
>>  
>> +        /* Skip patch sites already handled during the first pass. */
>> +        if ( a->priv )
>> +        {
>> +            ASSERT(force);
>> +            continue;
>> +        }
>> +
>>          /* If there is no replacement to make, see about optimising the 
>> nops. */
>>          if ( !boot_cpu_has(a->cpuid) )
>>          {
>> @@ -225,7 +233,7 @@ void init_or_livepatch apply_alternative
>>              if ( base->priv )
>>                  continue;
> 
> ... base is guaranteed to be a at this point, furthermore there is
> already a check to skip patching added in this patch.

Why would base equal a here?

> -            base->priv = 1;
> +            a->priv = 1;

This communicates from one pass to the next: Previously it was
sufficient to set ->priv on only the first of a group of patches for
the same site. This is no longer the case with the multi-pass
approach - we need to keep record for every entry, such that we
won't touch again in pass 2 what pass 1 has already dealt with.

With base and a not necessarily equal, I think the second half of
your statement becomes irrelevant (as we may be looking at
different entries' ->priv there and here). I agree this could perhaps
be written slightly differently; personally I find it easier to prove
correct in this shape than if we e.g. relied on base->priv to
necessarily be set in pass 2 when we process a non-"primary"
entry. The patch description forbids certain combinations of
patches, but I think the code should nevertheless have as few
latent bugs in this regard as possible.

>> @@ -236,20 +244,74 @@ void init_or_livepatch apply_alternative
>>              continue;
>>          }
>>  
>> -        base->priv = 1;
>> -
>>          memcpy(buf, repl, a->repl_len);
>>  
>>          /* 0xe8/0xe9 are relative branches; fix the offset. */
>>          if ( a->repl_len >= 5 && (*buf & 0xfe) == 0xe8 )
>> -            *(int32_t *)(buf + 1) += repl - orig;
>> +        {
>> +            /*
>> +             * Detect the special case of indirect-to-direct branch 
>> patching:
>> +             * - replacement is a direct CALL/JMP (opcodes 0xE8/0xE9; 
>> already
>> +             *   checked above),
>> +             * - replacement's displacement is -5 (pointing back at the very
>> +             *   insn, which makes no sense in a real replacement insn),
>> +             * - original is an indirect CALL/JMP (opcodes 0xFF/2 or 0xFF/4)
>> +             *   using RIP-relative addressing.
>> +             * Some function targets may not be available when we come here
>> +             * the first time. Defer patching of those until the 
>> post-presmp-
>> +             * initcalls re-invocation. If at that point the target pointer 
>> is
>> +             * still NULL, insert "UD2; UD0" (for ease of recognition) 
>> instead
>> +             * of CALL/JMP.
>> +             */
>> +            if ( a->cpuid == X86_FEATURE_ALWAYS &&
>> +                 *(int32_t *)(buf + 1) == -5 &&
>> +                 a->orig_len >= 6 &&
>> +                 orig[0] == 0xff &&
>> +                 orig[1] == (*buf & 1 ? 0x25 : 0x15) )
>> +            {
>> +                long disp = *(int32_t *)(orig + 2);
>> +                const uint8_t *dest = *(void **)(orig + 6 + disp);
>> +
>> +                if ( dest )
>> +                {
>> +                    disp = dest - (orig + 5);
>> +                    ASSERT(disp == (int32_t)disp);
>> +                    *(int32_t *)(buf + 1) = disp;
>> +                }
>> +                else if ( force )
>> +                {
>> +                    buf[0] = 0x0f;
>> +                    buf[1] = 0x0b;
>> +                    buf[2] = 0x0f;
>> +                    buf[3] = 0xff;
>> +                    buf[4] = 0xff;
> 
> I think these are opcodes for "UD2; UD0". Please add a comment for them.
> Having to go through SDM to figure out what they are isn't nice.

Well, I'm saying so in the relatively big comment ahead of this block of
code. I don't want to say the same thing twice.

> At this point I also think the name "force" is not very good. What/who
> is forced here? Why not use a more descriptive name like "post_init" or
> "system_active"?

_Patching_ is being forced here, i.e. even if we still can't find a non-NULL
pointer, we still patch the site. I'm certainly open for suggestions, but
I don't really like either of the two suggestions you make any better than
the current "force". The next best option I had been thinking about back
then was to pass in a number, to identify the stage / phase / pass we're in.

Jan


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