[livepatch-build-tools part2 1/6] common: Add is_standard_section() helper function

[livepatch-build-tools part2 1/6] common: Add is_standard_section() helper function

[Pawel Wieczorkiewicz]

Detect standard (always to be included) sections via their section
header type. The standard sections: ".shstrtab", ".symtab", ".strtab"
are either of type SHT_SYMTAB or SHT_STRTAB.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
---
 common.c             | 12 ++++++++++++
 common.h             |  1 +
 create-diff-object.c |  5 +----
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/common.c b/common.c
index bc63955..1fb07cb 100644
--- a/common.c
+++ b/common.c
@@ -5,6 +5,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <stdbool.h>
 #include <gelf.h>
 
 #include "list.h"
@@ -258,6 +259,17 @@ int is_debug_section(struct section *sec)
 	return !strncmp(name, ".debug_", 7);
 }
 
+int is_standard_section(struct section *sec)
+{
+	switch (sec->sh.sh_type) {
+	case SHT_STRTAB:
+	case SHT_SYMTAB:
+		return true;
+	default:
+		return false;
+	}
+}
+
 /* returns the offset of the string in the string table */
 int offset_of_string(struct list_head *list, char *name)
 {
diff --git a/common.h b/common.h
index 7599fe7..cda690d 100644
--- a/common.h
+++ b/common.h
@@ -150,6 +150,7 @@ struct symbol *find_symbol_by_name(struct list_head *list, const char *name);
 int is_text_section(struct section *sec);
 int is_debug_section(struct section *sec);
 int is_rela_section(struct section *sec);
+int is_standard_section(struct section *sec);
 int is_local_sym(struct symbol *sym);
 
 void rela_insn(struct section *sec, struct rela *rela, struct insn *insn);
diff --git a/create-diff-object.c b/create-diff-object.c
index 82f777e..1e6e617 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1278,10 +1278,7 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
 
 	list_for_each_entry(sec, &kelf->sections, list) {
 		/* include these sections even if they haven't changed */
-		if (!strcmp(sec->name, ".shstrtab") ||
-		    !strcmp(sec->name, ".strtab") ||
-		    !strcmp(sec->name, ".symtab") ||
-		    should_include_str_section(sec->name)) {
+		if (is_standard_section(sec) || should_include_str_section(sec->name)) {
 			sec->include = 1;
 			if (sec->secsym)
 				sec->secsym->include = 1;
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[livepatch-build-tools part2 2/6] common: Add is_referenced_section() helper function

[Pawel Wieczorkiewicz]

This function checks if given section has an included corresponding
RELA section and/or any of the symbols table symbols references the
section. Section associated symbols are ignored here as there is
always such a symbol for every section.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
---
 common.c | 22 +++++++++++++++++++++-
 common.h |  1 +
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/common.c b/common.c
index 1fb07cb..c968299 100644
--- a/common.c
+++ b/common.c
@@ -15,7 +15,7 @@
 
 int is_rela_section(struct section *sec)
 {
-	return (sec->sh.sh_type == SHT_RELA);
+	return sec && (sec->sh.sh_type == SHT_RELA);
 }
 
 int is_local_sym(struct symbol *sym)
@@ -270,6 +270,26 @@ int is_standard_section(struct section *sec)
 	}
 }
 
+int is_referenced_section(const struct section *sec, const struct kpatch_elf *kelf)
+{
+	struct symbol *sym;
+
+	if (is_rela_section(sec->rela) && sec->rela->include)
+		return true;
+
+	list_for_each_entry(sym, &kelf->symbols, list) {
+		if (!sym->include || !sym->sec)
+			continue;
+		/* Ignore section associated sections */
+		if (sym->type == STT_SECTION)
+			continue;
+		if (sym->sec->index == sec->index)
+			return true;
+	}
+
+	return false;
+}
+
 /* returns the offset of the string in the string table */
 int offset_of_string(struct list_head *list, char *name)
 {
diff --git a/common.h b/common.h
index cda690d..affe16b 100644
--- a/common.h
+++ b/common.h
@@ -151,6 +151,7 @@ int is_text_section(struct section *sec);
 int is_debug_section(struct section *sec);
 int is_rela_section(struct section *sec);
 int is_standard_section(struct section *sec);
+int is_referenced_section(const struct section *sec, const struct kpatch_elf *kelf);
 int is_local_sym(struct symbol *sym);
 
 void rela_insn(struct section *sec, struct rela *rela, struct insn *insn);
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[livepatch-build-tools part2 3/6] create-diff-object: Add is_special_section() helper function

[Pawel Wieczorkiewicz]

This function determines, based on the given section name, if the
sections belongs to the special sections category.
It treats a name from the special_sections array as a prefix. Any
sections whose name starts with special section prefix is considered
a special section.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
---
 common.c | 23 +++++++++++++++++++++++
 common.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/common.c b/common.c
index c968299..a2c860b 100644
--- a/common.c
+++ b/common.c
@@ -290,6 +290,29 @@ int is_referenced_section(const struct section *sec, const struct kpatch_elf *ke
 	return false;
 }
 
+int is_special_section(struct section *sec)
+{
+	static struct special_section_names {
+		const char *name;
+	} names[] = {
+		{ .name		= ".bug_frames"                 },
+		{ .name		= ".fixup"                      },
+		{ .name		= ".ex_table"                   },
+		{ .name		= ".altinstructions"            },
+		{ .name		= ".altinstr_replacement"       },
+		{ .name		= ".livepatch.hooks"            },
+		{ .name         = NULL                          },
+	};
+	struct special_section_names *special;
+
+	for (special = names; special->name; special++) {
+		if (!strncmp(sec->name, special->name, strlen(special->name)))
+			return true;
+	}
+
+	return false;
+}
+
 /* returns the offset of the string in the string table */
 int offset_of_string(struct list_head *list, char *name)
 {
diff --git a/common.h b/common.h
index affe16b..6d38e88 100644
--- a/common.h
+++ b/common.h
@@ -152,6 +152,7 @@ int is_debug_section(struct section *sec);
 int is_rela_section(struct section *sec);
 int is_standard_section(struct section *sec);
 int is_referenced_section(const struct section *sec, const struct kpatch_elf *kelf);
+int is_special_section(struct section *sec);
 int is_local_sym(struct symbol *sym);
 
 void rela_insn(struct section *sec, struct rela *rela, struct insn *insn);
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[livepatch-build-tools part2 4/6] livepatch-build: detect special section group sizes

[Pawel Wieczorkiewicz]

Hard-coding the special section group sizes is unreliable. Instead,
determine them dynamically by finding the related struct definitions
in the DWARF metadata.

This is a livepatch backport of kpatch upstream commit [1]:
kpatch-build: detect special section group sizes 170449847136a48b19fc

Xen only deals with alt_instr, bug_frame and exception_table_entry
structures, so sizes of these structers are obtained from xen-syms.

This change is needed since with recent Xen the alt_instr structure
has changed size from 12 to 14 bytes.

[1] https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Martin Mazein <amazein@amazon.de>
---
 create-diff-object.c | 60 ++++++++++++++++++++++++++++++++++++++++++++--------
 livepatch-build      | 23 ++++++++++++++++++++
 2 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/create-diff-object.c b/create-diff-object.c
index 1e6e617..b0b4dcb 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf)
 	}
 }
 
-static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
-static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
-static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
-static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { return 16; }
-static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
-static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) { return 12; }
+static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
+{
+    static int size = 0;
+    char *str;
+    if (!size) {
+        str = getenv("BUG_STRUCT_SIZE");
+        size = str ? atoi(str) : 8;
+    }
+
+    return size;
+}
+
+static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
+{
+    static int size = 0;
+    char *str;
+    if (!size) {
+        str = getenv("BUG_STRUCT_SIZE");
+        size = str ? atoi(str) : 16;
+    }
+
+    return size;
+}
+
+static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
+{
+    static int size = 0;
+    char *str;
+    if (!size) {
+        str = getenv("EX_STRUCT_SIZE");
+        size = str ? atoi(str) : 8;
+    }
+
+    return size;
+}
+
+static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
+{
+    static int size = 0;
+    char *str;
+    if (!size) {
+        str = getenv("ALT_STRUCT_SIZE");
+        size = str ? atoi(str) : 12;
+    }
+
+    printf("altinstr_size=%d\n", size);
+    return size;
+}
 
 /*
  * The rela groups in the .fixup section vary in size.  The beginning of each
@@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf *kelf, int offset)
 static struct special_section special_sections[] = {
 	{
 		.name		= ".bug_frames.0",
-		.group_size	= bug_frames_0_group_size,
+		.group_size	= bug_frames_group_size,
 	},
 	{
 		.name		= ".bug_frames.1",
-		.group_size	= bug_frames_1_group_size,
+		.group_size	= bug_frames_group_size,
 	},
 	{
 		.name		= ".bug_frames.2",
-		.group_size	= bug_frames_2_group_size,
+		.group_size	= bug_frames_group_size,
 	},
 	{
 		.name		= ".bug_frames.3",
diff --git a/livepatch-build b/livepatch-build
index c057fa1..a6cae12 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}"
 echo "================================================"
 echo
 
+if [ -f "$XENSYMS" ]; then
+    echo "Reading special section data"
+    SPECIAL_VARS=$(readelf -wi "$XENSYMS" |
+               gawk --non-decimal-data '
+               BEGIN { a = b = e = 0 }
+               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}
+               a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2}
+               b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2}
+               e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2}
+               a == 2 && b == 2 && e == 2 {exit}')
+    [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS"
+    if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ -z $EX_STRUCT_SIZE ]]; then
+        die "can't find special struct size"
+    fi
+    for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do
+        if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then
+            die "invalid special struct size $i"
+        fi
+    done
+fi
+
 if [ "${SKIP}" != "build" ]; then
     [ -e "${OUTPUT}" ] && die "Output directory exists"
     grep -q 'CONFIG_LIVEPATCH=y' "${CONFIGFILE}" || die "CONFIG_LIVEPATCH must be enabled"
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[livepatch-build-tools part2 5/6] create-diff-object: Add new entries to special sections array

[Pawel Wieczorkiewicz]

Handle .livepatch.hooks* and .altinstr_replacement sections as the
special sections with assigned group_size resolution function.
By default each .livepatch.hooks* sections' entry is 8 bytes long (a
pointer). The .altinstr_replacement section follows the .altinstructions
section settings.

Allow to specify different .livepatch.hooks* section entry size using
shell environment variable HOOK_STRUCT_SIZE.

Cleanup an incorrect indentation around group_size resulution functions.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
---
 create-diff-object.c | 87 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 33 deletions(-)

diff --git a/create-diff-object.c b/create-diff-object.c
index b0b4dcb..f6060cd 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -960,51 +960,64 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf)
 
 static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
 {
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("BUG_STRUCT_SIZE");
-        size = str ? atoi(str) : 8;
-    }
-
-    return size;
+	static int size = 0;
+	char *str;
+	if (!size) {
+		str = getenv("BUG_STRUCT_SIZE");
+		size = str ? atoi(str) : 8;
+	}
+
+	return size;
 }
 
 static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
 {
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("BUG_STRUCT_SIZE");
-        size = str ? atoi(str) : 16;
-    }
-
-    return size;
+	static int size = 0;
+	char *str;
+	if (!size) {
+		str = getenv("BUG_STRUCT_SIZE");
+		size = str ? atoi(str) : 16;
+	}
+
+	return size;
 }
 
 static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
 {
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("EX_STRUCT_SIZE");
-        size = str ? atoi(str) : 8;
-    }
-
-    return size;
+	static int size = 0;
+	char *str;
+	if (!size) {
+		str = getenv("EX_STRUCT_SIZE");
+		size = str ? atoi(str) : 8;
+	}
+
+	return size;
 }
 
 static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
 {
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("ALT_STRUCT_SIZE");
-        size = str ? atoi(str) : 12;
-    }
-
-    printf("altinstr_size=%d\n", size);
-    return size;
+	static int size = 0;
+	char *str;
+	if (!size) {
+		str = getenv("ALT_STRUCT_SIZE");
+		size = str ? atoi(str) : 12;
+	}
+
+	printf("altinstr_size=%d\n", size);
+	return size;
+}
+
+static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset)
+{
+	static int size = 0;
+	char *str;
+	if (!size) {
+		str = getenv("HOOK_STRUCT_SIZE");
+		size = str ? atoi(str) : 8;
+	}
+
+	printf("livepatch_hooks_size=%d\n", size);
+	return size;
 }
 
 /*
@@ -1084,6 +1097,14 @@ static struct special_section special_sections[] = {
 		.name		= ".altinstructions",
 		.group_size	= altinstructions_group_size,
 	},
+	{
+		.name		= ".altinstr_replacement",
+		.group_size	= altinstructions_group_size,
+	},
+	{
+		.name		= ".livepatch.hooks",
+		.group_size	= livepatch_hooks_group_size,
+	},
 	{},
 };
 
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[livepatch-build-tools part2 6/6] create-diff-object: Do not include all .rodata sections

[Pawel Wieczorkiewicz]

Starting with the "2af6f1aa6233 Fix patch creation with GCC 6.1+" commit
all .rodata sections are included by default (regardless of whether they
are needed or not).
During stacked hotpatch builds it leads to unnecessary duplication of
the .rodata sections as each and every consecutive hotpatch contains all
the .rodata section of previous hotpatches.

To prevent this situation, mark the .rodata section for inclusion only
if they are referenced by any of the current hotpatch symbols (or a
corresponding RELA section).

Extend patchability verification to detect all non-standard, non-rela,
non-debug and non-special sections that are not referenced by any of
the symbols or RELA sections.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
---
 create-diff-object.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/create-diff-object.c b/create-diff-object.c
index f6060cd..f7eb421 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1341,7 +1341,7 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
 
 	list_for_each_entry(sec, &kelf->sections, list) {
 		/* include these sections even if they haven't changed */
-		if (is_standard_section(sec) || should_include_str_section(sec->name)) {
+		if (is_standard_section(sec)) {
 			sec->include = 1;
 			if (sec->secsym)
 				sec->secsym->include = 1;
@@ -1352,6 +1352,19 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
 	list_entry(kelf->symbols.next, struct symbol, list)->include = 1;
 }
 
+static void kpatch_include_standard_string_elements(struct kpatch_elf *kelf)
+{
+	struct section *sec;
+
+	list_for_each_entry(sec, &kelf->sections, list) {
+		if (should_include_str_section(sec->name) && is_referenced_section(sec, kelf)) {
+			sec->include = 1;
+			if (sec->secsym)
+				sec->secsym->include = 1;
+		}
+	}
+}
+
 #define inc_printf(fmt, ...) \
 	log_debug("%*s" fmt, recurselevel, "", ##__VA_ARGS__);
 
@@ -1531,6 +1544,16 @@ static void kpatch_verify_patchability(struct kpatch_elf *kelf)
 			errs++;
 		}
 
+		if (sec->include) {
+			if (!is_standard_section(sec) && !is_rela_section(sec) &&
+			    !is_debug_section(sec) && !is_special_section(sec)) {
+				if (!is_referenced_section(sec, kelf)) {
+					log_normal("section %s included, but not referenced\n", sec->name);
+					errs++;
+				}
+			}
+		}
+
 		/*
 		 * ensure we aren't including .data.* or .bss.*
 		 * (.data.unlikely is ok b/c it only has __warned vars)
@@ -2062,6 +2085,8 @@ int main(int argc, char *argv[])
 	kpatch_include_debug_sections(kelf_patched);
 	log_debug("Include hook elements\n");
 	kpatch_include_hook_elements(kelf_patched);
+	log_debug("Include standard string elements\n");
+	kpatch_include_standard_string_elements(kelf_patched);
 	log_debug("Include new globals\n");
 	new_globals_exist = kpatch_include_new_globals(kelf_patched);
 	log_debug("new_globals_exist = %d\n", new_globals_exist);
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 4/6] livepatch-build: detect special section group sizes

[Konrad Rzeszutek Wilk]

On Tue, Apr 16, 2019 at 12:07:14PM +0000, Pawel Wieczorkiewicz wrote:
> Hard-coding the special section group sizes is unreliable. Instead,
> determine them dynamically by finding the related struct definitions
> in the DWARF metadata.
> 
> This is a livepatch backport of kpatch upstream commit [1]:
> kpatch-build: detect special section group sizes 170449847136a48b19fc
> 
> Xen only deals with alt_instr, bug_frame and exception_table_entry
> structures, so sizes of these structers are obtained from xen-syms.
> 
> This change is needed since with recent Xen the alt_instr structure
> has changed size from 12 to 14 bytes.

Oh this is so much better than the "solution" we coded.

Thank you!

Ross, will commit to repo unless you have concerns..
> 
> [1] https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Martin Mazein <amazein@amazon.de>
> ---
>  create-diff-object.c | 60 ++++++++++++++++++++++++++++++++++++++++++++--------
>  livepatch-build      | 23 ++++++++++++++++++++
>  2 files changed, 74 insertions(+), 9 deletions(-)
> 
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 1e6e617..b0b4dcb 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf)
>  	}
>  }
>  
> -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
> -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
> -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
> -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { return 16; }
> -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
> -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) { return 12; }
> +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +    static int size = 0;
> +    char *str;
> +    if (!size) {
> +        str = getenv("BUG_STRUCT_SIZE");
> +        size = str ? atoi(str) : 8;
> +    }
> +
> +    return size;
> +}
> +
> +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +    static int size = 0;
> +    char *str;
> +    if (!size) {
> +        str = getenv("BUG_STRUCT_SIZE");
> +        size = str ? atoi(str) : 16;
> +    }
> +
> +    return size;
> +}
> +
> +static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +    static int size = 0;
> +    char *str;
> +    if (!size) {
> +        str = getenv("EX_STRUCT_SIZE");
> +        size = str ? atoi(str) : 8;
> +    }
> +
> +    return size;
> +}
> +
> +static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +    static int size = 0;
> +    char *str;
> +    if (!size) {
> +        str = getenv("ALT_STRUCT_SIZE");
> +        size = str ? atoi(str) : 12;
> +    }
> +
> +    printf("altinstr_size=%d\n", size);
> +    return size;
> +}
>  
>  /*
>   * The rela groups in the .fixup section vary in size.  The beginning of each
> @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf *kelf, int offset)
>  static struct special_section special_sections[] = {
>  	{
>  		.name		= ".bug_frames.0",
> -		.group_size	= bug_frames_0_group_size,
> +		.group_size	= bug_frames_group_size,
>  	},
>  	{
>  		.name		= ".bug_frames.1",
> -		.group_size	= bug_frames_1_group_size,
> +		.group_size	= bug_frames_group_size,
>  	},
>  	{
>  		.name		= ".bug_frames.2",
> -		.group_size	= bug_frames_2_group_size,
> +		.group_size	= bug_frames_group_size,
>  	},
>  	{
>  		.name		= ".bug_frames.3",
> diff --git a/livepatch-build b/livepatch-build
> index c057fa1..a6cae12 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}"
>  echo "================================================"
>  echo
>  
> +if [ -f "$XENSYMS" ]; then
> +    echo "Reading special section data"
> +    SPECIAL_VARS=$(readelf -wi "$XENSYMS" |
> +               gawk --non-decimal-data '
> +               BEGIN { a = b = e = 0 }
> +               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}
> +               a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2}
> +               b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2}
> +               e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2}
> +               a == 2 && b == 2 && e == 2 {exit}')
> +    [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS"
> +    if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ -z $EX_STRUCT_SIZE ]]; then
> +        die "can't find special struct size"
> +    fi
> +    for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do
> +        if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then
> +            die "invalid special struct size $i"
> +        fi
> +    done
> +fi
> +
>  if [ "${SKIP}" != "build" ]; then
>      [ -e "${OUTPUT}" ] && die "Output directory exists"
>      grep -q 'CONFIG_LIVEPATCH=y' "${CONFIGFILE}" || die "CONFIG_LIVEPATCH must be enabled"
> -- 
> 2.16.5
> 
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
> Ust-ID: DE 289 237 879
> Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 4/6] livepatch-build: detect special section group sizes

[Ross Lagerwall]

On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
> Hard-coding the special section group sizes is unreliable. Instead,
> determine them dynamically by finding the related struct definitions
> in the DWARF metadata.
> 
> This is a livepatch backport of kpatch upstream commit [1]:
> kpatch-build: detect special section group sizes 170449847136a48b19fc
> 
> Xen only deals with alt_instr, bug_frame and exception_table_entry
> structures, so sizes of these structers are obtained from xen-syms.

structers -> structures

> 
> This change is needed since with recent Xen the alt_instr structure
> has changed size from 12 to 14 bytes.
> 
> [1] https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Martin Mazein <amazein@amazon.de>
> ---
>   create-diff-object.c | 60 ++++++++++++++++++++++++++++++++++++++++++++--------
>   livepatch-build      | 23 ++++++++++++++++++++
>   2 files changed, 74 insertions(+), 9 deletions(-)
> 
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 1e6e617..b0b4dcb 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf)
>   	}
>   }
>   
> -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
> -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
> -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
> -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { return 16; }
> -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
> -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) { return 12; }
> +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +    static int size = 0;
> +    char *str;
> +    if (!size) {
> +        str = getenv("BUG_STRUCT_SIZE");
> +        size = str ? atoi(str) : 8;

Why did you remove the hard error here from the original kpatch commit? 
I think a hard error is preferable to guessing.

> +    }
> +
> +    return size;
> +}
> +
> +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +    static int size = 0;
> +    char *str;
> +    if (!size) {
> +        str = getenv("BUG_STRUCT_SIZE");
> +        size = str ? atoi(str) : 16;
> +    }
> +
> +    return size;
> +}
> +
> +static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +    static int size = 0;
> +    char *str;
> +    if (!size) {
> +        str = getenv("EX_STRUCT_SIZE");
> +        size = str ? atoi(str) : 8;
> +    }
> +
> +    return size;
> +}
> +
> +static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +    static int size = 0;
> +    char *str;
> +    if (!size) {
> +        str = getenv("ALT_STRUCT_SIZE");
> +        size = str ? atoi(str) : 12;
> +    }
> +
> +    printf("altinstr_size=%d\n", size);
> +    return size;
> +}
>   
>   /*
>    * The rela groups in the .fixup section vary in size.  The beginning of each
> @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf *kelf, int offset)
>   static struct special_section special_sections[] = {
>   	{
>   		.name		= ".bug_frames.0",
> -		.group_size	= bug_frames_0_group_size,
> +		.group_size	= bug_frames_group_size,
>   	},
>   	{
>   		.name		= ".bug_frames.1",
> -		.group_size	= bug_frames_1_group_size,
> +		.group_size	= bug_frames_group_size,
>   	},
>   	{
>   		.name		= ".bug_frames.2",
> -		.group_size	= bug_frames_2_group_size,
> +		.group_size	= bug_frames_group_size,
>   	},
>   	{
>   		.name		= ".bug_frames.3",
> diff --git a/livepatch-build b/livepatch-build
> index c057fa1..a6cae12 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}"
>   echo "================================================"
>   echo
>   
> +if [ -f "$XENSYMS" ]; then
> +    echo "Reading special section data"
> +    SPECIAL_VARS=$(readelf -wi "$XENSYMS" |
> +               gawk --non-decimal-data '
> +               BEGIN { a = b = e = 0 }
> +               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}
> +               a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2}
> +               b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2}
> +               e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2}
> +               a == 2 && b == 2 && e == 2 {exit}')
> +    [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS"
> +    if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ -z $EX_STRUCT_SIZE ]]; then
> +        die "can't find special struct size"
> +    fi
> +    for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do
> +        if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then
> +            die "invalid special struct size $i"
> +        fi
> +    done
> +fi
> +

If this hunk is moved after the call to build_full(), then it can be run 
directly on the xen-syms that has just been built which would allow 
dropping `if [ -f "$XENSYMS" ]...` and guaranteeing that *_STRUCT_SIZE 
are always set.

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 4/6] livepatch-build: detect special section group sizes

[Ross Lagerwall]

On 4/25/19 5:53 AM, Konrad Rzeszutek Wilk wrote:
> On Tue, Apr 16, 2019 at 12:07:14PM +0000, Pawel Wieczorkiewicz wrote:
>> Hard-coding the special section group sizes is unreliable. Instead,
>> determine them dynamically by finding the related struct definitions
>> in the DWARF metadata.
>>
>> This is a livepatch backport of kpatch upstream commit [1]:
>> kpatch-build: detect special section group sizes 170449847136a48b19fc
>>
>> Xen only deals with alt_instr, bug_frame and exception_table_entry
>> structures, so sizes of these structers are obtained from xen-syms.
>>
>> This change is needed since with recent Xen the alt_instr structure
>> has changed size from 12 to 14 bytes.
> 
> Oh this is so much better than the "solution" we coded.
> 
> Thank you!
> 
> Ross, will commit to repo unless you have concerns..
I have reviewed it with a few comments. Note that this is basically the 
same as Glenn Enright's recent patch ("livepatch-build-tools: Detect 
special section group sizes") but slightly more complete as it detects 
sizes for the bug frames too so it should probably be used instead.

Thanks,
-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 4/6] livepatch-build: detect special section group sizes

[Ross Lagerwall]

On 4/29/19 3:10 PM, Ross Lagerwall wrote:
> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>> Hard-coding the special section group sizes is unreliable. Instead,
>> determine them dynamically by finding the related struct definitions
>> in the DWARF metadata.
>>
>> This is a livepatch backport of kpatch upstream commit [1]:
>> kpatch-build: detect special section group sizes 170449847136a48b19fc
>>
>> Xen only deals with alt_instr, bug_frame and exception_table_entry
>> structures, so sizes of these structers are obtained from xen-syms.
> 
> structers -> structures
> 
>>
>> This change is needed since with recent Xen the alt_instr structure
>> has changed size from 12 to 14 bytes.
>>
>> [1] 
>> https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56 
>>
>>
>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Martin Mazein <amazein@amazon.de>
>> ---
>>   create-diff-object.c | 60 
>> ++++++++++++++++++++++++++++++++++++++++++++--------
>>   livepatch-build      | 23 ++++++++++++++++++++
>>   2 files changed, 74 insertions(+), 9 deletions(-)
>>
>> diff --git a/create-diff-object.c b/create-diff-object.c
>> index 1e6e617..b0b4dcb 100644
>> --- a/create-diff-object.c
>> +++ b/create-diff-object.c
>> @@ -958,12 +958,54 @@ static void 
>> kpatch_mark_constant_labels_same(struct kpatch_elf *kelf)
>>       }
>>   }
>> -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int 
>> offset) { return 8; }
>> -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int 
>> offset) { return 8; }
>> -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int 
>> offset) { return 8; }
>> -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int 
>> offset) { return 16; }
>> -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { 
>> return 8; }
>> -static int altinstructions_group_size(struct kpatch_elf *kelf, int 
>> offset) { return 12; }
>> +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
>> +{
>> +    static int size = 0;
>> +    char *str;
>> +    if (!size) {
>> +        str = getenv("BUG_STRUCT_SIZE");
>> +        size = str ? atoi(str) : 8;
> 
> Why did you remove the hard error here from the original kpatch commit? 
> I think a hard error is preferable to guessing.
> 
>> +    }
>> +
>> +    return size;
>> +}
>> +
>> +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
>> +{
>> +    static int size = 0;
>> +    char *str;
>> +    if (!size) {
>> +        str = getenv("BUG_STRUCT_SIZE");
>> +        size = str ? atoi(str) : 16;
>> +    }
>> +
>> +    return size;
>> +}
>> +
>> +static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
>> +{
>> +    static int size = 0;
>> +    char *str;
>> +    if (!size) {
>> +        str = getenv("EX_STRUCT_SIZE");
>> +        size = str ? atoi(str) : 8;
>> +    }
>> +
>> +    return size;
>> +}
>> +
>> +static int altinstructions_group_size(struct kpatch_elf *kelf, int 
>> offset)
>> +{
>> +    static int size = 0;
>> +    char *str;
>> +    if (!size) {
>> +        str = getenv("ALT_STRUCT_SIZE");
>> +        size = str ? atoi(str) : 12;
>> +    }
>> +
>> +    printf("altinstr_size=%d\n", size);
>> +    return size;
>> +}
>>   /*
>>    * The rela groups in the .fixup section vary in size.  The 
>> beginning of each
>> @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf 
>> *kelf, int offset)
>>   static struct special_section special_sections[] = {
>>       {
>>           .name        = ".bug_frames.0",
>> -        .group_size    = bug_frames_0_group_size,
>> +        .group_size    = bug_frames_group_size,
>>       },
>>       {
>>           .name        = ".bug_frames.1",
>> -        .group_size    = bug_frames_1_group_size,
>> +        .group_size    = bug_frames_group_size,
>>       },
>>       {
>>           .name        = ".bug_frames.2",
>> -        .group_size    = bug_frames_2_group_size,
>> +        .group_size    = bug_frames_group_size,
>>       },
>>       {
>>           .name        = ".bug_frames.3",
>> diff --git a/livepatch-build b/livepatch-build
>> index c057fa1..a6cae12 100755
>> --- a/livepatch-build
>> +++ b/livepatch-build
>> @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}"
>>   echo "================================================"
>>   echo
>> +if [ -f "$XENSYMS" ]; then
>> +    echo "Reading special section data"
>> +    SPECIAL_VARS=$(readelf -wi "$XENSYMS" |
>> +               gawk --non-decimal-data '
>> +               BEGIN { a = b = e = 0 }
>> +               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}
>> +               a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2}
>> +               b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2}
>> +               e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2}
>> +               a == 2 && b == 2 && e == 2 {exit}')
>> +    [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS"
>> +    if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ 
>> -z $EX_STRUCT_SIZE ]]; then
>> +        die "can't find special struct size"
>> +    fi
>> +    for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do
>> +        if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then
>> +            die "invalid special struct size $i"
>> +        fi
>> +    done
>> +fi
>> +
> 
> If this hunk is moved after the call to build_full(), then it can be run 
> directly on the xen-syms that has just been built which would allow 
> dropping `if [ -f "$XENSYMS" ]...` and guaranteeing that *_STRUCT_SIZE 
> are always set.
> 

One more thing, previously:

bug_frames_0_group_size == 8
bug_frames_1_group_size == 8
bug_frames_2_group_size == 8
bug_frames_3_group_size == 16

And now:

bug_frames_0_group_size == BUG_STRUCT_SIZE
bug_frames_1_group_size == BUG_STRUCT_SIZE
bug_frames_2_group_size == BUG_STRUCT_SIZE
bug_frames_3_group_size == BUG_STRUCT_SIZE

This seems wrong to me. When reading special section data, should you 
detect e.g. BUG0_STRUCT_SIZE, BUG1_STRUCT_SIZE, ... ?

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 1/6] common: Add is_standard_section() helper function

[Ross Lagerwall]

On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
> Detect standard (always to be included) sections via their section
> header type. The standard sections: ".shstrtab", ".symtab", ".strtab"
> are either of type SHT_SYMTAB or SHT_STRTAB.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> ---
>   common.c             | 12 ++++++++++++
>   common.h             |  1 +
>   create-diff-object.c |  5 +----
>   3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/common.c b/common.c
> index bc63955..1fb07cb 100644
> --- a/common.c
> +++ b/common.c
> @@ -5,6 +5,7 @@
>   #include <sys/stat.h>
>   #include <fcntl.h>
>   #include <unistd.h>
> +#include <stdbool.h>
>   #include <gelf.h>
>   
>   #include "list.h"
> @@ -258,6 +259,17 @@ int is_debug_section(struct section *sec)
>   	return !strncmp(name, ".debug_", 7);
>   }
>   
> +int is_standard_section(struct section *sec)
> +{
> +	switch (sec->sh.sh_type) {
> +	case SHT_STRTAB:
> +	case SHT_SYMTAB:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>   /* returns the offset of the string in the string table */
>   int offset_of_string(struct list_head *list, char *name)
>   {
> diff --git a/common.h b/common.h
> index 7599fe7..cda690d 100644
> --- a/common.h
> +++ b/common.h
> @@ -150,6 +150,7 @@ struct symbol *find_symbol_by_name(struct list_head *list, const char *name);
>   int is_text_section(struct section *sec);
>   int is_debug_section(struct section *sec);
>   int is_rela_section(struct section *sec);
> +int is_standard_section(struct section *sec);
>   int is_local_sym(struct symbol *sym);
>   
>   void rela_insn(struct section *sec, struct rela *rela, struct insn *insn);
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 82f777e..1e6e617 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -1278,10 +1278,7 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
>   
>   	list_for_each_entry(sec, &kelf->sections, list) {
>   		/* include these sections even if they haven't changed */
> -		if (!strcmp(sec->name, ".shstrtab") ||
> -		    !strcmp(sec->name, ".strtab") ||
> -		    !strcmp(sec->name, ".symtab") ||
> -		    should_include_str_section(sec->name)) {
> +		if (is_standard_section(sec) || should_include_str_section(sec->name)) {

Let's keep lines to 80 chars where feasible (1 tab == 8 spaces). 
Otherwise LGTM.

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 2/6] common: Add is_referenced_section() helper function

[Ross Lagerwall]

On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
> This function checks if given section has an included corresponding
> RELA section and/or any of the symbols table symbols references the
> section. Section associated symbols are ignored here as there is
> always such a symbol for every section.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> ---
>   common.c | 22 +++++++++++++++++++++-
>   common.h |  1 +
>   2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/common.c b/common.c
> index 1fb07cb..c968299 100644
> --- a/common.c
> +++ b/common.c
> @@ -15,7 +15,7 @@
>   
>   int is_rela_section(struct section *sec)
>   {
> -	return (sec->sh.sh_type == SHT_RELA);
> +	return sec && (sec->sh.sh_type == SHT_RELA);
>   }
>   
>   int is_local_sym(struct symbol *sym)
> @@ -270,6 +270,26 @@ int is_standard_section(struct section *sec)
>   	}
>   }
>   
> +int is_referenced_section(const struct section *sec, const struct kpatch_elf *kelf)

Let's keep to 80 chars where practical (and throughout the rest of the 
patches).

> +{
> +	struct symbol *sym;
> +
> +	if (is_rela_section(sec->rela) && sec->rela->include)
> +		return true;
> +
> +	list_for_each_entry(sym, &kelf->symbols, list) {
> +		if (!sym->include || !sym->sec)
> +			continue;
> +		/* Ignore section associated sections */
> +		if (sym->type == STT_SECTION)
> +			continue;
> +		if (sym->sec->index == sec->index)
> +			return true;

You can simplify this check to `sym->sec == sec` like the rest of the 
code does.

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 4/6] livepatch-build: detect special section group sizes

[Wieczorkiewicz, Pawel]

> On 29. Apr 2019, at 16:21, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
> 
> On 4/29/19 3:10 PM, Ross Lagerwall wrote:
>> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>>> Hard-coding the special section group sizes is unreliable. Instead,
>>> determine them dynamically by finding the related struct definitions
>>> in the DWARF metadata.
>>> 
>>> This is a livepatch backport of kpatch upstream commit [1]:
>>> kpatch-build: detect special section group sizes 170449847136a48b19fc
>>> 
>>> Xen only deals with alt_instr, bug_frame and exception_table_entry
>>> structures, so sizes of these structers are obtained from xen-syms.
>> structers -> structures

Thanks, will fix.

>>> 
>>> This change is needed since with recent Xen the alt_instr structure
>>> has changed size from 12 to 14 bytes.
>>> 
>>> [1] https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56 
>>> 
>>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
>>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>>> Reviewed-by: Martin Mazein <amazein@amazon.de>
>>> ---
>>>   create-diff-object.c | 60 ++++++++++++++++++++++++++++++++++++++++++++--------
>>>   livepatch-build      | 23 ++++++++++++++++++++
>>>   2 files changed, 74 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/create-diff-object.c b/create-diff-object.c
>>> index 1e6e617..b0b4dcb 100644
>>> --- a/create-diff-object.c
>>> +++ b/create-diff-object.c
>>> @@ -958,12 +958,54 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf)
>>>       }
>>>   }
>>> -static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
>>> -static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
>>> -static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
>>> -static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { return 16; }
>>> -static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { return 8; }
>>> -static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) { return 12; }
>>> +static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +    static int size = 0;
>>> +    char *str;
>>> +    if (!size) {
>>> +        str = getenv("BUG_STRUCT_SIZE");
>>> +        size = str ? atoi(str) : 8;
>> Why did you remove the hard error here from the original kpatch commit? I think a hard error is preferable to guessing.

Previously the sizes were hard-coded. I prefer to use them directly over failing hard here.
If we could not come up with a sane defaults, than failing hard would be the only option.
Anyway, I am cool to go either way upon your good judgment.

>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +
>>> +static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +    static int size = 0;
>>> +    char *str;
>>> +    if (!size) {
>>> +        str = getenv("BUG_STRUCT_SIZE");
>>> +        size = str ? atoi(str) : 16;
>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +
>>> +static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +    static int size = 0;
>>> +    char *str;
>>> +    if (!size) {
>>> +        str = getenv("EX_STRUCT_SIZE");
>>> +        size = str ? atoi(str) : 8;
>>> +    }
>>> +
>>> +    return size;
>>> +}
>>> +
>>> +static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
>>> +{
>>> +    static int size = 0;
>>> +    char *str;
>>> +    if (!size) {
>>> +        str = getenv("ALT_STRUCT_SIZE");
>>> +        size = str ? atoi(str) : 12;
>>> +    }
>>> +
>>> +    printf("altinstr_size=%d\n", size);
>>> +    return size;
>>> +}
>>>   /*
>>>    * The rela groups in the .fixup section vary in size.  The beginning of each
>>> @@ -1016,15 +1058,15 @@ static int fixup_group_size(struct kpatch_elf *kelf, int offset)
>>>   static struct special_section special_sections[] = {
>>>       {
>>>           .name        = ".bug_frames.0",
>>> -        .group_size    = bug_frames_0_group_size,
>>> +        .group_size    = bug_frames_group_size,
>>>       },
>>>       {
>>>           .name        = ".bug_frames.1",
>>> -        .group_size    = bug_frames_1_group_size,
>>> +        .group_size    = bug_frames_group_size,
>>>       },
>>>       {
>>>           .name        = ".bug_frames.2",
>>> -        .group_size    = bug_frames_2_group_size,
>>> +        .group_size    = bug_frames_group_size,
>>>       },
>>>       {
>>>           .name        = ".bug_frames.3",
>>> diff --git a/livepatch-build b/livepatch-build
>>> index c057fa1..a6cae12 100755
>>> --- a/livepatch-build
>>> +++ b/livepatch-build
>>> @@ -284,6 +284,29 @@ echo "Output directory: ${OUTPUT}"
>>>   echo "================================================"
>>>   echo
>>> +if [ -f "$XENSYMS" ]; then
>>> +    echo "Reading special section data"
>>> +    SPECIAL_VARS=$(readelf -wi "$XENSYMS" |
>>> +               gawk --non-decimal-data '
>>> +               BEGIN { a = b = e = 0 }
>>> +               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}
>>> +               a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2}
>>> +               b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2}
>>> +               e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2}
>>> +               a == 2 && b == 2 && e == 2 {exit}')
>>> +    [[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS"
>>> +    if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ -z $EX_STRUCT_SIZE ]]; then
>>> +        die "can't find special struct size"
>>> +    fi
>>> +    for i in $ALT_STRUCT_SIZE $BUG_STRUCT_SIZE $EX_STRUCT_SIZE; do
>>> +        if [[ ! $i -gt 0 ]] || [[ ! $i -le 16 ]]; then
>>> +            die "invalid special struct size $i"
>>> +        fi
>>> +    done
>>> +fi
>>> +
>> If this hunk is moved after the call to build_full(), then it can be run directly on the xen-syms that has just been built which would allow dropping `if [ -f "$XENSYMS" ]...` and guaranteeing that *_STRUCT_SIZE are always set.
> 
> One more thing, previously:
> 
> bug_frames_0_group_size == 8
> bug_frames_1_group_size == 8
> bug_frames_2_group_size == 8
> bug_frames_3_group_size == 16
> 
> And now:
> 
> bug_frames_0_group_size == BUG_STRUCT_SIZE
> bug_frames_1_group_size == BUG_STRUCT_SIZE
> bug_frames_2_group_size == BUG_STRUCT_SIZE
> bug_frames_3_group_size == BUG_STRUCT_SIZE
> 
> This seems wrong to me. When reading special section data, should you detect e.g. BUG0_STRUCT_SIZE, BUG1_STRUCT_SIZE, … ?
> 

There is only one struct bug_frame definition in Xen:
Using pahole:

struct bug_frame {
        unsigned char              ud2[2];               /*     0     2 */
        unsigned char              ret;                  /*     2     1 */
        short unsigned int         id;                   /*     3     2 */

        /* size: 5, cachelines: 1, members: 3 */
        /* last cacheline: 5 bytes */
};

It’s size is 5, so fits into 8 bytes.

Definitions for all the 0, 1, 2, 3 groups use the same struct bug_frame:
Example grep:
include/asm-x86/bug.h:extern const struct bug_frame __start_bug_frames[],
include/asm-x86/bug.h:                              __stop_bug_frames_0[],
include/asm-x86/bug.h:                              __stop_bug_frames_1[],
include/asm-x86/bug.h:                              __stop_bug_frames_2[],
include/asm-x86/bug.h:                              __stop_bug_frames_3[];

So, the default group size of 16 bytes does not seem right.

Example grep:
$ objdump -g xen-syms|grep -A3 DW_TAG_structure_type|grep -A1 bug_frame|cut -f2- -d'>'|sort -u
--
   DW_AT_byte_size   : 8
   DW_AT_name        : (indirect string, offset: 0x2556): bug_frame


> -- 
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz






Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 3/6] create-diff-object: Add is_special_section() helper function

[Ross Lagerwall]

On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
> This function determines, based on the given section name, if the
> sections belongs to the special sections category.
> It treats a name from the special_sections array as a prefix. Any
> sections whose name starts with special section prefix is considered
> a special section.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> ---
>   common.c | 23 +++++++++++++++++++++++
>   common.h |  1 +
>   2 files changed, 24 insertions(+)
> 
> diff --git a/common.c b/common.c
> index c968299..a2c860b 100644
> --- a/common.c
> +++ b/common.c
> @@ -290,6 +290,29 @@ int is_referenced_section(const struct section *sec, const struct kpatch_elf *ke
>   	return false;
>   }
>   
> +int is_special_section(struct section *sec)

const for the parameter?

> +{
> +	static struct special_section_names {
> +		const char *name;
> +	} names[] = {
> +		{ .name		= ".bug_frames"                 },
> +		{ .name		= ".fixup"                      },
> +		{ .name		= ".ex_table"                   },
> +		{ .name		= ".altinstructions"            },
> +		{ .name		= ".altinstr_replacement"       },
> +		{ .name		= ".livepatch.hooks"            },
> +		{ .name         = NULL                          },
> +	};
> +	struct special_section_names *special;

Wouldn't it be better to reuse the existing special_sections array 
rather than partially duplicating it here?

> +
> +	for (special = names; special->name; special++) {
> +		if (!strncmp(sec->name, special->name, strlen(special->name)))
> +			return true;

This check is not as precise as it could be, since ".altinstructionsfoo" 
would be considered special when it actually means nothing to this tool.

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 5/6] create-diff-object: Add new entries to special sections array

[Ross Lagerwall]

On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
> Handle .livepatch.hooks* and .altinstr_replacement sections as the
> special sections with assigned group_size resolution function.
> By default each .livepatch.hooks* sections' entry is 8 bytes long (a
> pointer). The .altinstr_replacement section follows the .altinstructions
> section settings.
> 
> Allow to specify different .livepatch.hooks* section entry size using
> shell environment variable HOOK_STRUCT_SIZE.
> 
> Cleanup an incorrect indentation around group_size resulution functions.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> ---
>   create-diff-object.c | 87 ++++++++++++++++++++++++++++++++--------------------
>   1 file changed, 54 insertions(+), 33 deletions(-)
> 
> diff --git a/create-diff-object.c b/create-diff-object.c
> index b0b4dcb..f6060cd 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -960,51 +960,64 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf)

Fix the indentation in the patch which introduces the problem rather 
than fixing it in a subsequent patch.

>   
>   static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
>   {
> -    static int size = 0;
> -    char *str;
> -    if (!size) {
> -        str = getenv("BUG_STRUCT_SIZE");
> -        size = str ? atoi(str) : 8;
> -    }
> -
> -    return size;
> +	static int size = 0;
> +	char *str;
> +	if (!size) {
> +		str = getenv("BUG_STRUCT_SIZE");
> +		size = str ? atoi(str) : 8;
> +	}
> +
> +	return size;
>   }
>   
>   static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
>   {
> -    static int size = 0;
> -    char *str;
> -    if (!size) {
> -        str = getenv("BUG_STRUCT_SIZE");
> -        size = str ? atoi(str) : 16;
> -    }
> -
> -    return size;
> +	static int size = 0;
> +	char *str;
> +	if (!size) {
> +		str = getenv("BUG_STRUCT_SIZE");
> +		size = str ? atoi(str) : 16;
> +	}
> +
> +	return size;
>   }
>   
>   static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
>   {
> -    static int size = 0;
> -    char *str;
> -    if (!size) {
> -        str = getenv("EX_STRUCT_SIZE");
> -        size = str ? atoi(str) : 8;
> -    }
> -
> -    return size;
> +	static int size = 0;
> +	char *str;
> +	if (!size) {
> +		str = getenv("EX_STRUCT_SIZE");
> +		size = str ? atoi(str) : 8;
> +	}
> +
> +	return size;
>   }
>   
>   static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
>   {
> -    static int size = 0;
> -    char *str;
> -    if (!size) {
> -        str = getenv("ALT_STRUCT_SIZE");
> -        size = str ? atoi(str) : 12;
> -    }
> -
> -    printf("altinstr_size=%d\n", size);
> -    return size;
> +	static int size = 0;
> +	char *str;
> +	if (!size) {
> +		str = getenv("ALT_STRUCT_SIZE");
> +		size = str ? atoi(str) : 12;
> +	}
> +
> +	printf("altinstr_size=%d\n", size);
> +	return size;
> +}
> +
> +static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset)
> +{
> +	static int size = 0;
> +	char *str;
> +	if (!size) {
> +		str = getenv("HOOK_STRUCT_SIZE");
> +		size = str ? atoi(str) : 8;
> +	}
> +
> +	printf("livepatch_hooks_size=%d\n", size);

If you want to keep this debugging output, rather use log_debug().

> +	return size;
>   }
>   
>   /*
> @@ -1084,6 +1097,14 @@ static struct special_section special_sections[] = {
>   		.name		= ".altinstructions",
>   		.group_size	= altinstructions_group_size,
>   	},
> +	{
> +		.name		= ".altinstr_replacement",
> +		.group_size	= altinstructions_group_size,
> +	},
> +	{
> +		.name		= ".livepatch.hooks",
> +		.group_size	= livepatch_hooks_group_size,
> +	},
>   	{},
>   };
>   
> 

I don't understand the point of this change.

IIUC unlike .altinstructions, the .altinstr_replacement section does not 
have a fixed group size, so this change would not work other than by 
luck. If the goal was to prune bits of the .altinstr_replacement table 
that do not need to be included, you need to update the code in 
kpatch_process_special_sections() which has special handling for 
.altinstr_replacement. It needs to parse the updated, pruned 
.altinstructions section and regenerate .altinstr_replacement, including 
only what is needed.

I don't know why .livepatch.hooks is included in this list either since 
all hooks are always included and there is nothing to regenerate/prune.

Regards,
-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 2/6] common: Add is_referenced_section() helper function

[Wieczorkiewicz, Pawel]

> On 29. Apr 2019, at 17:14, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
> 
> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>> This function checks if given section has an included corresponding
>> RELA section and/or any of the symbols table symbols references the
>> section. Section associated symbols are ignored here as there is
>> always such a symbol for every section.
>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
>> ---
>>  common.c | 22 +++++++++++++++++++++-
>>  common.h |  1 +
>>  2 files changed, 22 insertions(+), 1 deletion(-)
>> diff --git a/common.c b/common.c
>> index 1fb07cb..c968299 100644
>> --- a/common.c
>> +++ b/common.c
>> @@ -15,7 +15,7 @@
>>    int is_rela_section(struct section *sec)
>>  {
>> -	return (sec->sh.sh_type == SHT_RELA);
>> +	return sec && (sec->sh.sh_type == SHT_RELA);
>>  }
>>    int is_local_sym(struct symbol *sym)
>> @@ -270,6 +270,26 @@ int is_standard_section(struct section *sec)
>>  	}
>>  }
>>  +int is_referenced_section(const struct section *sec, const struct kpatch_elf *kelf)
> 
> Let's keep to 80 chars where practical (and throughout the rest of the patches).

ACK. Will fix. Although, sometimes it makes the code pretty unreadable.

> 
>> +{
>> +	struct symbol *sym;
>> +
>> +	if (is_rela_section(sec->rela) && sec->rela->include)
>> +		return true;
>> +
>> +	list_for_each_entry(sym, &kelf->symbols, list) {
>> +		if (!sym->include || !sym->sec)
>> +			continue;
>> +		/* Ignore section associated sections */
>> +		if (sym->type == STT_SECTION)
>> +			continue;
>> +		if (sym->sec->index == sec->index)
>> +			return true;
> 
> You can simplify this check to `sym->sec == sec` like the rest of the code does.

Might be my paranoia again (or I am simply missing some obvious assumptions/invariants),
but I explicitly wanted to check whether given section/symbol is referenced using other means
than addresses.

> 
> -- 
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 6/6] create-diff-object: Do not include all .rodata sections

[Ross Lagerwall]

On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
> Starting with the "2af6f1aa6233 Fix patch creation with GCC 6.1+" commit
> all .rodata sections are included by default (regardless of whether they
> are needed or not).
> During stacked hotpatch builds it leads to unnecessary duplication of
> the .rodata sections as each and every consecutive hotpatch contains all
> the .rodata section of previous hotpatches.

This commit message is a bit confusing.

1) All of this only applies to .rodata.str* sections. Other .rodata 
sections are handled separately.

2) The above commit _did not_ introduce the problem. Previous versions 
of GCC did not split .rodata.str sections by function which meant that 
the entire section was always included. The commit fixed patch creation 
and _maintained_ the existing behaviour for GCC 6.1+ by including all 
the .rodata.str* sections. This patch now includes only what is needed 
(but it should be noted that this would likely not be useful on older 
versions of GCC since they don't split .rodata.str properly).

> 
> To prevent this situation, mark the .rodata section for inclusion only
> if they are referenced by any of the current hotpatch symbols (or a
> corresponding RELA section).
> 
> Extend patchability verification to detect all non-standard, non-rela,
> non-debug and non-special sections that are not referenced by any of
> the symbols or RELA sections.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> ---
>   create-diff-object.c | 27 ++++++++++++++++++++++++++-
>   1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/create-diff-object.c b/create-diff-object.c
> index f6060cd..f7eb421 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -1341,7 +1341,7 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
>   
>   	list_for_each_entry(sec, &kelf->sections, list) {
>   		/* include these sections even if they haven't changed */
> -		if (is_standard_section(sec) || should_include_str_section(sec->name)) {
> +		if (is_standard_section(sec)) {
>   			sec->include = 1;
>   			if (sec->secsym)
>   				sec->secsym->include = 1;
> @@ -1352,6 +1352,19 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
>   	list_entry(kelf->symbols.next, struct symbol, list)->include = 1;
>   }
>   
> +static void kpatch_include_standard_string_elements(struct kpatch_elf *kelf)
> +{
> +	struct section *sec;
> +
> +	list_for_each_entry(sec, &kelf->sections, list) {
> +		if (should_include_str_section(sec->name) && is_referenced_section(sec, kelf)) {
> +			sec->include = 1;
> +			if (sec->secsym)
> +				sec->secsym->include = 1;
> +		}
> +	}
> +}

I think it would be simpler to just amend the previous function rather 
than introducing a new one. E.g.

... || (should_include_str_section(sec->name) && 
is_referenced_section(sec, kelf))

Regards,
-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 4/6] livepatch-build: detect special section group sizes

[Glenn Enright]

On 30/04/19 2:14 AM, Ross Lagerwall wrote:
> On 4/25/19 5:53 AM, Konrad Rzeszutek Wilk wrote:
>> On Tue, Apr 16, 2019 at 12:07:14PM +0000, Pawel Wieczorkiewicz wrote:
>>> Hard-coding the special section group sizes is unreliable. Instead,
>>> determine them dynamically by finding the related struct definitions
>>> in the DWARF metadata.
>>>
>>> This is a livepatch backport of kpatch upstream commit [1]:
>>> kpatch-build: detect special section group sizes 170449847136a48b19fc
>>>
>>> Xen only deals with alt_instr, bug_frame and exception_table_entry
>>> structures, so sizes of these structers are obtained from xen-syms.
>>>
>>> This change is needed since with recent Xen the alt_instr structure
>>> has changed size from 12 to 14 bytes.
>>
>> Oh this is so much better than the "solution" we coded.
>>
>> Thank you!
>>
>> Ross, will commit to repo unless you have concerns..
> I have reviewed it with a few comments. Note that this is basically the
> same as Glenn Enright's recent patch ("livepatch-build-tools: Detect
> special section group sizes") but slightly more complete as it detects
> sizes for the bug frames too so it should probably be used instead.
> 
> Thanks,


This is a goodness. Glad to see an appropriate patch get in regardless
of which patch is used.

Best, Glenn

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 3/6] create-diff-object: Add is_special_section() helper function

[Wieczorkiewicz, Pawel]

> On 29. Apr 2019, at 17:25, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
> 
> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>> This function determines, based on the given section name, if the
>> sections belongs to the special sections category.
>> It treats a name from the special_sections array as a prefix. Any
>> sections whose name starts with special section prefix is considered
>> a special section.
>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
>> ---
>>  common.c | 23 +++++++++++++++++++++++
>>  common.h |  1 +
>>  2 files changed, 24 insertions(+)
>> diff --git a/common.c b/common.c
>> index c968299..a2c860b 100644
>> --- a/common.c
>> +++ b/common.c
>> @@ -290,6 +290,29 @@ int is_referenced_section(const struct section *sec, const struct kpatch_elf *ke
>>  	return false;
>>  }
>>  +int is_special_section(struct section *sec)
> 
> const for the parameter?

ACK. Will add.

> 
>> +{
>> +	static struct special_section_names {
>> +		const char *name;
>> +	} names[] = {
>> +		{ .name		= ".bug_frames"                 },
>> +		{ .name		= ".fixup"                      },
>> +		{ .name		= ".ex_table"                   },
>> +		{ .name		= ".altinstructions"            },
>> +		{ .name		= ".altinstr_replacement"       },
>> +		{ .name		= ".livepatch.hooks"            },
>> +		{ .name         = NULL                          },
>> +	};
>> +	struct special_section_names *special;
> 
> Wouldn't it be better to reuse the existing special_sections array rather than partially duplicating it here?

After looking at this code again, I think you’re right and it would be better.
It would require to explicitly call out all sections to be considered special in
the special_sections array, but this is actually what I need for further changes anyway.

ACK. Will fix. 

> 
>> +
>> +	for (special = names; special->name; special++) {
>> +		if (!strncmp(sec->name, special->name, strlen(special->name)))
>> +			return true;
> 
> This check is not as precise as it could be, since ".altinstructionsfoo" would be considered special when it actually means nothing to this tool.

Given the above reasoning, that’s right.

ACK. Will fix.

> 
> -- 
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 5/6] create-diff-object: Add new entries to special sections array

[Wieczorkiewicz, Pawel]

[-- Attachment #1.1: Type: text/plain, Size: 5930 bytes --]


On 29. Apr 2019, at 17:47, Ross Lagerwall <ross.lagerwall@citrix.com<mailto:ross.lagerwall@citrix.com>> wrote:

On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
Handle .livepatch.hooks* and .altinstr_replacement sections as the
special sections with assigned group_size resolution function.
By default each .livepatch.hooks* sections' entry is 8 bytes long (a
pointer). The .altinstr_replacement section follows the .altinstructions
section settings.
Allow to specify different .livepatch.hooks* section entry size using
shell environment variable HOOK_STRUCT_SIZE.
Cleanup an incorrect indentation around group_size resulution functions.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de<mailto:wipawel@amazon.de>>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com<mailto:andraprs@amazon.com>>
Reviewed-by: Bjoern Doebel <doebel@amazon.de<mailto:doebel@amazon.de>>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de<mailto:nmanthey@amazon.de>>
---
 create-diff-object.c | 87 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 33 deletions(-)
diff --git a/create-diff-object.c b/create-diff-object.c
index b0b4dcb..f6060cd 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -960,51 +960,64 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf)

Fix the indentation in the patch which introduces the problem rather than fixing it in a subsequent patch.

Oh, certainly. I forgot to update the commits involved properly.

ACK. Will fix.


   static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
 {
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("BUG_STRUCT_SIZE");
-        size = str ? atoi(str) : 8;
-    }
-
-    return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("BUG_STRUCT_SIZE");
+ size = str ? atoi(str) : 8;
+ }
+
+ return size;
 }
   static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
 {
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("BUG_STRUCT_SIZE");
-        size = str ? atoi(str) : 16;
-    }
-
-    return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("BUG_STRUCT_SIZE");
+ size = str ? atoi(str) : 16;
+ }
+
+ return size;
 }
   static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
 {
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("EX_STRUCT_SIZE");
-        size = str ? atoi(str) : 8;
-    }
-
-    return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("EX_STRUCT_SIZE");
+ size = str ? atoi(str) : 8;
+ }
+
+ return size;
 }
   static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
 {
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("ALT_STRUCT_SIZE");
-        size = str ? atoi(str) : 12;
-    }
-
-    printf("altinstr_size=%d\n", size);
-    return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("ALT_STRUCT_SIZE");
+ size = str ? atoi(str) : 12;
+ }
+
+ printf("altinstr_size=%d\n", size);
+ return size;
+}
+
+static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset)
+{
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("HOOK_STRUCT_SIZE");
+ size = str ? atoi(str) : 8;
+ }
+
+ printf("livepatch_hooks_size=%d\n", size);

If you want to keep this debugging output, rather use log_debug().

ACK. Will fix.


+ return size;
 }
   /*
@@ -1084,6 +1097,14 @@ static struct special_section special_sections[] = {
  .name = ".altinstructions",
  .group_size = altinstructions_group_size,
  },
+ {
+ .name = ".altinstr_replacement",
+ .group_size = altinstructions_group_size,
+ },
+ {
+ .name = ".livepatch.hooks",
+ .group_size = livepatch_hooks_group_size,
+ },
  {},
 };


I don't understand the point of this change.

IIUC unlike .altinstructions, the .altinstr_replacement section does not have a fixed group size, so this change would not work other than by luck. If

That’s true. The .altinstr_replacement group_size should be 0 (aka undefined).

ACK. Will fix.

the goal was to prune bits of the .altinstr_replacement table that do not need to be included, you need to update the code in kpatch_process_special_sections() which has special handling for .altinstr_replacement. It needs to parse the updated, pruned .altinstructions section and regenerate .altinstr_replacement, including only what is needed.


No this was not the goal. I am explaining it below.

I don't know why .livepatch.hooks is included in this list either since all hooks are always included and there is nothing to regenerate/prune.

As discussed when reviewing [1], I need a function telling which section is special.
By special I mean:
a) should not be a subject of extra validation (See [2])
b) should not be a subject of STN_UNDEF purging (See [3])

Thus, I want to explicitly call out all "special" sections by their name and specify
their group_size whenever applicable (I did it incorrectly for .altinstr_replacement,
as you have spotted - thanks!) in the common array and use it with the helper
function or for obtaining a group_size.

[1] [livepatch-build-tools part2 3/6] create-diff-object: Add is_special_section() helper function
[2] [livepatch-build-tools part3 2/3] create-diff-object: Extend patchability verification: STN_UNDEF
[3] [livepatch-build-tools part3 3/3] create-diff-object: Strip all undefined entires of known size


Regards,
--
Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B


[-- Attachment #1.2: Type: text/html, Size: 15466 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools part2 6/6] create-diff-object: Do not include all .rodata sections

[Wieczorkiewicz, Pawel]

> On 29. Apr 2019, at 18:11, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
> 
> On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:
>> Starting with the "2af6f1aa6233 Fix patch creation with GCC 6.1+" commit
>> all .rodata sections are included by default (regardless of whether they
>> are needed or not).
>> During stacked hotpatch builds it leads to unnecessary duplication of
>> the .rodata sections as each and every consecutive hotpatch contains all
>> the .rodata section of previous hotpatches.
> 
> This commit message is a bit confusing.
> 
> 1) All of this only applies to .rodata.str* sections. Other .rodata sections are handled separately.

Yes, that’s right. I will make the commit message precise.

ACK. Will fix.

> 
> 2) The above commit _did not_ introduce the problem. Previous versions of GCC did not split .rodata.str sections by function which meant that the entire section was always included. The commit fixed patch creation and _maintained_ the existing behaviour for GCC 6.1+ by including all the

I see, thanks for the clarification. I will update the wording to avoid confusion and incorrect attribution here.

> .rodata.str* sections. This patch now includes only what is needed (but it should be noted that this would likely not be useful on older versions of GCC since they don't split .rodata.str properly).

OK, I suppose users of older versions of GCC have to accept the fact.

> 
>> To prevent this situation, mark the .rodata section for inclusion only
>> if they are referenced by any of the current hotpatch symbols (or a
>> corresponding RELA section).
>> Extend patchability verification to detect all non-standard, non-rela,
>> non-debug and non-special sections that are not referenced by any of
>> the symbols or RELA sections.
>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
>> ---
>>  create-diff-object.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>> diff --git a/create-diff-object.c b/create-diff-object.c
>> index f6060cd..f7eb421 100644
>> --- a/create-diff-object.c
>> +++ b/create-diff-object.c
>> @@ -1341,7 +1341,7 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
>>    	list_for_each_entry(sec, &kelf->sections, list) {
>>  		/* include these sections even if they haven't changed */
>> -		if (is_standard_section(sec) || should_include_str_section(sec->name)) {
>> +		if (is_standard_section(sec)) {
>>  			sec->include = 1;
>>  			if (sec->secsym)
>>  				sec->secsym->include = 1;
>> @@ -1352,6 +1352,19 @@ static void kpatch_include_standard_elements(struct kpatch_elf *kelf)
>>  	list_entry(kelf->symbols.next, struct symbol, list)->include = 1;
>>  }
>>  +static void kpatch_include_standard_string_elements(struct kpatch_elf *kelf)
>> +{
>> +	struct section *sec;
>> +
>> +	list_for_each_entry(sec, &kelf->sections, list) {
>> +		if (should_include_str_section(sec->name) && is_referenced_section(sec, kelf)) {
>> +			sec->include = 1;
>> +			if (sec->secsym)
>> +				sec->secsym->include = 1;
>> +		}
>> +	}
>> +}
> 
> I think it would be simpler to just amend the previous function rather than introducing a new one. E.g.
> 
> ... || (should_include_str_section(sec->name) && is_referenced_section(sec, kelf))

IIRC, it is unfortunately too early to do that from within kpatch_include_standard_elements().
I want to call the kpatch_include_standard_string_elements() after the following functions are called:
- kpatch_include_changed_functions()
- kpatch_include_debug_sections()
- kpatch_include_hook_elements()
because they may affect is_referenced_section() result.

> 
> Regards,
> -- 
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel