Re: [PATCH v5] x86/livepatch: align functions to ensure minimal distance between entry points

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.01.2024 18:27, Roger Pau Monné wrote:
> On Mon, Jan 22, 2024 at 12:21:47PM +0100, Jan Beulich wrote:
>> On 22.01.2024 12:02, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -99,6 +99,10 @@ SECTIONS
>>>         *(.text)
>>>  #ifdef CONFIG_CC_SPLIT_SECTIONS
>>>         *(.text.*)
>>> +#endif
>>> +#ifdef CONFIG_FUNCTION_ALIGNMENT
>>> +       /* Ensure enough distance with the next placed section. */
>>> +       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
>>>  #endif
>>>         *(.text.__x86_indirect_thunk_*)
>>
>> I continue to fail to see how an alignment directive can guarantee minimum
>> distance. In the worst case such a directive inserts nothing at all.
> 
> I'm confused, you did provide a RB for this in v4:
> 
> https://lore.kernel.org/xen-devel/4cad003f-dda0-4e22-a770-5a5ff56f4d35@xxxxxxxx/
> 
> Which is basically the same code with a few comments and wording
> adjustments.

Hmm, yes. I think the aspect above was raised before, but then (perhaps)
kind of addressed. (I'm puzzled then too: Why did you drop the R-b, when
nothing substantially changed?) Yet re-reading the description, there's
nothing said to this effect. Specifically ...

>> IOW
>> at the very least there's a non-spelled-out assumption here about the last
>> item in the earlier section having suitable alignment and thus, if small
>> in size, being suitably padded.
> 
> Please bear with me, but I'm afraid I don't understand your concerns.
> 
> For livepatch build tools (which is the only consumer of such
> alignments) we already have the requirement that a function in order
> to be suitable for being live patched must reside in it's own
> section.
> 
> We do want to aim for functions (even assembly ones) to live in their
> own sections in order to be live patched, and to be properly aligned.
> However it's also fine for functions to use a different (smaller)
> alignment, the livepatch build tools will detect this and use the
> alignment reported.

... I don't think this and ...

> While we want to get to a point where everything that we care to patch
> lives in it's own section, and is properly padded to ensure minimal
> required space, I don't see why the proposed approach here should be
> blocked, as it's a step in the right direction of achieving the
> goal.
> 
> Granted, there's still assembly code that won't be suitably padded,
> but the livepatch build tools won't assume it to be padded.

... this is being pointed out. Which I think is relevant to make
explicit not the least because the build tools aren't part of the main
Xen tree. Plus many (like me) may not be overly familiar with how they
work.

>  After
> your series to enable assembly annotations we can also make sure the
> assembly annotated functions live in separate sections and are
> suitably aligned.
> 
>> Personally I don't think merely spelling
>> out such a requirement would help - it would end up being a trap for
>> someone to fall into.
> 
>> I'm further curious why .text.__x86_indirect_thunk_* is left past the
>> inserted alignment. While pretty unlikely, isn't it in principle possible
>> for the thunks there to also need patching? Aren't we instead requiring
>> then that assembly functions (and thunks) all be suitably aligned as well?
> 
> Those are defined in assembly, so requires CONFIG_FUNCTION_ALIGNMENT
> to also be applied to the function entry points in assembly files.

I see. Yet the question then remains: Why is the alignment not inserted
after them? Or will the insertion need to move later on (which would feel
odd)?

Jan