Re: [Xen-devel] [PATCH v1 7/9] livepatch: ARM64: Ignore mapping symbols: $[a, d, x, p]

[Konrad Rzeszutek Wilk]

On Wed, Aug 17, 2016 at 06:21:03AM -0600, Jan Beulich wrote:
> >>> On 15.08.16 at 01:07,  wrote:
> 
> According to the code you mean $t instead of $p in the subject.

Yes! Thanks for noticing that.
> 
> > --- a/xen/arch/arm/livepatch.c
> > +++ b/xen/arch/arm/livepatch.c
> > @@ -90,6 +90,35 @@ 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 A64 ($x), ARM ($a), or Thumb instructions ($t); or 
> > data ($d)
> > + *
> > + * The format is either short: '$x' or long: '$x.'. 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' ||
> 
> May I suggest not nesting two if()-s like this?
> 
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -780,7 +780,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);
> >  }
> 
> Taking into consideration what's still visible as context here - are .L
> prefixed symbols really architecture independent? If not, checking

They are architecture independent and even the ARM ELF document mentions
it:
"Note: Multiple conventions exist for the names of compiler temporary symbols
(for example, ARMCC uses Lxxx.yyy, while GNU uses .Lxxx)."

(pg 23)

> for them should probably be moved.
> 
> Jan
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1 7/9] livepatch: ARM64: Ignore mapping symbols: $[a, d, x, p]

[Jan Beulich]

>>> On 15.08.16 at 01:07,  wrote:

According to the code you mean $t instead of $p in the subject.

> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -90,6 +90,35 @@ 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 A64 ($x), ARM ($a), or Thumb instructions ($t); or data 
> ($d)
> + *
> + * The format is either short: '$x' or long: '$x.'. 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' ||

May I suggest not nesting two if()-s like this?

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -780,7 +780,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);
>  }

Taking into consideration what's still visible as context here - are .L
prefixed symbols really architecture independent? If not, checking
for them should probably be moved.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v1 7/9] livepatch: ARM64: Ignore mapping symbols: $[a, d, x, p]

[Konrad Rzeszutek Wilk]

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 

---
Cc: Konrad Rzeszutek Wilk 
Cc: Ross Lagerwall 
Cc: Stefano Stabellini 
Cc: Julien Grall 
Cc: Andrew Cooper 

v1: First submission
---
 xen/arch/arm/livepatch.c| 29 +
 xen/arch/x86/livepatch.c|  7 +++
 xen/common/livepatch.c  |  2 +-
 xen/include/xen/livepatch.h |  2 ++
 4 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 19f6033..833b06b 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -90,6 +90,35 @@ 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 A64 ($x), ARM ($a), or Thumb instructions ($t); or data 
($d)
+ *
+ * The format is either short: '$x' or long: '$x.'. 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 e3f3f37..f3c6f46 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -116,6 +116,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 af9443d..94bfb07 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -780,7 +780,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);
-- 
2.4.11


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel