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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.04.2023 15:37, Andrew Cooper wrote:
> 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.

Oh, right - I'm sorry for the noise.

Jan

> 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