All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements
@ 2024-04-29 14:56 Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 1/9] livepatch-build-tools: allow patch file name sizes up to 127 characters Roger Pau Monne
                   ` (8 more replies)
  0 siblings, 9 replies; 10+ messages in thread
From: Roger Pau Monne @ 2024-04-29 14:56 UTC (permalink / raw
  To: xen-devel; +Cc: konrad.wilk, ross.lagerwall, Roger Pau Monne

Hello,

The first 3 patches in the series attempt to fix some bugs, I don't
think they will be specially controversial or difficult to review (patch
1 was already posted standalone).

The rest of the patches are more convoluted, as they attempt to solve
some shortcomings when attempting to create livepatches that change
alternatives or exceptions.  For example a patch that only changes
content in alternative blocks (the code that ends up in the
.altinstr_replacement section) won't work, as create-diff-object will
report that there are no changes.

I've attempted to test as many corner cases as I could come up with, but
it's hard to figure all the possible changes, plus it's also fairly easy
to inadvertently regress existing functionality.

Thanks, Roger.

Roger Pau Monne (9):
  livepatch-build-tools: allow patch file name sizes up to 127
    characters
  create-diff-object: update default alt_instr size
  livepatch-build: fix detection of structure sizes
  create-diff-object: document kpatch_regenerate_special_section()
    behavior
  create-diff-object: move kpatch_include_symbol()
  create-diff-object: detect changes in .altinstr_replacement and .fixup
    sections
  create-diff-object: don't account for changes to .bug_frame.? sections
  create-diff-object: account for changes in the special section itself
  create-diff-object: account for special section changes

 create-diff-object.c | 196 +++++++++++++++++++++++++++++++++----------
 livepatch-build      |  12 ++-
 2 files changed, 161 insertions(+), 47 deletions(-)

-- 
2.44.0



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/9] livepatch-build-tools: allow patch file name sizes up to 127 characters
  2024-04-29 14:56 [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements Roger Pau Monne
@ 2024-04-29 14:56 ` Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 2/9] create-diff-object: update default alt_instr size Roger Pau Monne
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monne @ 2024-04-29 14:56 UTC (permalink / raw
  To: xen-devel; +Cc: konrad.wilk, ross.lagerwall, 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 character, 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.44.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/9] create-diff-object: update default alt_instr size
  2024-04-29 14:56 [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 1/9] livepatch-build-tools: allow patch file name sizes up to 127 characters Roger Pau Monne
@ 2024-04-29 14:56 ` Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 3/9] livepatch-build: fix detection of structure sizes Roger Pau Monne
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monne @ 2024-04-29 14:56 UTC (permalink / raw
  To: xen-devel; +Cc: konrad.wilk, ross.lagerwall, 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.44.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/9] livepatch-build: fix detection of structure sizes
  2024-04-29 14:56 [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 1/9] livepatch-build-tools: allow patch file name sizes up to 127 characters Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 2/9] create-diff-object: update default alt_instr size Roger Pau Monne
@ 2024-04-29 14:56 ` Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 4/9] create-diff-object: document kpatch_regenerate_special_section() behavior Roger Pau Monne
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monne @ 2024-04-29 14:56 UTC (permalink / raw
  To: xen-devel; +Cc: konrad.wilk, ross.lagerwall, Roger Pau Monne

The current runes assume that in the list of DWARF tags DW_AT_byte_size will
come after DW_AT_name, but that's not always the case.  On one of my builds
I've seen:

    <b618>   DW_AT_name        : (indirect string, offset: 0x3c45): exception_table_entry
    <b61c>   DW_AT_declaration : 1
 <1><b61c>: Abbrev Number: 5 (DW_TAG_const_type)
    <b61d>   DW_AT_type        : <0xb617>
 <1><b621>: Abbrev Number: 14 (DW_TAG_pointer_type)
    <b622>   DW_AT_byte_size   : 8

Instead of assuming such order, explicitly search for the DW_AT_byte_size tag
when a match in the DW_AT_name one is found.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
All this ad hoc parsing of DWARF data seems very fragile.  This is an
improvement over the current logic, but I would still prefer if we could find a
more reliable way to obtain the struct sizes we need.
---
 livepatch-build | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/livepatch-build b/livepatch-build
index f3ca9399d149..aad9849f2ba9 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -427,9 +427,16 @@ if [ "${SKIP}" != "build" ]; then
     SPECIAL_VARS=$(readelf -wi "$OUTPUT/xen-syms" |
                awk '
                BEGIN { a = b = e = 0 }
+               # Ensure no fall through to the next tag without getting the size
+               (a == 1 || b == 1 || e == 1) && /DW_AT_name/ \
+                   {print "can not get special structure size" > "/dev/stderr"; exit 1}
                a == 0 && /DW_AT_name.* alt_instr/ {a = 1; next}
                b == 0 && /DW_AT_name.* bug_frame/ {b = 1; next}
                e == 0 && /DW_AT_name.* exception_table_entry/ {e = 1; next}
+               # Seek the line that contains the size
+               a == 1 && !/DW_AT_byte_size/ {next}
+               b == 1 && !/DW_AT_byte_size/ {next}
+               e == 1 && !/DW_AT_byte_size/ {next}
                # Use shell printf to (possibly) convert from hex to decimal
                a == 1 {printf("export ALT_STRUCT_SIZE=`printf \"%%d\" \"%s\"`\n", $4); a = 2}
                b == 1 {printf("export BUG_STRUCT_SIZE=`printf \"%%d\" \"%s\"`\n", $4); b = 2}
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/9] create-diff-object: document kpatch_regenerate_special_section() behavior
  2024-04-29 14:56 [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements Roger Pau Monne
                   ` (2 preceding siblings ...)
  2024-04-29 14:56 ` [PATCH 3/9] livepatch-build: fix detection of structure sizes Roger Pau Monne
@ 2024-04-29 14:56 ` Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 5/9] create-diff-object: move kpatch_include_symbol() Roger Pau Monne
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monne @ 2024-04-29 14:56 UTC (permalink / raw
  To: xen-devel; +Cc: konrad.wilk, ross.lagerwall, Roger Pau Monne

The purpose of kpatch_regenerate_special_section() is fairly opaque without
spending a good amount of time and having quite a lot of knowledge about what
the special sections contains.

Introduce some comments in order to give context and describe the expected
functionality.

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

diff --git a/create-diff-object.c b/create-diff-object.c
index d8a2afbf2774..6a751bf3b789 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1210,6 +1210,12 @@ static void kpatch_regenerate_special_section(struct kpatch_elf *kelf,
 
 	src_offset = 0;
 	dest_offset = 0;
+
+	/*
+	 * Special sections processed here are array objects, hence in order to
+	 * detect whether a special section needs processing attempt to get the
+	 * element size.  Returning a size of 0 means no processing required.
+	 */
 	group_size = special->group_size(kelf, src_offset);
 	if (group_size == 0) {
 		log_normal("Skipping regeneration of a special section: %s\n",
@@ -1246,6 +1252,33 @@ static void kpatch_regenerate_special_section(struct kpatch_elf *kelf,
 		if (src_offset + group_size > sec->base->sh.sh_size)
 			group_size = sec->base->sh.sh_size - src_offset;
 
+		/*
+		 * Special sections handled perform a bunch of different tasks,
+		 * but they all have something in common: they are array like
+		 * sections that reference functions in the object file being
+		 * processed.
+		 *
+		 * .bug_frames.* relocations reference the symbol (plus offset)
+		 * where the exception is triggered from.
+		 *
+		 * .altinstructions relocations contain references to
+		 * coordinates where the alternatives are to be applied, plus
+		 * coordinates that point to the replacement code in
+		 * .altinstr_replacement.
+		 *
+		 * .ex_table relocations contain references to the coordinates
+		 * where the fixup code should be executed, plus relocation
+		 * coordinates that point to the text code to execte living in
+		 * the .fixup section.
+		 *
+		 * .livepatch.hooks.* relocations point to the hook
+		 * functions.
+		 *
+		 * Such dependencies allow to make a decision on whether an
+		 * element in the array needs including in the livepatch: if
+		 * the symbol pointed by the relocation is new or has changed
+		 * the array element needs including.
+		 */
 		include = should_keep_rela_group(sec, src_offset, group_size);
 
 		if (!include)
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 5/9] create-diff-object: move kpatch_include_symbol()
  2024-04-29 14:56 [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements Roger Pau Monne
                   ` (3 preceding siblings ...)
  2024-04-29 14:56 ` [PATCH 4/9] create-diff-object: document kpatch_regenerate_special_section() behavior Roger Pau Monne
@ 2024-04-29 14:56 ` Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 6/9] create-diff-object: detect changes in .altinstr_replacement and .fixup sections Roger Pau Monne
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monne @ 2024-04-29 14:56 UTC (permalink / raw
  To: xen-devel; +Cc: konrad.wilk, ross.lagerwall, Roger Pau Monne

So it can be used by kpatch_process_special_sections() in further changes.

Non functional change.

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

diff --git a/create-diff-object.c b/create-diff-object.c
index 6a751bf3b789..7674d972e301 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1198,6 +1198,43 @@ void kpatch_update_ex_table_addend(struct kpatch_elf *kelf,
 	}
 }
 
+#define inc_printf(fmt, ...) \
+	log_debug("%*s" fmt, recurselevel, "", ##__VA_ARGS__);
+
+static void kpatch_include_symbol(struct symbol *sym, int recurselevel)
+{
+	struct rela *rela;
+	struct section *sec;
+
+	inc_printf("start include_symbol(%s)\n", sym->name);
+	sym->include = 1;
+	inc_printf("symbol %s is included\n", sym->name);
+	/*
+	 * Check if sym is a non-local symbol (sym->sec is NULL) or
+	 * if an unchanged local symbol.  This a base case for the
+	 * inclusion recursion.
+	 */
+	if (!sym->sec || sym->sec->include ||
+	    (sym->type != STT_SECTION && sym->status == SAME))
+		goto out;
+	sec = sym->sec;
+	sec->include = 1;
+	inc_printf("section %s is included\n", sec->name);
+	if (sec->secsym && sec->secsym != sym) {
+		sec->secsym->include = 1;
+		inc_printf("section symbol %s is included\n", sec->secsym->name);
+	}
+	if (!sec->rela)
+		goto out;
+	sec->rela->include = 1;
+	inc_printf("section %s is included\n", sec->rela->name);
+	list_for_each_entry(rela, &sec->rela->relas, list)
+		kpatch_include_symbol(rela->sym, recurselevel+1);
+out:
+	inc_printf("end include_symbol(%s)\n", sym->name);
+	return;
+}
+
 static void kpatch_regenerate_special_section(struct kpatch_elf *kelf,
 				              struct special_section *special,
 				              struct section *sec)
@@ -1455,43 +1492,6 @@ static void kpatch_include_standard_string_elements(struct kpatch_elf *kelf)
 	}
 }
 
-#define inc_printf(fmt, ...) \
-	log_debug("%*s" fmt, recurselevel, "", ##__VA_ARGS__);
-
-static void kpatch_include_symbol(struct symbol *sym, int recurselevel)
-{
-	struct rela *rela;
-	struct section *sec;
-
-	inc_printf("start include_symbol(%s)\n", sym->name);
-	sym->include = 1;
-	inc_printf("symbol %s is included\n", sym->name);
-	/*
-	 * Check if sym is a non-local symbol (sym->sec is NULL) or
-	 * if an unchanged local symbol.  This a base case for the
-	 * inclusion recursion.
-	 */
-	if (!sym->sec || sym->sec->include ||
-	    (sym->type != STT_SECTION && sym->status == SAME))
-		goto out;
-	sec = sym->sec;
-	sec->include = 1;
-	inc_printf("section %s is included\n", sec->name);
-	if (sec->secsym && sec->secsym != sym) {
-		sec->secsym->include = 1;
-		inc_printf("section symbol %s is included\n", sec->secsym->name);
-	}
-	if (!sec->rela)
-		goto out;
-	sec->rela->include = 1;
-	inc_printf("section %s is included\n", sec->rela->name);
-	list_for_each_entry(rela, &sec->rela->relas, list)
-		kpatch_include_symbol(rela->sym, recurselevel+1);
-out:
-	inc_printf("end include_symbol(%s)\n", sym->name);
-	return;
-}
-
 static int kpatch_include_changed_functions(struct kpatch_elf *kelf)
 {
 	struct symbol *sym;
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 6/9] create-diff-object: detect changes in .altinstr_replacement and .fixup sections
  2024-04-29 14:56 [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements Roger Pau Monne
                   ` (4 preceding siblings ...)
  2024-04-29 14:56 ` [PATCH 5/9] create-diff-object: move kpatch_include_symbol() Roger Pau Monne
@ 2024-04-29 14:56 ` Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 7/9] create-diff-object: don't account for changes to .bug_frame.? sections Roger Pau Monne
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monne @ 2024-04-29 14:56 UTC (permalink / raw
  To: xen-devel; +Cc: konrad.wilk, ross.lagerwall, Roger Pau Monne

The current logic to detect changes in special sections elements will only
include group elements that reference function symbols that need including
(either because they have changed or are new).

This works fine in order to detect when a special section element needs
including because of the function is points has changed or is newly introduced,
but doesn't detect changes to the replacement code in .altinstr_replacement or
.fixup sections, as the relocations in that case are against STT_SECTION
symbols.

Fix this by also including the special section group if the symbol the
relocations points to is of type section.

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

diff --git a/create-diff-object.c b/create-diff-object.c
index 7674d972e301..f4e4da063d0a 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1158,11 +1158,21 @@ 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.
+	 *
+	 * Relocations in the .altinstructions and .ex_table special sections
+	 * can also reference changed section symbols, since the replacement
+	 * text in both cases resides on a section that has no function symbols
+	 * (.altinstr_replacement and .fixup respectively).  Account for that
+	 * and also check whether the referenced symbols are section ones in
+	 * order to decide whether the relocation group needs including.
+	 */
 	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",
@@ -1338,6 +1348,21 @@ static void kpatch_regenerate_special_section(struct kpatch_elf *kelf,
 
 				rela->sym->include = 1;
 
+				/*
+				 * Changes that cause only the
+				 * .altinstr_replacement or .fixup sections to
+				 * be modified need the target function of the
+				 * replacement to also be marked as CHANGED,
+				 * otherwise livepatch won't replace the
+				 * function, and the new replacement code won't
+				 * take effect.
+				 */
+				if (rela->sym->type == STT_FUNC &&
+				    rela->sym->status == SAME) {
+					rela->sym->status = CHANGED;
+					kpatch_include_symbol(rela->sym, 0);
+				}
+
 				if (!strcmp(special->name, ".fixup"))
 					kpatch_update_ex_table_addend(kelf, special,
 								      src_offset,
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 7/9] create-diff-object: don't account for changes to .bug_frame.? sections
  2024-04-29 14:56 [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements Roger Pau Monne
                   ` (5 preceding siblings ...)
  2024-04-29 14:56 ` [PATCH 6/9] create-diff-object: detect changes in .altinstr_replacement and .fixup sections Roger Pau Monne
@ 2024-04-29 14:56 ` Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 8/9] create-diff-object: account for changes in the special section itself Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 9/9] create-diff-object: account for special section changes Roger Pau Monne
  8 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monne @ 2024-04-29 14:56 UTC (permalink / raw
  To: xen-devel; +Cc: konrad.wilk, ross.lagerwall, Roger Pau Monne

bug_frame related sections exclusively contain addresses that reference back to
the address where the BUG_INSTR is executed.  As such, any change to the
contents of bug_frame sections (or it's relocations) will necessarily require a
change in the caller function, as the placement of the BUG_INSTR must have
changed.

Take advantage of this relocation, and unconditionally mark the bug_frame
sections themselves as not having changed, the logic in
kpatch_regenerate_special_section() will already take care of including any
bug_frame element group that references a function that has changed.

This should be a non functional change in the payload generated by
create-diff-object, but needs doing so that we can take into account changes to
.altinstructions and .ex_table sections themselves without unnecessarily also
pulling .bug_frame sections.

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

diff --git a/create-diff-object.c b/create-diff-object.c
index f4e4da063d0a..d1b1477be1cd 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1284,6 +1284,17 @@ static void kpatch_regenerate_special_section(struct kpatch_elf *kelf,
 		}
 	}
 
+	/*
+	 * For bug_frame sections don't care if the section itself or the
+	 * relocations have changed, as any change in the bug_frames will be
+	 * accompanied by a change in the caller function text, as the
+	 * BUG_INSTR will have a different placement in the caller.
+	 */
+	if (!strncmp(special->name, ".bug_frames.", strlen(".bug_frames."))) {
+		sec->status = SAME;
+		sec->base->status = SAME;
+	}
+
 	for ( ; src_offset < sec->base->sh.sh_size; src_offset += group_size) {
 
 		group_size = special->group_size(kelf, src_offset);
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 8/9] create-diff-object: account for changes in the special section itself
  2024-04-29 14:56 [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements Roger Pau Monne
                   ` (6 preceding siblings ...)
  2024-04-29 14:56 ` [PATCH 7/9] create-diff-object: don't account for changes to .bug_frame.? sections Roger Pau Monne
@ 2024-04-29 14:56 ` Roger Pau Monne
  2024-04-29 14:56 ` [PATCH 9/9] create-diff-object: account for special section changes Roger Pau Monne
  8 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monne @ 2024-04-29 14:56 UTC (permalink / raw
  To: xen-devel; +Cc: konrad.wilk, ross.lagerwall, Roger Pau Monne

Changes to special sections are not accounted for right now.  For example a
change that only affects .altinstructions or .ex_table won't be included in the
livepatch payload, if none of the symbols referenced by those sections have
also changed.

Since it's not possible to know which elements in the special group have
changed if we detect a change in the special section the whole group (minus
elements that have references to init section symbols) must be included.

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

diff --git a/create-diff-object.c b/create-diff-object.c
index d1b1477be1cd..8bfb6bf92a1b 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1170,13 +1170,32 @@ static int should_keep_rela_group(struct section *sec, int start, int size)
 	 */
 	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_SECTION) &&
-		    rela->sym->sec->include) {
-			found = 1;
-			log_debug("new/changed symbol %s found in special section %s\n",
-				  rela->sym->name, sec->name);
+		    rela->offset < start + size)
+		{
+			/*
+			 * Avoid including groups with relocations against
+			 * symbols living in init sections, those are never
+			 * included in livepatches.
+			 */
+			if (is_init_section(rela->sym->sec))
+				return 0;
+
+			/*
+			 * If the base section has changed, always include all
+			 * groups (except those pointing to .init section
+			 * symbols), as we have no way to differentiate which
+			 * groups have changed.
+			 */
+			if (sec->status != SAME || sec->base->status != SAME)
+				found = 1;
+			else if ((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",
+				          rela->sym->name, sec->name);
+			}
 		}
 	}
 
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 9/9] create-diff-object: account for special section changes
  2024-04-29 14:56 [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements Roger Pau Monne
                   ` (7 preceding siblings ...)
  2024-04-29 14:56 ` [PATCH 8/9] create-diff-object: account for changes in the special section itself Roger Pau Monne
@ 2024-04-29 14:56 ` Roger Pau Monne
  8 siblings, 0 replies; 10+ messages in thread
From: Roger Pau Monne @ 2024-04-29 14:56 UTC (permalink / raw
  To: xen-devel; +Cc: konrad.wilk, ross.lagerwall, Roger Pau Monne

When deciding whether there's content to generate a payload also take into
account whether special sections have changed.  Initially account for changes
to alternative related section to cause the generation of a livepatch.

Note that accounting for hook sections is already done by
kpatch_include_hook_elements() when deciding whether there's changed content in
the object file.  .bugframe sections are also not accounted for since any
section in a bugframe section will be accompanied by a change in the function
the bugframe references (the placement of the BUG_INSTR will change in the
caller function).

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

diff --git a/create-diff-object.c b/create-diff-object.c
index 8bfb6bf92a1b..957631097b8a 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1663,6 +1663,28 @@ static int kpatch_include_hook_elements(struct kpatch_elf *kelf)
 	return num_new_functions;
 }
 
+static unsigned int include_special_sections(const struct kpatch_elf *kelf)
+{
+	const struct section *sec;
+	unsigned int nr = 0;
+
+	/*
+	 * Only account for changes in alternatives and exception table related
+	 * sections.  Hooks are already taken care of separately, and changes
+	 * in bug_frame sections always go along with changes in the caller
+	 * functions.
+	 */
+	list_for_each_entry(sec, &kelf->sections, list)
+		if (sec->status != SAME &&
+		    (!strcmp(sec->name, ".altinstructions") ||
+		     !strcmp(sec->name, ".altinstr_replacement") ||
+		     !strcmp(sec->name, ".ex_table") ||
+		     !strcmp(sec->name, ".fixup")))
+			nr++;
+
+	return nr;
+}
+
 static int kpatch_include_new_globals(struct kpatch_elf *kelf)
 {
 	struct symbol *sym;
@@ -2469,6 +2491,8 @@ int main(int argc, char *argv[])
 	kpatch_include_debug_sections(kelf_patched);
 	log_debug("Include hook elements\n");
 	num_changed += kpatch_include_hook_elements(kelf_patched);
+	log_debug("Include special sections\n");
+	num_changed += include_special_sections(kelf_patched);
 	log_debug("num_changed = %d\n", num_changed);
 	log_debug("Include new globals\n");
 	new_globals_exist = kpatch_include_new_globals(kelf_patched);
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-04-29 15:01 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-29 14:56 [PATCH 0/9] livepatch-build-tools: some bug fixes and improvements Roger Pau Monne
2024-04-29 14:56 ` [PATCH 1/9] livepatch-build-tools: allow patch file name sizes up to 127 characters Roger Pau Monne
2024-04-29 14:56 ` [PATCH 2/9] create-diff-object: update default alt_instr size Roger Pau Monne
2024-04-29 14:56 ` [PATCH 3/9] livepatch-build: fix detection of structure sizes Roger Pau Monne
2024-04-29 14:56 ` [PATCH 4/9] create-diff-object: document kpatch_regenerate_special_section() behavior Roger Pau Monne
2024-04-29 14:56 ` [PATCH 5/9] create-diff-object: move kpatch_include_symbol() Roger Pau Monne
2024-04-29 14:56 ` [PATCH 6/9] create-diff-object: detect changes in .altinstr_replacement and .fixup sections Roger Pau Monne
2024-04-29 14:56 ` [PATCH 7/9] create-diff-object: don't account for changes to .bug_frame.? sections Roger Pau Monne
2024-04-29 14:56 ` [PATCH 8/9] create-diff-object: account for changes in the special section itself Roger Pau Monne
2024-04-29 14:56 ` [PATCH 9/9] create-diff-object: account for special section changes Roger Pau Monne

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.