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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Jan 23, 2024 at 08:53:15AM +0100, Jan Beulich wrote:
> 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?)

The RB was given quite some time ago, so I felt it was probably best
to drop it in case you wanted to re-asses the patch.  Specially given
you have now done the work to also add support for this feature to
assembly annotated functions.

> 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.

OK, I can integrate some of this wording in the commit message.

> >  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)?

The thunk sections will currently be consumed by *(.text.*) when using
split sections.  Looking at the assembly for them I think they are
suitable annotated to create the right symbols for livepatch tools to
pick.  They won't however have the right alignment just yet, as I
expect that will get solved with your follow up patch to respect
CONFIG_FUNCTION_ALIGNMENT in assembly annotated functions also.

Thanks, Roger.