All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] livepatch: further fixes
@ 2022-03-10 15:08 Roger Pau Monne
  2022-03-10 15:08 ` [PATCH 1/3] livepatch: use basename to perform object file matching Roger Pau Monne
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Roger Pau Monne @ 2022-03-10 15:08 UTC (permalink / raw)
  To: xen-devel; +Cc: Konrad Rzeszutek Wilk, Ross Lagerwall, doebel, Roger Pau Monne

Hello,

The following series does some more cleanup after the Xen hypervisor
build changes (patches 1 and 2). Patch 3 fixes handling of .altinstr*
sections.

Thanks, Roger.

Roger Pau Monne (3):
  livepatch: use basename to perform object file matching
  livepatch: add extra efi/ objects to be ignored
  livepatch: correctly handle altinstruction sections

 common.c             |  7 +++++--
 create-diff-object.c | 26 --------------------------
 livepatch-gcc        | 14 +++++++++-----
 3 files changed, 14 insertions(+), 33 deletions(-)

-- 
2.34.1



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

* [PATCH 1/3] livepatch: use basename to perform object file matching
  2022-03-10 15:08 [PATCH 0/3] livepatch: further fixes Roger Pau Monne
@ 2022-03-10 15:08 ` Roger Pau Monne
  2022-03-10 17:22   ` Doebel, Bjoern
  2022-03-10 15:08 ` [PATCH 2/3] livepatch: add extra efi/ objects to be ignored Roger Pau Monne
  2022-03-10 15:08 ` [PATCH 3/3] livepatch: correctly handle altinstruction sections Roger Pau Monne
  2 siblings, 1 reply; 11+ messages in thread
From: Roger Pau Monne @ 2022-03-10 15:08 UTC (permalink / raw)
  To: xen-devel; +Cc: Konrad Rzeszutek Wilk, Ross Lagerwall, doebel, Roger Pau Monne

The changes in the Xen build logic has resulted in the compiler and
objcopy being called from xen/ instead of relative to each object
directory. This requires using basename so that the directory is not
taken into account when checking against the list of files to be
explicitly ignored.

Also adjust the paths used to store the differing object files, as
with the current logic the resulting path will be wrong when using
newer Xen versions, changed_objs would end containing entries like:

xen/arch/x86/hvm/vmx/arch/x86/hvm/vmx/vmx.o

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 livepatch-gcc | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/livepatch-gcc b/livepatch-gcc
index 91333d5..fe782e0 100755
--- a/livepatch-gcc
+++ b/livepatch-gcc
@@ -32,10 +32,10 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
         if [ "$1" = "-o" ]; then
             obj=$2
             [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
-            case "$obj" in
+            case "$(basename $obj)" in
             version.o|\
             debug.o|\
-            efi/check.o|\
+            check.o|\
             *.xen-syms.*.o|\
             *.xen.efi.*.o|\
             built_in.o|\
@@ -46,6 +46,7 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
             *.o)
                 path="$(pwd)/$(dirname $obj)"
                 dir="${path#$LIVEPATCH_BUILD_DIR}"
+                obj=$(basename $obj)
                 if [ -n "$LIVEPATCH_CAPTURE_DIR" -a -d "$LIVEPATCH_CAPTURE_DIR" ]; then
                     echo "$dir/$obj" >> "${LIVEPATCH_CAPTURE_DIR}/changed_objs"
                     keep=yes
@@ -61,15 +62,16 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
 done
 elif [[ "$TOOLCHAINCMD" =~ $OBJCOPY_RE ]] ; then
     obj="${!#}"
-    case "$obj" in
+    case "$(basename $obj)" in
         version.o|\
         debug.o|\
-        efi/check.o|\
+        check.o|\
         .*.o)
             ;;
         *.o)
             path="$(pwd)/$(dirname $obj)"
             dir="${path#$LIVEPATCH_BUILD_DIR}"
+            obj=$(basename $obj)
             if [ -n "$LIVEPATCH_CAPTURE_DIR" -a -d "$LIVEPATCH_CAPTURE_DIR" ]; then
                 echo "$dir/$obj" >> "${LIVEPATCH_CAPTURE_DIR}/changed_objs"
                 keep=yes
@@ -85,7 +87,7 @@ ret="$?"
 
 if [[ "$keep" = "yes" ]] ; then
     mkdir -p "$(dirname $LIVEPATCH_CAPTURE_DIR/$dir/$obj)"
-    cp "$obj" "$LIVEPATCH_CAPTURE_DIR/$dir/$obj"
+    cp "$path/$obj" "$LIVEPATCH_CAPTURE_DIR/$dir/$obj"
 fi
 
 exit "$ret"
-- 
2.34.1



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

* [PATCH 2/3] livepatch: add extra efi/ objects to be ignored
  2022-03-10 15:08 [PATCH 0/3] livepatch: further fixes Roger Pau Monne
  2022-03-10 15:08 ` [PATCH 1/3] livepatch: use basename to perform object file matching Roger Pau Monne
@ 2022-03-10 15:08 ` Roger Pau Monne
  2022-03-10 17:23   ` Doebel, Bjoern
  2022-03-10 15:08 ` [PATCH 3/3] livepatch: correctly handle altinstruction sections Roger Pau Monne
  2 siblings, 1 reply; 11+ messages in thread
From: Roger Pau Monne @ 2022-03-10 15:08 UTC (permalink / raw)
  To: xen-devel; +Cc: Konrad Rzeszutek Wilk, Ross Lagerwall, doebel, Roger Pau Monne

The contents of this objects is init only, and cannot be patched.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 livepatch-gcc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/livepatch-gcc b/livepatch-gcc
index fe782e0..b0b9ce4 100755
--- a/livepatch-gcc
+++ b/livepatch-gcc
@@ -66,6 +66,8 @@ elif [[ "$TOOLCHAINCMD" =~ $OBJCOPY_RE ]] ; then
         version.o|\
         debug.o|\
         check.o|\
+        boot.o|\
+        *.init.o|\
         .*.o)
             ;;
         *.o)
-- 
2.34.1



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

* [PATCH 3/3] livepatch: correctly handle altinstruction sections
  2022-03-10 15:08 [PATCH 0/3] livepatch: further fixes Roger Pau Monne
  2022-03-10 15:08 ` [PATCH 1/3] livepatch: use basename to perform object file matching Roger Pau Monne
  2022-03-10 15:08 ` [PATCH 2/3] livepatch: add extra efi/ objects to be ignored Roger Pau Monne
@ 2022-03-10 15:08 ` Roger Pau Monne
  2022-03-10 17:25   ` Doebel, Bjoern
                     ` (2 more replies)
  2 siblings, 3 replies; 11+ messages in thread
From: Roger Pau Monne @ 2022-03-10 15:08 UTC (permalink / raw)
  To: xen-devel; +Cc: Konrad Rzeszutek Wilk, Ross Lagerwall, doebel, Roger Pau Monne

The current handling of altinstructions sections by the livepatch
tools is incorrect, as on Xen those sections are part of .init and
thus discarded after load. Correctly handle them by just ignoring, as
it's done with other .init related sections.

While there also add .data.ro_after_init section as a read-only
section and introduce some syntactic sugar for comparing section
names.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
I assume this handling of .altinstr* sections was inherited from Linux
where the sections are not discarded after load in order to apply
alternative patching to the loaded modules after boot.
---
 common.c             |  7 +++++--
 create-diff-object.c | 26 --------------------------
 2 files changed, 5 insertions(+), 28 deletions(-)

diff --git a/common.c b/common.c
index 68a71f7..a148d8a 100644
--- a/common.c
+++ b/common.c
@@ -249,19 +249,22 @@ int is_text_section(struct section *sec)
 		(sec->sh.sh_flags & SHF_EXECINSTR));
 }
 
+#define SEC_MATCH(n) !strncmp(sec->name, n, strlen(n) - 1)
 int is_rodata_section(struct section *sec)
 {
 	return sec->sh.sh_type == SHT_PROGBITS &&
 	       !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) &&
-	       !strncmp(sec->name, ".rodata", 7);
+	       (SEC_MATCH(".rodata") || SEC_MATCH(".data.ro_after_init"));
 }
 
 int is_init_section(struct section *sec)
 {
 	return sec->sh.sh_type == SHT_PROGBITS &&
 	       (sec->sh.sh_flags & SHF_ALLOC) &&
-	       !strncmp(sec->name, ".init", 5);
+	       (SEC_MATCH(".init") || SEC_MATCH(".text.startup") ||
+	        SEC_MATCH(".altinstr") || SEC_MATCH(".ctors"));
 }
+#undef SEC_MATCH
 
 int is_debug_section(struct section *sec)
 {
diff --git a/create-diff-object.c b/create-diff-object.c
index a516670..ec2afb4 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -995,19 +995,6 @@ static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
 	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;
-	}
-
-	log_debug("altinstr_size=%d\n", size);
-	return size;
-}
-
 static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset)
 {
 	static int size = 0;
@@ -1021,11 +1008,6 @@ static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset)
 	return size;
 }
 
-static int undefined_group_size(struct kpatch_elf *kelf, int offset)
-{
-	return 0;
-}
-
 /*
  * The rela groups in the .fixup section vary in size.  The beginning of each
  * .fixup rela group is referenced by the .ex_table section. To find the size
@@ -1099,14 +1081,6 @@ static struct special_section special_sections[] = {
 		.name		= ".ex_table",
 		.group_size	= ex_table_group_size,
 	},
-	{
-		.name		= ".altinstructions",
-		.group_size	= altinstructions_group_size,
-	},
-	{
-		.name		= ".altinstr_replacement",
-		.group_size	= undefined_group_size,
-	},
 	{
 		.name		= ".livepatch.hooks.load",
 		.group_size	= livepatch_hooks_group_size,
-- 
2.34.1



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

* Re: [PATCH 1/3] livepatch: use basename to perform object file matching
  2022-03-10 15:08 ` [PATCH 1/3] livepatch: use basename to perform object file matching Roger Pau Monne
@ 2022-03-10 17:22   ` Doebel, Bjoern
  0 siblings, 0 replies; 11+ messages in thread
From: Doebel, Bjoern @ 2022-03-10 17:22 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel; +Cc: Konrad Rzeszutek Wilk, Ross Lagerwall



On 10.03.22 16:08, Roger Pau Monne wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> The changes in the Xen build logic has resulted in the compiler and
> objcopy being called from xen/ instead of relative to each object
> directory. This requires using basename so that the directory is not
> taken into account when checking against the list of files to be
> explicitly ignored.
> 
> Also adjust the paths used to store the differing object files, as
> with the current logic the resulting path will be wrong when using
> newer Xen versions, changed_objs would end containing entries like:
> 
> xen/arch/x86/hvm/vmx/arch/x86/hvm/vmx/vmx.o
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   livepatch-gcc | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/livepatch-gcc b/livepatch-gcc
> index 91333d5..fe782e0 100755
> --- a/livepatch-gcc
> +++ b/livepatch-gcc
> @@ -32,10 +32,10 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
>           if [ "$1" = "-o" ]; then
>               obj=$2
>               [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
> -            case "$obj" in
> +            case "$(basename $obj)" in
>               version.o|\
>               debug.o|\
> -            efi/check.o|\
> +            check.o|\
>               *.xen-syms.*.o|\
>               *.xen.efi.*.o|\
>               built_in.o|\
> @@ -46,6 +46,7 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
>               *.o)
>                   path="$(pwd)/$(dirname $obj)"
>                   dir="${path#$LIVEPATCH_BUILD_DIR}"
> +                obj=$(basename $obj)
>                   if [ -n "$LIVEPATCH_CAPTURE_DIR" -a -d "$LIVEPATCH_CAPTURE_DIR" ]; then
>                       echo "$dir/$obj" >> "${LIVEPATCH_CAPTURE_DIR}/changed_objs"
>                       keep=yes
> @@ -61,15 +62,16 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
>   done
>   elif [[ "$TOOLCHAINCMD" =~ $OBJCOPY_RE ]] ; then
>       obj="${!#}"
> -    case "$obj" in
> +    case "$(basename $obj)" in
>           version.o|\
>           debug.o|\
> -        efi/check.o|\
> +        check.o|\
>           .*.o)
>               ;;
>           *.o)
>               path="$(pwd)/$(dirname $obj)"
>               dir="${path#$LIVEPATCH_BUILD_DIR}"
> +            obj=$(basename $obj)
>               if [ -n "$LIVEPATCH_CAPTURE_DIR" -a -d "$LIVEPATCH_CAPTURE_DIR" ]; then
>                   echo "$dir/$obj" >> "${LIVEPATCH_CAPTURE_DIR}/changed_objs"
>                   keep=yes
> @@ -85,7 +87,7 @@ ret="$?"
> 
>   if [[ "$keep" = "yes" ]] ; then
>       mkdir -p "$(dirname $LIVEPATCH_CAPTURE_DIR/$dir/$obj)"
> -    cp "$obj" "$LIVEPATCH_CAPTURE_DIR/$dir/$obj"
> +    cp "$path/$obj" "$LIVEPATCH_CAPTURE_DIR/$dir/$obj"
>   fi
> 
>   exit "$ret"
> --
> 2.34.1

Reviewed-by: Bjoern Doebel <doebel@amazon.de>



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



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

* Re: [PATCH 2/3] livepatch: add extra efi/ objects to be ignored
  2022-03-10 15:08 ` [PATCH 2/3] livepatch: add extra efi/ objects to be ignored Roger Pau Monne
@ 2022-03-10 17:23   ` Doebel, Bjoern
  0 siblings, 0 replies; 11+ messages in thread
From: Doebel, Bjoern @ 2022-03-10 17:23 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel; +Cc: Konrad Rzeszutek Wilk, Ross Lagerwall



On 10.03.22 16:08, Roger Pau Monne wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> The contents of this objects is init only, and cannot be patched.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>   livepatch-gcc | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/livepatch-gcc b/livepatch-gcc
> index fe782e0..b0b9ce4 100755
> --- a/livepatch-gcc
> +++ b/livepatch-gcc
> @@ -66,6 +66,8 @@ elif [[ "$TOOLCHAINCMD" =~ $OBJCOPY_RE ]] ; then
>           version.o|\
>           debug.o|\
>           check.o|\
> +        boot.o|\
> +        *.init.o|\
>           .*.o)
>               ;;
>           *.o)
> --
> 2.34.1
> 

Reviewed-by: Bjoern Doebel <doebel@amazon.de>



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



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

* Re: [PATCH 3/3] livepatch: correctly handle altinstruction sections
  2022-03-10 15:08 ` [PATCH 3/3] livepatch: correctly handle altinstruction sections Roger Pau Monne
@ 2022-03-10 17:25   ` Doebel, Bjoern
  2022-03-11  7:35   ` Jan Beulich
  2022-03-15 15:41   ` Roger Pau Monné
  2 siblings, 0 replies; 11+ messages in thread
From: Doebel, Bjoern @ 2022-03-10 17:25 UTC (permalink / raw)
  To: Roger Pau Monne, xen-devel; +Cc: Konrad Rzeszutek Wilk, Ross Lagerwall



On 10.03.22 16:08, Roger Pau Monne wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> The current handling of altinstructions sections by the livepatch
> tools is incorrect, as on Xen those sections are part of .init and
> thus discarded after load. Correctly handle them by just ignoring, as
> it's done with other .init related sections.
> 
> While there also add .data.ro_after_init section as a read-only
> section and introduce some syntactic sugar for comparing section
> names.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> I assume this handling of .altinstr* sections was inherited from Linux
> where the sections are not discarded after load in order to apply
> alternative patching to the loaded modules after boot.
> ---
>   common.c             |  7 +++++--
>   create-diff-object.c | 26 --------------------------
>   2 files changed, 5 insertions(+), 28 deletions(-)
> 
> diff --git a/common.c b/common.c
> index 68a71f7..a148d8a 100644
> --- a/common.c
> +++ b/common.c
> @@ -249,19 +249,22 @@ int is_text_section(struct section *sec)
>                  (sec->sh.sh_flags & SHF_EXECINSTR));
>   }
> 
> +#define SEC_MATCH(n) !strncmp(sec->name, n, strlen(n) - 1)
>   int is_rodata_section(struct section *sec)
>   {
>          return sec->sh.sh_type == SHT_PROGBITS &&
>                 !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) &&
> -              !strncmp(sec->name, ".rodata", 7);
> +              (SEC_MATCH(".rodata") || SEC_MATCH(".data.ro_after_init"));
>   }
> 
>   int is_init_section(struct section *sec)
>   {
>          return sec->sh.sh_type == SHT_PROGBITS &&
>                 (sec->sh.sh_flags & SHF_ALLOC) &&
> -              !strncmp(sec->name, ".init", 5);
> +              (SEC_MATCH(".init") || SEC_MATCH(".text.startup") ||
> +               SEC_MATCH(".altinstr") || SEC_MATCH(".ctors"));
>   }
> +#undef SEC_MATCH
> 
>   int is_debug_section(struct section *sec)
>   {
> diff --git a/create-diff-object.c b/create-diff-object.c
> index a516670..ec2afb4 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -995,19 +995,6 @@ static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
>          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;
> -       }
> -
> -       log_debug("altinstr_size=%d\n", size);
> -       return size;
> -}
> -
>   static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset)
>   {
>          static int size = 0;
> @@ -1021,11 +1008,6 @@ static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset)
>          return size;
>   }
> 
> -static int undefined_group_size(struct kpatch_elf *kelf, int offset)
> -{
> -       return 0;
> -}
> -
>   /*
>    * The rela groups in the .fixup section vary in size.  The beginning of each
>    * .fixup rela group is referenced by the .ex_table section. To find the size
> @@ -1099,14 +1081,6 @@ static struct special_section special_sections[] = {
>                  .name           = ".ex_table",
>                  .group_size     = ex_table_group_size,
>          },
> -       {
> -               .name           = ".altinstructions",
> -               .group_size     = altinstructions_group_size,
> -       },
> -       {
> -               .name           = ".altinstr_replacement",
> -               .group_size     = undefined_group_size,
> -       },
>          {
>                  .name           = ".livepatch.hooks.load",
>                  .group_size     = livepatch_hooks_group_size,
> --
> 2.34.1
> 

Confirming, this solves the altsection issue I reported via 
https://lore.kernel.org/xen-devel/b74a68b038c31df4bb94a5b5e87453f5a249cfe2.1646753657.git.doebel@amazon.de/

Reviewed-by: Bjoern Doebel <doebel@amazon.de>



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



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

* Re: [PATCH 3/3] livepatch: correctly handle altinstruction sections
  2022-03-10 15:08 ` [PATCH 3/3] livepatch: correctly handle altinstruction sections Roger Pau Monne
  2022-03-10 17:25   ` Doebel, Bjoern
@ 2022-03-11  7:35   ` Jan Beulich
  2022-03-11  8:12     ` Roger Pau Monné
  2022-03-15 15:41   ` Roger Pau Monné
  2 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2022-03-11  7:35 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: Konrad Rzeszutek Wilk, Ross Lagerwall, doebel, xen-devel

On 10.03.2022 16:08, Roger Pau Monne wrote:
> --- a/common.c
> +++ b/common.c
> @@ -249,19 +249,22 @@ int is_text_section(struct section *sec)
>  		(sec->sh.sh_flags & SHF_EXECINSTR));
>  }
>  
> +#define SEC_MATCH(n) !strncmp(sec->name, n, strlen(n) - 1)
>  int is_rodata_section(struct section *sec)
>  {
>  	return sec->sh.sh_type == SHT_PROGBITS &&
>  	       !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) &&
> -	       !strncmp(sec->name, ".rodata", 7);
> +	       (SEC_MATCH(".rodata") || SEC_MATCH(".data.ro_after_init"));
>  }
>  
>  int is_init_section(struct section *sec)
>  {
>  	return sec->sh.sh_type == SHT_PROGBITS &&
>  	       (sec->sh.sh_flags & SHF_ALLOC) &&
> -	       !strncmp(sec->name, ".init", 5);
> +	       (SEC_MATCH(".init") || SEC_MATCH(".text.startup") ||
> +	        SEC_MATCH(".altinstr") || SEC_MATCH(".ctors"));

Having dealt with this recently - what about .init_array? Modern gcc
prefers that over .ctors. Of course the question is whether either
really needs dealing with here - these sections, to my knowledge,
appear only with gcov support enabled. Not sure that's a case where
livepatching is actually expected to be used.

Jan



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

* Re: [PATCH 3/3] livepatch: correctly handle altinstruction sections
  2022-03-11  7:35   ` Jan Beulich
@ 2022-03-11  8:12     ` Roger Pau Monné
  2022-03-11  8:18       ` Jan Beulich
  0 siblings, 1 reply; 11+ messages in thread
From: Roger Pau Monné @ 2022-03-11  8:12 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Konrad Rzeszutek Wilk, Ross Lagerwall, doebel, xen-devel

On Fri, Mar 11, 2022 at 08:35:18AM +0100, Jan Beulich wrote:
> On 10.03.2022 16:08, Roger Pau Monne wrote:
> > --- a/common.c
> > +++ b/common.c
> > @@ -249,19 +249,22 @@ int is_text_section(struct section *sec)
> >  		(sec->sh.sh_flags & SHF_EXECINSTR));
> >  }
> >  
> > +#define SEC_MATCH(n) !strncmp(sec->name, n, strlen(n) - 1)
> >  int is_rodata_section(struct section *sec)
> >  {
> >  	return sec->sh.sh_type == SHT_PROGBITS &&
> >  	       !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) &&
> > -	       !strncmp(sec->name, ".rodata", 7);
> > +	       (SEC_MATCH(".rodata") || SEC_MATCH(".data.ro_after_init"));
> >  }
> >  
> >  int is_init_section(struct section *sec)
> >  {
> >  	return sec->sh.sh_type == SHT_PROGBITS &&
> >  	       (sec->sh.sh_flags & SHF_ALLOC) &&
> > -	       !strncmp(sec->name, ".init", 5);
> > +	       (SEC_MATCH(".init") || SEC_MATCH(".text.startup") ||
> > +	        SEC_MATCH(".altinstr") || SEC_MATCH(".ctors"));
> 
> Having dealt with this recently - what about .init_array? Modern gcc
> prefers that over .ctors. Of course the question is whether either
> really needs dealing with here - these sections, to my knowledge,
> appear only with gcov support enabled. Not sure that's a case where
> livepatching is actually expected to be used.

.init_array will match the .init comparison, and thus is already
handled.

Regarding .ctors, it's certainly an .init section, so it doesn't hurt
to get added here in any case? (regardless of us only knowing it being
used for code coverage so far)

Thanks, Roger.


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

* Re: [PATCH 3/3] livepatch: correctly handle altinstruction sections
  2022-03-11  8:12     ` Roger Pau Monné
@ 2022-03-11  8:18       ` Jan Beulich
  0 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2022-03-11  8:18 UTC (permalink / raw)
  To: Roger Pau Monné
  Cc: Konrad Rzeszutek Wilk, Ross Lagerwall, doebel, xen-devel

On 11.03.2022 09:12, Roger Pau Monné wrote:
> On Fri, Mar 11, 2022 at 08:35:18AM +0100, Jan Beulich wrote:
>> On 10.03.2022 16:08, Roger Pau Monne wrote:
>>> --- a/common.c
>>> +++ b/common.c
>>> @@ -249,19 +249,22 @@ int is_text_section(struct section *sec)
>>>  		(sec->sh.sh_flags & SHF_EXECINSTR));
>>>  }
>>>  
>>> +#define SEC_MATCH(n) !strncmp(sec->name, n, strlen(n) - 1)
>>>  int is_rodata_section(struct section *sec)
>>>  {
>>>  	return sec->sh.sh_type == SHT_PROGBITS &&
>>>  	       !(sec->sh.sh_flags & (SHF_EXECINSTR | SHF_WRITE)) &&
>>> -	       !strncmp(sec->name, ".rodata", 7);
>>> +	       (SEC_MATCH(".rodata") || SEC_MATCH(".data.ro_after_init"));
>>>  }
>>>  
>>>  int is_init_section(struct section *sec)
>>>  {
>>>  	return sec->sh.sh_type == SHT_PROGBITS &&
>>>  	       (sec->sh.sh_flags & SHF_ALLOC) &&
>>> -	       !strncmp(sec->name, ".init", 5);
>>> +	       (SEC_MATCH(".init") || SEC_MATCH(".text.startup") ||
>>> +	        SEC_MATCH(".altinstr") || SEC_MATCH(".ctors"));
>>
>> Having dealt with this recently - what about .init_array? Modern gcc
>> prefers that over .ctors. Of course the question is whether either
>> really needs dealing with here - these sections, to my knowledge,
>> appear only with gcov support enabled. Not sure that's a case where
>> livepatching is actually expected to be used.
> 
> .init_array will match the .init comparison, and thus is already
> handled.

Oh, I guess I should have looked at what SEC_MATCH() actually does.

> Regarding .ctors, it's certainly an .init section, so it doesn't hurt
> to get added here in any case? (regardless of us only knowing it being
> used for code coverage so far)

It certainly doesn't hurt, sure.

Jan



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

* Re: [PATCH 3/3] livepatch: correctly handle altinstruction sections
  2022-03-10 15:08 ` [PATCH 3/3] livepatch: correctly handle altinstruction sections Roger Pau Monne
  2022-03-10 17:25   ` Doebel, Bjoern
  2022-03-11  7:35   ` Jan Beulich
@ 2022-03-15 15:41   ` Roger Pau Monné
  2 siblings, 0 replies; 11+ messages in thread
From: Roger Pau Monné @ 2022-03-15 15:41 UTC (permalink / raw)
  To: xen-devel; +Cc: Konrad Rzeszutek Wilk, Ross Lagerwall, doebel

On Thu, Mar 10, 2022 at 04:08:34PM +0100, Roger Pau Monne wrote:
> The current handling of altinstructions sections by the livepatch
> tools is incorrect, as on Xen those sections are part of .init and
> thus discarded after load. Correctly handle them by just ignoring, as
> it's done with other .init related sections.

I think my logic here is wrong.

While looking at something else I realized that livepatch does support
'.altinstructions', as it needs to be able to patch the contents of
the payload that is being loaded - hence ignoring any '.altintr*'
section at patch generation is not OK.

I have to withdraw this patch, will likely repost with the other
sections that do need to be ignored.

Thanks, Roger.


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

end of thread, other threads:[~2022-03-15 15:41 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-10 15:08 [PATCH 0/3] livepatch: further fixes Roger Pau Monne
2022-03-10 15:08 ` [PATCH 1/3] livepatch: use basename to perform object file matching Roger Pau Monne
2022-03-10 17:22   ` Doebel, Bjoern
2022-03-10 15:08 ` [PATCH 2/3] livepatch: add extra efi/ objects to be ignored Roger Pau Monne
2022-03-10 17:23   ` Doebel, Bjoern
2022-03-10 15:08 ` [PATCH 3/3] livepatch: correctly handle altinstruction sections Roger Pau Monne
2022-03-10 17:25   ` Doebel, Bjoern
2022-03-11  7:35   ` Jan Beulich
2022-03-11  8:12     ` Roger Pau Monné
2022-03-11  8:18       ` Jan Beulich
2022-03-15 15:41   ` Roger Pau Monné

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.