[PATCH] livepatch: set -f{function,data}-sections compiler option

[PATCH] livepatch: set -f{function,data}-sections compiler option

[Roger Pau Monne]

If livepatching support is enabled build the hypervisor with
-f{function,data}-sections compiler options, which is required by the
livepatching tools to detect changes and create livepatches.

This shouldn't result in any functional change on the hypervisor
binary image, but does however require some changes in the linker
script in order to handle that each function and data item will now be
placed into its own section in object files. As a result add catch-all
for .text, .data and .bss in order to merge each individual item
section into the final image.

The main difference will be that .text.startup will end up being part
of .text rather than .init, and thus won't be freed. Such section only
seems to appear when using -Os, which not the default for debug or
production binaries.

The benefit of having CONFIG_LIVEPATCH enable those compiler options
is that the livepatch build tools no longer need to fiddle with the
build system in order to enable them. Note the current livepatch tools
are broken after the recent build changes due to the way they
attempt to set  -f{function,data}-sections.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/Makefile           | 4 ++++
 xen/arch/arm/xen.lds.S | 9 +++++++++
 xen/arch/x86/xen.lds.S | 9 +++++++++
 xen/common/Kconfig     | 4 +++-
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/xen/Makefile b/xen/Makefile
index ed4891daf1..bf14a9bdd2 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -269,6 +269,10 @@ else
 CFLAGS += -fomit-frame-pointer
 endif
 
+ifeq ($(CONFIG_LIVEPATCH),y)
+CFLAGS += -ffunction-sections -fdata-sections
+endif
+
 CFLAGS += -nostdinc -fno-builtin -fno-common
 CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
 $(call cc-option-add,CFLAGS,CC,-Wvla)
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 08016948ab..1c7c7d5469 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -33,6 +33,9 @@ SECTIONS
        *(.text)
        *(.text.cold)
        *(.text.unlikely)
+#ifdef CONFIG_LIVEPATCH
+       *(.text.*)
+#endif
        *(.fixup)
        *(.gnu.warning)
        _etext = .;             /* End of text section */
@@ -96,6 +99,9 @@ SECTIONS
 
        *(.data.rel)
        *(.data.rel.*)
+#ifdef CONFIG_LIVEPATCH
+       *(.data.*)
+#endif
        CONSTRUCTORS
   } :text
 
@@ -208,6 +214,9 @@ SECTIONS
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
        *(.bss)
+#ifdef CONFIG_LIVEPATCH
+       *(.bss.*)
+#endif
        . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } :text
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 83def6541e..81bb608151 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -88,6 +88,9 @@ SECTIONS
 
        *(.text.cold)
        *(.text.unlikely)
+#ifdef CONFIG_LIVEPATCH
+       *(.text.*)
+#endif
        *(.fixup)
        *(.gnu.warning)
        _etext = .;             /* End of text section */
@@ -292,6 +295,9 @@ SECTIONS
        *(.data)
        *(.data.rel)
        *(.data.rel.*)
+#ifdef CONFIG_LIVEPATCH
+       *(.data.*)
+#endif
        CONSTRUCTORS
   } PHDR(text)
 
@@ -308,6 +314,9 @@ SECTIONS
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
        *(.bss)
+#ifdef CONFIG_LIVEPATCH
+       *(.bss.*)
+#endif
        . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } PHDR(text)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 6443943889..2423d9f490 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -353,7 +353,9 @@ config CRYPTO
 config LIVEPATCH
 	bool "Live patching support"
 	default X86
-	depends on "$(XEN_HAS_BUILD_ID)" = "y"
+	depends on "$(XEN_HAS_BUILD_ID)" = "y" && \
+	           $(cc-option,-ffunction-sections) && \
+	           $(cc-option,-fdata-sections)
 	---help---
 	  Allows a running Xen hypervisor to be dynamically patched using
 	  binary patches without rebooting. This is primarily used to binarily
-- 
2.34.1

Re: [PATCH] livepatch: set -f{function,data}-sections compiler option

[Jan Beulich]

On 02.03.2022 14:44, Roger Pau Monne wrote:
> If livepatching support is enabled build the hypervisor with
> -f{function,data}-sections compiler options, which is required by the
> livepatching tools to detect changes and create livepatches.
> 
> This shouldn't result in any functional change on the hypervisor
> binary image, but does however require some changes in the linker
> script in order to handle that each function and data item will now be
> placed into its own section in object files. As a result add catch-all
> for .text, .data and .bss in order to merge each individual item
> section into the final image.
> 
> The main difference will be that .text.startup will end up being part
> of .text rather than .init, and thus won't be freed. Such section only
> seems to appear when using -Os, which not the default for debug or
> production binaries.

That's too optimistic a statement imo. I've observed it appear with -Os,
but looking at gcc's gcc/varasm.c:default_function_section() there's
ample room for this appearing for other reasons. Also you don't mention
.text.exit, which will no longer be discarded.

> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -269,6 +269,10 @@ else
>  CFLAGS += -fomit-frame-pointer
>  endif
>  
> +ifeq ($(CONFIG_LIVEPATCH),y)
> +CFLAGS += -ffunction-sections -fdata-sections
> +endif

Perhaps

CFLAGS-$(CONFIG_LIVEPATCH) += -ffunction-sections -fdata-sections

?

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -88,6 +88,9 @@ SECTIONS
>  
>         *(.text.cold)
>         *(.text.unlikely)
> +#ifdef CONFIG_LIVEPATCH
> +       *(.text.*)
> +#endif

This coming after the "cold" and "unlikely" special sections and
ahead of .fixup isn't very nice. Also from looking at the linker
scripts ld supplies I'm getting the impression that there could/
would then also be e.g. .text.cold.* and .text.unlikely.* which
you'd want to separate.

We may want to put the entry point in a special .text.head, put
that first, and then follow ld in putting cold/unlikely stuff ahead
of main .text.

For the reason given in the description I can see why a conditional
is warranted here. But ...

> @@ -292,6 +295,9 @@ SECTIONS
>         *(.data)
>         *(.data.rel)
>         *(.data.rel.*)
> +#ifdef CONFIG_LIVEPATCH
> +       *(.data.*)
> +#endif
>         CONSTRUCTORS
>    } PHDR(text)
>  
> @@ -308,6 +314,9 @@ SECTIONS
>         . = ALIGN(SMP_CACHE_BYTES);
>         __per_cpu_data_end = .;
>         *(.bss)
> +#ifdef CONFIG_LIVEPATCH
> +       *(.bss.*)
> +#endif

... are these two really in need of being conditional?

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -353,7 +353,9 @@ config CRYPTO
>  config LIVEPATCH
>  	bool "Live patching support"
>  	default X86
> -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
> +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && \
> +	           $(cc-option,-ffunction-sections) && \
> +	           $(cc-option,-fdata-sections)

Is this for certain Clang versions? Gcc has been supporting this in
4.1.x already (didn't check when it was introduced).

Jan

Re: [PATCH] livepatch: set -f{function,data}-sections compiler option

[Andrew Cooper]

On 02/03/2022 13:44, Roger Pau Monne wrote:
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 6443943889..2423d9f490 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -353,7 +353,9 @@ config CRYPTO
>  config LIVEPATCH
>  	bool "Live patching support"
>  	default X86
> -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
> +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && \
> +	           $(cc-option,-ffunction-sections) && \
> +	           $(cc-option,-fdata-sections)

cc-option can take multiple, so just $(cc-option,-ffunction-sections
-fdata-sections)

However, we in practice want these in combination with $(LD)
--gc-sections anyway although that wants to be separately configurable.

Therefore, we probably want something like:

config FUNC_SECTIONS
    bool

config LIVEPATCH
    selects FUNC_SECTIONS

or so, so in the future we can add "config LD_GC_SECTIONS" which also
selects FUNC_SECTIONS.

Thoughts?

~Andrew

Re: [PATCH] livepatch: set -f{function,data}-sections compiler option

[Roger Pau Monné]

On Wed, Mar 02, 2022 at 03:41:21PM +0100, Jan Beulich wrote:
> On 02.03.2022 14:44, Roger Pau Monne wrote:
> > If livepatching support is enabled build the hypervisor with
> > -f{function,data}-sections compiler options, which is required by the
> > livepatching tools to detect changes and create livepatches.
> > 
> > This shouldn't result in any functional change on the hypervisor
> > binary image, but does however require some changes in the linker
> > script in order to handle that each function and data item will now be
> > placed into its own section in object files. As a result add catch-all
> > for .text, .data and .bss in order to merge each individual item
> > section into the final image.
> > 
> > The main difference will be that .text.startup will end up being part
> > of .text rather than .init, and thus won't be freed. Such section only
> > seems to appear when using -Os, which not the default for debug or
> > production binaries.
> 
> That's too optimistic a statement imo. I've observed it appear with -Os,
> but looking at gcc's gcc/varasm.c:default_function_section() there's
> ample room for this appearing for other reasons. Also you don't mention
> .text.exit, which will no longer be discarded.
> 
> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -269,6 +269,10 @@ else
> >  CFLAGS += -fomit-frame-pointer
> >  endif
> >  
> > +ifeq ($(CONFIG_LIVEPATCH),y)
> > +CFLAGS += -ffunction-sections -fdata-sections
> > +endif
> 
> Perhaps
> 
> CFLAGS-$(CONFIG_LIVEPATCH) += -ffunction-sections -fdata-sections
> 
> ?

Sure.

> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -88,6 +88,9 @@ SECTIONS
> >  
> >         *(.text.cold)
> >         *(.text.unlikely)
> > +#ifdef CONFIG_LIVEPATCH
> > +       *(.text.*)
> > +#endif
> 
> This coming after the "cold" and "unlikely" special sections and
> ahead of .fixup isn't very nice. Also from looking at the linker
> scripts ld supplies I'm getting the impression that there could/
> would then also be e.g. .text.cold.* and .text.unlikely.* which
> you'd want to separate.
> 
> We may want to put the entry point in a special .text.head, put
> that first, and then follow ld in putting cold/unlikely stuff ahead
> of main .text.

I can give that a try.

> For the reason given in the description I can see why a conditional
> is warranted here. But ...
> 
> > @@ -292,6 +295,9 @@ SECTIONS
> >         *(.data)
> >         *(.data.rel)
> >         *(.data.rel.*)
> > +#ifdef CONFIG_LIVEPATCH
> > +       *(.data.*)
> > +#endif
> >         CONSTRUCTORS
> >    } PHDR(text)
> >  
> > @@ -308,6 +314,9 @@ SECTIONS
> >         . = ALIGN(SMP_CACHE_BYTES);
> >         __per_cpu_data_end = .;
> >         *(.bss)
> > +#ifdef CONFIG_LIVEPATCH
> > +       *(.bss.*)
> > +#endif
> 
> ... are these two really in need of being conditional?

Will drop if you agree. I didn't want to risk introducing unwanted
changes in the !CONFIG_LIVEPATCH case.

> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -353,7 +353,9 @@ config CRYPTO
> >  config LIVEPATCH
> >  	bool "Live patching support"
> >  	default X86
> > -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
> > +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && \
> > +	           $(cc-option,-ffunction-sections) && \
> > +	           $(cc-option,-fdata-sections)
> 
> Is this for certain Clang versions? Gcc has been supporting this in
> 4.1.x already (didn't check when it was introduced).

I've checked clang and it seems to be prevent in at least Clang 5,
which is likely enough?

I've added the check just to be on the safe side.

Thanks, Roger.

Re: [PATCH] livepatch: set -f{function,data}-sections compiler option

[Jan Beulich]

On 02.03.2022 16:46, Roger Pau Monné wrote:
> On Wed, Mar 02, 2022 at 03:41:21PM +0100, Jan Beulich wrote:
>> On 02.03.2022 14:44, Roger Pau Monne wrote:
>>> @@ -292,6 +295,9 @@ SECTIONS
>>>         *(.data)
>>>         *(.data.rel)
>>>         *(.data.rel.*)
>>> +#ifdef CONFIG_LIVEPATCH
>>> +       *(.data.*)
>>> +#endif
>>>         CONSTRUCTORS
>>>    } PHDR(text)
>>>  
>>> @@ -308,6 +314,9 @@ SECTIONS
>>>         . = ALIGN(SMP_CACHE_BYTES);
>>>         __per_cpu_data_end = .;
>>>         *(.bss)
>>> +#ifdef CONFIG_LIVEPATCH
>>> +       *(.bss.*)
>>> +#endif
>>
>> ... are these two really in need of being conditional?
> 
> Will drop if you agree. I didn't want to risk introducing unwanted
> changes in the !CONFIG_LIVEPATCH case.

The only "unwanted" change I can imagine here would be that we place a
section which the linker would otherwise need to guess how to place,
for being "orphan".

>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -353,7 +353,9 @@ config CRYPTO
>>>  config LIVEPATCH
>>>  	bool "Live patching support"
>>>  	default X86
>>> -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
>>> +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && \
>>> +	           $(cc-option,-ffunction-sections) && \
>>> +	           $(cc-option,-fdata-sections)
>>
>> Is this for certain Clang versions? Gcc has been supporting this in
>> 4.1.x already (didn't check when it was introduced).
> 
> I've checked clang and it seems to be prevent in at least Clang 5,
> which is likely enough?

Clang5 accepts the options fine here. But that wouldn't be enough,
./README says "Clang/LLVM 3.5 or later".

> I've added the check just to be on the safe side.

Well, yes, if you're unsure and the old version can't be checked,
then perhaps indeed better to probe.

Jan

Re: [PATCH] livepatch: set -f{function,data}-sections compiler option

[Jan Beulich]

On 02.03.2022 15:41, Jan Beulich wrote:
> On 02.03.2022 14:44, Roger Pau Monne wrote:
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -88,6 +88,9 @@ SECTIONS
>>  
>>         *(.text.cold)
>>         *(.text.unlikely)
>> +#ifdef CONFIG_LIVEPATCH
>> +       *(.text.*)
>> +#endif
> 
> This coming after the "cold" and "unlikely" special sections and
> ahead of .fixup isn't very nice. Also from looking at the linker
> scripts ld supplies I'm getting the impression that there could/
> would then also be e.g. .text.cold.* and .text.unlikely.* which
> you'd want to separate.

Thinking about it, with -ffunction-sections startup code cannot
reasonably be placed in .text.startup, or else there would be
confusion with code originating from a function named startup().
But of course .text.startup.* is similarly difficult to arrange
for not ending up in the output's main .text, so adding
.text.startup.* and .text.exit.* later in the script wouldn't
gain us anything.

Jan

Re: [PATCH] livepatch: set -f{function,data}-sections compiler option

[Roger Pau Monné]

On Wed, Mar 02, 2022 at 03:35:07PM +0000, Andrew Cooper wrote:
> On 02/03/2022 13:44, Roger Pau Monne wrote:
> > diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> > index 6443943889..2423d9f490 100644
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -353,7 +353,9 @@ config CRYPTO
> >  config LIVEPATCH
> >  	bool "Live patching support"
> >  	default X86
> > -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
> > +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && \
> > +	           $(cc-option,-ffunction-sections) && \
> > +	           $(cc-option,-fdata-sections)
> 
> cc-option can take multiple, so just $(cc-option,-ffunction-sections
> -fdata-sections)
> 
> However, we in practice want these in combination with $(LD)
> --gc-sections anyway although that wants to be separately configurable.
> 
> Therefore, we probably want something like:
> 
> config FUNC_SECTIONS
>     bool
> 
> config LIVEPATCH
>     selects FUNC_SECTIONS
> 
> or so, so in the future we can add "config LD_GC_SECTIONS" which also
> selects FUNC_SECTIONS.
> 
> Thoughts?

Do we want separate options for ffunction-sections and fdata-sections
options, or is FUNC_SECTIONS supposed to cover them both?

I assume you are fine with Jan's suggestion to not check for the
option presence, since it should be in all supported versions.

Thanks, Roger.

Re: [PATCH] livepatch: set -f{function,data}-sections compiler option

[Roger Pau Monné]

On Wed, Mar 02, 2022 at 05:09:10PM +0100, Jan Beulich wrote:
> On 02.03.2022 16:46, Roger Pau Monné wrote:
> > On Wed, Mar 02, 2022 at 03:41:21PM +0100, Jan Beulich wrote:
> >> On 02.03.2022 14:44, Roger Pau Monne wrote:
> >>> @@ -292,6 +295,9 @@ SECTIONS
> >>>         *(.data)
> >>>         *(.data.rel)
> >>>         *(.data.rel.*)
> >>> +#ifdef CONFIG_LIVEPATCH
> >>> +       *(.data.*)
> >>> +#endif
> >>>         CONSTRUCTORS
> >>>    } PHDR(text)
> >>>  
> >>> @@ -308,6 +314,9 @@ SECTIONS
> >>>         . = ALIGN(SMP_CACHE_BYTES);
> >>>         __per_cpu_data_end = .;
> >>>         *(.bss)
> >>> +#ifdef CONFIG_LIVEPATCH
> >>> +       *(.bss.*)
> >>> +#endif
> >>
> >> ... are these two really in need of being conditional?
> > 
> > Will drop if you agree. I didn't want to risk introducing unwanted
> > changes in the !CONFIG_LIVEPATCH case.
> 
> The only "unwanted" change I can imagine here would be that we place a
> section which the linker would otherwise need to guess how to place,
> for being "orphan".
> 
> >>> --- a/xen/common/Kconfig
> >>> +++ b/xen/common/Kconfig
> >>> @@ -353,7 +353,9 @@ config CRYPTO
> >>>  config LIVEPATCH
> >>>  	bool "Live patching support"
> >>>  	default X86
> >>> -	depends on "$(XEN_HAS_BUILD_ID)" = "y"
> >>> +	depends on "$(XEN_HAS_BUILD_ID)" = "y" && \
> >>> +	           $(cc-option,-ffunction-sections) && \
> >>> +	           $(cc-option,-fdata-sections)
> >>
> >> Is this for certain Clang versions? Gcc has been supporting this in
> >> 4.1.x already (didn't check when it was introduced).
> > 
> > I've checked clang and it seems to be prevent in at least Clang 5,
> > which is likely enough?
> 
> Clang5 accepts the options fine here. But that wouldn't be enough,
> ./README says "Clang/LLVM 3.5 or later".
> 
> > I've added the check just to be on the safe side.
> 
> Well, yes, if you're unsure and the old version can't be checked,
> then perhaps indeed better to probe.

OK, so I've managed to probe clang 3.5.0 and it does support
-f{function,data}-sections so we can drop the check.

Thanks, Roger.