diff mbox

[v2,14/20] livepatch: ARM 32|64: Ignore mapping symbols: $[d, a, x, t]

Message ID 1472132255-23470-15-git-send-email-konrad.wilk@oracle.com (mailing list archive)
State New, archived
Headers show

Commit Message

Konrad Rzeszutek Wilk Aug. 25, 2016, 1:37 p.m. UTC
Those symbols are used to help final linkers to replace insn.
The ARM ELF specification mandates that they are present
to denote the start of certain CPU features. There are two
variants of it - short and long format.

Either way - we can ignore these symbols.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v1: First submission
v2: Update the order of symbols, fix title
    Add {} in after the first if - per Jan's recommendation.
---
 xen/arch/arm/livepatch.c    | 32 ++++++++++++++++++++++++++++++++
 xen/arch/x86/livepatch.c    |  7 +++++++
 xen/common/livepatch.c      |  2 +-
 xen/include/xen/livepatch.h |  2 ++
 4 files changed, 42 insertions(+), 1 deletion(-)

Comments

Andrew Cooper Aug. 25, 2016, 2:03 p.m. UTC | #1
On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> Those symbols are used to help final linkers to replace insn.
> The ARM ELF specification mandates that they are present
> to denote the start of certain CPU features. There are two
> variants of it - short and long format.
>
> Either way - we can ignore these symbols.
>
> Signed-off-by: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>

x86 bits Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Julien Grall Sept. 1, 2016, 2:48 p.m. UTC | #2
Hi Konrad,

On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
> Those symbols are used to help final linkers to replace insn.
> The ARM ELF specification mandates that they are present
> to denote the start of certain CPU features. There are two
> variants of it - short and long format.
>
> Either way - we can ignore these symbols.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>
> v1: First submission
> v2: Update the order of symbols, fix title
>     Add {} in after the first if - per Jan's recommendation.
> ---
>  xen/arch/arm/livepatch.c    | 32 ++++++++++++++++++++++++++++++++
>  xen/arch/x86/livepatch.c    |  7 +++++++
>  xen/common/livepatch.c      |  2 +-
>  xen/include/xen/livepatch.h |  2 ++
>  4 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index f49e347..c290602 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -82,6 +82,38 @@ void arch_livepatch_unmask(void)
>      local_abort_enable();
>  }
>
> +int arch_is_payload_symbol(const struct livepatch_elf *elf,
> +                           const struct livepatch_elf_sym *sym)

I think this function should return bool (or bool_t) as the return will 
be used by is_payload_symbol as bool.

> +{
> +    /*
> +     * - Mapping symbols - denote the "start of a sequence of bytes of the
> +     *   appropiate type" to mark certain features - such as start of region

s/appropiate/appropriate/

> +     *   containing data ($d); ARM ($a), A64 ($x), or Thumb instructions ($t).
> +     *
> +     * The format is either short: '$x' or long: '$x.<any>'. We do not
> +     * need this and more importantly - each payload will contain this
> +     * resulting in symbol collisions.
> +     */
> +    if ( *sym->name == '$' && sym->name[1] != '\0' )
> +    {
> +        char p = sym->name[1];
> +        size_t len = strlen(sym->name);
> +
> +        if ( (len >= 3 && ( sym->name[2] == '.' )) || (len == 2) )
> +        {
> +            if ( p == 'd' ||
> +#ifdef CONFIG_ARM_32
> +                 p == 'a' || p == 't'

Note that Xen is not using Thumb instructions which have variable 
length, so we shouldn't expect to see $t. symbols.

> +#else
> +                 p == 'x'
> +#endif
> +               )
> +                return 0;

Please use false here.

> +        }
> +    }
> +    return 1;

Please use true here.

> +}
> +

Regards,
diff mbox

Patch

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index f49e347..c290602 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -82,6 +82,38 @@  void arch_livepatch_unmask(void)
     local_abort_enable();
 }
 
+int arch_is_payload_symbol(const struct livepatch_elf *elf,
+                           const struct livepatch_elf_sym *sym)
+{
+    /*
+     * - Mapping symbols - denote the "start of a sequence of bytes of the
+     *   appropiate type" to mark certain features - such as start of region
+     *   containing data ($d); ARM ($a), A64 ($x), or Thumb instructions ($t).
+     *
+     * The format is either short: '$x' or long: '$x.<any>'. We do not
+     * need this and more importantly - each payload will contain this
+     * resulting in symbol collisions.
+     */
+    if ( *sym->name == '$' && sym->name[1] != '\0' )
+    {
+        char p = sym->name[1];
+        size_t len = strlen(sym->name);
+
+        if ( (len >= 3 && ( sym->name[2] == '.' )) || (len == 2) )
+        {
+            if ( p == 'd' ||
+#ifdef CONFIG_ARM_32
+                 p == 'a' || p == 't'
+#else
+                 p == 'x'
+#endif
+               )
+                return 0;
+        }
+    }
+    return 1;
+}
+
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
                                const struct livepatch_elf_sec *rela)
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 5491c09..f45eccf 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -130,6 +130,13 @@  int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
     return 0;
 }
 
+int arch_is_payload_symbol(const struct livepatch_elf *elf,
+                           const struct livepatch_elf_sym *sym)
+{
+    /* No special checks on x86. */
+    return 1;
+}
+
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
                                const struct livepatch_elf_sec *rela)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 7d21326..d5f069e 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -779,7 +779,7 @@  static bool_t is_payload_symbol(const struct livepatch_elf *elf,
          !strncmp(sym->name, ".L", 2) )
         return 0;
 
-    return 1;
+    return arch_is_payload_symbol(elf, sym);
 }
 
 static int build_symbol_table(struct payload *payload,
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 6f30c0d..d94155f 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -46,6 +46,8 @@  bool_t is_patch(const void *addr);
 /* Arch hooks. */
 int arch_verify_insn_length(unsigned long len);
 int arch_livepatch_verify_elf(const struct livepatch_elf *elf);
+int arch_is_payload_symbol(const struct livepatch_elf *elf,
+                           const struct livepatch_elf_sym *sym);
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
                                const struct livepatch_elf_sec *rela);