Re: [PATCH v2 3/3] xen/livepatch: Fix .altinstructions safety checks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 17/04/2023 1:35 pm, Jan Beulich wrote:
> On 17.04.2023 14:13, Andrew Cooper wrote:
>> --- a/xen/common/livepatch.c
>> +++ b/xen/common/livepatch.c
>> @@ -803,28 +803,84 @@ static int prepare_payload(struct payload *payload,
>>      if ( sec )
>>      {
>>  #ifdef CONFIG_HAS_ALTERNATIVE
>> +        /*
>> +         * (As of April 2023), Alternatives are formed of:
>> +         * - An .altinstructions section with an array of struct 
>> alt_instr's.
>> +         * - An .altinstr_replacement section containing instructions.
>> +         *
>> +         * An individual alt_instr contains:
>> +         * - An orig reference, pointing into .text with a nonzero length
>> +         * - A repl reference, pointing into .altinstr_replacement
>> +         *
>> +         * It is legal to have zero-length replacements, meaning it is legal
>> +         * for the .altinstr_replacement section to be empty too.  An
>> +         * implementation detail means that a zero-length replacement's repl
>> +         * reference will still be in the .altinstr_replacement section.
> Didn't you agree that "will" is not really true, and it's at best "may", but
> then also doesn't really matter here in the first place (suggesting that the
> sentence might best be dropped, to avoid drawing attention to something that
> might at best confuse the reader as to its relevance)?

Only that "will be at 0" wasn't actually true.

Right now, the repl reference *will* be somewhere in
altinstr_replacement.  It is discussed here because it is what the check
enforces.

As an implementation detail, it is of course free to change in the
future if needs be.

~Andrew