[PATCH 0/4] livepatch-build-tools: fix for livepatch hooks

[PATCH 0/4] livepatch-build-tools: fix for livepatch hooks

[Roger Pau Monne]

Hello,

First two patches in the series are misc (IMO trivial) fixes.  Last two
patches fix the usage of hooks.

Thanks, Roger.

Roger Pau Monne (4):
  livepatch-build: allow patch file name sizes up to 127 characters
  create-diff-object: update default alt_instr size
  create-diff-object: don't include symbols for .livepatch.hooks.*
    sections
  create-diff-object: also include relas that point to changed sections

 create-diff-object.c | 14 +++++++++-----
 livepatch-build      |  5 +++--
 2 files changed, 12 insertions(+), 7 deletions(-)

-- 
2.46.0

[PATCH 1/4] livepatch-build: allow patch file name sizes up to 127 characters

[Roger Pau Monne]

XenServer uses quite long Xen version names, and encode such in the livepatch
filename, and it's currently running out of space in the file name.

Bump max filename size to 127, so it also matches the patch name length in the
hypervisor interface.  Note the size of the buffer is 128 characters, and the
last one is reserved for the null terminator.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Take into account the null terminator.
---
 livepatch-build | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/livepatch-build b/livepatch-build
index 948b2acfc2f6..f3ca9399d149 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -72,8 +72,9 @@ function make_patch_name()
     fi
 
     # Only allow alphanumerics and '_' and '-' in the patch name.  Everything
-    # else is replaced with '-'.  Truncate to 48 chars.
-    echo ${PATCHNAME//[^a-zA-Z0-9_-]/-} |cut -c 1-48
+    # else is replaced with '-'.  Truncate to 127 chars
+    # (XEN_LIVEPATCH_NAME_SIZE - 1).
+    echo ${PATCHNAME//[^a-zA-Z0-9_-]/-} |cut -c -127
 }
 
 # Do a full normal build
-- 
2.46.0

Re: [PATCH 1/4] livepatch-build: allow patch file name sizes up to 127 characters

[Ross Lagerwall]

On Thu, Nov 7, 2024 at 3:15 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> XenServer uses quite long Xen version names, and encode such in the livepatch
> filename, and it's currently running out of space in the file name.
>
> Bump max filename size to 127, so it also matches the patch name length in the
> hypervisor interface.  Note the size of the buffer is 128 characters, and the
> last one is reserved for the null terminator.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - Take into account the null terminator.
> ---
>  livepatch-build | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/livepatch-build b/livepatch-build
> index 948b2acfc2f6..f3ca9399d149 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -72,8 +72,9 @@ function make_patch_name()
>      fi
>
>      # Only allow alphanumerics and '_' and '-' in the patch name.  Everything
> -    # else is replaced with '-'.  Truncate to 48 chars.
> -    echo ${PATCHNAME//[^a-zA-Z0-9_-]/-} |cut -c 1-48
> +    # else is replaced with '-'.  Truncate to 127 chars
> +    # (XEN_LIVEPATCH_NAME_SIZE - 1).
> +    echo ${PATCHNAME//[^a-zA-Z0-9_-]/-} |cut -c -127
>  }

I think this 48 char limit erroneously came from kpatch / Linux's module
name length limit and is therefore not relevant.

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Thanks

[PATCH 2/4] create-diff-object: update default alt_instr size

[Roger Pau Monne]

The size of the alt_instr structure in Xen is 14 instead of 12 bytes, adjust
it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 create-diff-object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/create-diff-object.c b/create-diff-object.c
index fed360a9aa68..d8a2afbf2774 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1000,7 +1000,7 @@ static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
 	char *str;
 	if (!size) {
 		str = getenv("ALT_STRUCT_SIZE");
-		size = str ? atoi(str) : 12;
+		size = str ? atoi(str) : 14;
 	}
 
 	log_debug("altinstr_size=%d\n", size);
-- 
2.46.0

Re: [PATCH 2/4] create-diff-object: update default alt_instr size

[Jan Beulich]

On 07.11.2024 16:15, Roger Pau Monne wrote:
> The size of the alt_instr structure in Xen is 14 instead of 12 bytes, adjust
> it.

Nowadays yes. Isn't the tool supposed to be usable with all livepatch-capable
Xen versions, though? As a random data point, 4.7 still had the size at 12.

Jan

Re: [PATCH 2/4] create-diff-object: update default alt_instr size

[Roger Pau Monné]

On Thu, Nov 07, 2024 at 04:20:35PM +0100, Jan Beulich wrote:
> On 07.11.2024 16:15, Roger Pau Monne wrote:
> > The size of the alt_instr structure in Xen is 14 instead of 12 bytes, adjust
> > it.
> 
> Nowadays yes. Isn't the tool supposed to be usable with all livepatch-capable
> Xen versions, though? As a random data point, 4.7 still had the size at 12.

Yes, livepatch-build-tools will fetch the alt_intsrt size from the
DWARF info.

However when using create-diff-object without the env variable being
set a default value is used, and that's currently 12.  I think it
would be best to update that default to the value used by the current
Xen version.

Thanks, Roger.

Re: [PATCH 2/4] create-diff-object: update default alt_instr size

[Jan Beulich]

On 07.11.2024 16:55, Roger Pau Monné wrote:
> On Thu, Nov 07, 2024 at 04:20:35PM +0100, Jan Beulich wrote:
>> On 07.11.2024 16:15, Roger Pau Monne wrote:
>>> The size of the alt_instr structure in Xen is 14 instead of 12 bytes, adjust
>>> it.
>>
>> Nowadays yes. Isn't the tool supposed to be usable with all livepatch-capable
>> Xen versions, though? As a random data point, 4.7 still had the size at 12.
> 
> Yes, livepatch-build-tools will fetch the alt_intsrt size from the
> DWARF info.
> 
> However when using create-diff-object without the env variable being
> set a default value is used, and that's currently 12.  I think it
> would be best to update that default to the value used by the current
> Xen version.

Oh, I see. Makes sense of course.

Jan

Re: [PATCH 2/4] create-diff-object: update default alt_instr size

[Ross Lagerwall]

On Thu, Nov 7, 2024 at 3:15 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> The size of the alt_instr structure in Xen is 14 instead of 12 bytes, adjust
> it.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  create-diff-object.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/create-diff-object.c b/create-diff-object.c
> index fed360a9aa68..d8a2afbf2774 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -1000,7 +1000,7 @@ static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
>         char *str;
>         if (!size) {
>                 str = getenv("ALT_STRUCT_SIZE");
> -               size = str ? atoi(str) : 12;
> +               size = str ? atoi(str) : 14;
>         }
>
>         log_debug("altinstr_size=%d\n", size);
> --
> 2.46.0
>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

[PATCH 3/4] create-diff-object: don't include symbols for .livepatch.hooks.* sections

[Roger Pau Monne]

Not all toolchains generate symbols for the .livepatch.hooks.* sections,
neither those symbols are required by the livepatch loading logic in Xen to
find and process the hooks.  Hooks in livepatch payloads are found and
processed based exclusively on section data.

The unconditional attempt to expect each hook serction to have a matching
symbol leads to a segmentation fault in create-diff-object when such symbol is
not present, as the code references a NULL pointer.

Fix this by not attempting to include symbols associated with hook sections.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 create-diff-object.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/create-diff-object.c b/create-diff-object.c
index d8a2afbf2774..924059a1842b 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1555,8 +1555,6 @@ static int kpatch_include_hook_elements(struct kpatch_elf *kelf)
 				sym->sec->sym = NULL;
 				/* use section symbol instead */
 				rela->sym = sym->sec->secsym;
-			} else {
-				sec->secsym->include = 1;
 			}
 		}
 	}
-- 
2.46.0

Re: [PATCH 3/4] create-diff-object: don't include symbols for .livepatch.hooks.* sections

[Ross Lagerwall]

On Thu, Nov 7, 2024 at 3:15 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> Not all toolchains generate symbols for the .livepatch.hooks.* sections,
> neither those symbols are required by the livepatch loading logic in Xen to
> find and process the hooks.  Hooks in livepatch payloads are found and
> processed based exclusively on section data.
>
> The unconditional attempt to expect each hook serction to have a matching
> symbol leads to a segmentation fault in create-diff-object when such symbol is
> not present, as the code references a NULL pointer.
>
> Fix this by not attempting to include symbols associated with hook sections.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  create-diff-object.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/create-diff-object.c b/create-diff-object.c
> index d8a2afbf2774..924059a1842b 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -1555,8 +1555,6 @@ static int kpatch_include_hook_elements(struct kpatch_elf *kelf)
>                                 sym->sec->sym = NULL;
>                                 /* use section symbol instead */
>                                 rela->sym = sym->sec->secsym;
> -                       } else {
> -                               sec->secsym->include = 1;
>                         }
>                 }
>         }
> --
> 2.46.0
>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Thanks

[PATCH 4/4] create-diff-object: also include relas that point to changed sections

[Roger Pau Monne]

create-diff-object has a special handling for some specific sections, like
.altinstructions or .livepatch.hooks.*.  The contents of those sections are in
the form of array elements, where each element can be processed independently
of the rest.  For example an element in .altinstructions is a set of
replacement coordinates, with the layout specified by the alt_instr struct.  In
the case of .livepatch.hooks.* each element is a pointer to a hook function to
call.

The contents of this array is processed element wise, so that
create-diff-object can decide whether the element relates to the content in the
livepatch and thus needs keeping.  Such relation is driven based on the
contents of the relocations for the special sections.  If a relocation to be
applied to a special section element depends on any symbol to be included in
the livepatch then the special element is also considered required and thus
added to the livepatch contents.

However relocations don't always reference function type symbols, they can also
reference sections type symbols, and that's usually the case with hook symbols
that have relocations based on section symbols, as an example:

RELOCATION RECORDS FOR [.livepatch.hooks.load]:
OFFSET           TYPE              VALUE
0000000000000000 R_X86_64_64       .text.foobar

Symbol information for .text.foobar:

0000000000000000 l    d  .text.foobar      0000000000000000 .text.foobar

As seen above, the .livepatch.hooks.load relocation uses a non-function symbol,
which given the current code in should_keep_rela_group() would mean it's not
considered for inclusion in the livepatch.

Fix this by allowing should_keep_rela_group() to also keep relocations if they
either point to function or section symbols.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 create-diff-object.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/create-diff-object.c b/create-diff-object.c
index 924059a1842b..c21cc576052a 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1158,11 +1158,17 @@ static int should_keep_rela_group(struct section *sec, int start, int size)
 	struct rela *rela;
 	int found = 0;
 
-	/* check if any relas in the group reference any changed functions */
+	/*
+	 * Check if any relas in the group reference any changed functions or
+	 * sections.  As seen by hook related relocations (.livepatch.hooks.*),
+	 * it's possible they use the section symbol as a reference rather than
+	 * the function symbol.
+	 */
 	list_for_each_entry(rela, &sec->relas, list) {
 		if (rela->offset >= start &&
 		    rela->offset < start + size &&
-		    rela->sym->type == STT_FUNC &&
+		    (rela->sym->type == STT_FUNC ||
+		     rela->sym->type == STT_SECTION) &&
 		    rela->sym->sec->include) {
 			found = 1;
 			log_debug("new/changed symbol %s found in special section %s\n",
-- 
2.46.0

Re: [PATCH 4/4] create-diff-object: also include relas that point to changed sections

[Ross Lagerwall]

On Thu, Nov 7, 2024 at 3:15 PM Roger Pau Monne <roger.pau@citrix.com> wrote:
>
> create-diff-object has a special handling for some specific sections, like
> .altinstructions or .livepatch.hooks.*.  The contents of those sections are in
> the form of array elements, where each element can be processed independently
> of the rest.  For example an element in .altinstructions is a set of
> replacement coordinates, with the layout specified by the alt_instr struct.  In
> the case of .livepatch.hooks.* each element is a pointer to a hook function to
> call.
>
> The contents of this array is processed element wise, so that
> create-diff-object can decide whether the element relates to the content in the
> livepatch and thus needs keeping.  Such relation is driven based on the
> contents of the relocations for the special sections.  If a relocation to be
> applied to a special section element depends on any symbol to be included in
> the livepatch then the special element is also considered required and thus
> added to the livepatch contents.
>
> However relocations don't always reference function type symbols, they can also
> reference sections type symbols, and that's usually the case with hook symbols
> that have relocations based on section symbols, as an example:
>
> RELOCATION RECORDS FOR [.livepatch.hooks.load]:
> OFFSET           TYPE              VALUE
> 0000000000000000 R_X86_64_64       .text.foobar
>
> Symbol information for .text.foobar:
>
> 0000000000000000 l    d  .text.foobar      0000000000000000 .text.foobar
>
> As seen above, the .livepatch.hooks.load relocation uses a non-function symbol,
> which given the current code in should_keep_rela_group() would mean it's not
> considered for inclusion in the livepatch.
>
> Fix this by allowing should_keep_rela_group() to also keep relocations if they
> either point to function or section symbols.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  create-diff-object.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 924059a1842b..c21cc576052a 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -1158,11 +1158,17 @@ static int should_keep_rela_group(struct section *sec, int start, int size)
>         struct rela *rela;
>         int found = 0;
>
> -       /* check if any relas in the group reference any changed functions */
> +       /*
> +        * Check if any relas in the group reference any changed functions or
> +        * sections.  As seen by hook related relocations (.livepatch.hooks.*),
> +        * it's possible they use the section symbol as a reference rather than
> +        * the function symbol.
> +        */
>         list_for_each_entry(rela, &sec->relas, list) {
>                 if (rela->offset >= start &&
>                     rela->offset < start + size &&
> -                   rela->sym->type == STT_FUNC &&
> +                   (rela->sym->type == STT_FUNC ||
> +                    rela->sym->type == STT_SECTION) &&
>                     rela->sym->sec->include) {
>                         found = 1;
>                         log_debug("new/changed symbol %s found in special section %s\n",
> --
> 2.46.0
>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Thanks