Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Jun 13, 2017 at 09:51:35PM +0100, Andrew Cooper wrote:
>  * Reduce symbol scope and initalisation as much as possible
>  * Annotate a fallthrough case in arm64
>  * Fix switch statement style in arm32
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> The purpose of this patch is simply to make the following patch easier to
> review.
> ---
>  xen/arch/arm/arm32/livepatch.c | 27 ++++++++++++---------------
>  xen/arch/arm/arm64/livepatch.c | 19 +++++++------------
>  xen/arch/x86/livepatch.c       | 13 +++++--------
>  3 files changed, 24 insertions(+), 35 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> index a7fd5e2..a328179 100644
> --- a/xen/arch/arm/arm32/livepatch.c
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -224,21 +224,21 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>                             const struct livepatch_elf_sec *rela,
>                             bool use_rela)
>  {
> -    const Elf_RelA *r_a;
> -    const Elf_Rel *r;
> -    unsigned int symndx, i;
> -    uint32_t val;
> -    void *dest;
> +    unsigned int i;
>      int rc = 0;
>  
>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>      {
> +        unsigned int symndx;
> +        uint32_t val;
> +        void *dest;
>          unsigned char type;
> -        s32 addend = 0;
> +        s32 addend;
>  
>          if ( use_rela )
>          {
> -            r_a = rela->data + i * rela->sec->sh_entsize;
> +            const Elf_RelA *r_a = rela->data + i * rela->sec->sh_entsize;
> +
>              symndx = ELF32_R_SYM(r_a->r_info);
>              type = ELF32_R_TYPE(r_a->r_info);
>              dest = base->load_addr + r_a->r_offset; /* P */
> @@ -246,10 +246,12 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>          }
>          else
>          {
> -            r = rela->data + i * rela->sec->sh_entsize;
> +            const Elf_Rel *r = rela->data + i * rela->sec->sh_entsize;
> +
>              symndx = ELF32_R_SYM(r->r_info);
>              type = ELF32_R_TYPE(r->r_info);
>              dest = base->load_addr + r->r_offset; /* P */
> +            addend = get_addend(type, dest);
>          }
>  
>          if ( symndx > elf->nsym )
> @@ -259,13 +261,11 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>              return -EINVAL;
>          }
>  
> -        if ( !use_rela )
> -            addend = get_addend(type, dest);

This was added right after the symndx > elf->nsym check as
way to make sure we won't dereference the dest (b/c the symbol
may be outside the bounds).


> -
>          val = elf->sym[symndx].sym->st_value; /* S */
>  
>          rc = perform_rel(type, dest, val, addend);
> -        switch ( rc ) {
> +        switch ( rc )
> +        {
>          case -EOVERFLOW:
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n",
>                      elf->name, i, rela->name, base->name);
> @@ -275,9 +275,6 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation #%x\n",
>                      elf->name, type);
>              break;
> -
> -        default:
> -            break;
>          }
>  
>          if ( rc )
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index dae64f5..63929b1 100644
> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -241,19 +241,16 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>                                  const struct livepatch_elf_sec *base,
>                                  const struct livepatch_elf_sec *rela)
>  {
> -    const Elf_RelA *r;
> -    unsigned int symndx, i;
> -    uint64_t val;
> -    void *dest;
> -    bool_t overflow_check;
> +    unsigned int i;
>  
>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>      {
> +        const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
> +        unsigned int symndx = ELF64_R_SYM(r->r_info);
> +        void *dest = base->load_addr + r->r_offset; /* P */
> +        bool overflow_check = true;
>          int ovf = 0;
> -
> -        r = rela->data + i * rela->sec->sh_entsize;
> -
> -        symndx = ELF64_R_SYM(r->r_info);
> +        uint64_t val;
>  
>          if ( symndx > elf->nsym )
>          {
> @@ -262,11 +259,8 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>              return -EINVAL;
>          }
>  
> -        dest = base->load_addr + r->r_offset; /* P */
>          val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
>  
> -        overflow_check = true;
> -
>          /* ARM64 operations at minimum are always 32-bit. */
>          if ( r->r_offset >= base->sec->sh_size ||
>              (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )
> @@ -403,6 +397,7 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>  
>          case R_AARCH64_ADR_PREL_PG_HI21_NC:
>              overflow_check = false;
> +            /* Fallthrough. */
>          case R_AARCH64_ADR_PREL_PG_HI21:
>              ovf = reloc_insn_imm(RELOC_OP_PAGE, dest, val, 12, 21,
>                                   AARCH64_INSN_IMM_ADR);

OK, that part is fine.
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index dd50dd1..7917610 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -161,16 +161,14 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>                                  const struct livepatch_elf_sec *base,
>                                  const struct livepatch_elf_sec *rela)
>  {
> -    const Elf_RelA *r;
> -    unsigned int symndx, i;
> -    uint64_t val;
> -    uint8_t *dest;
> +    unsigned int i;
>  
>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>      {
> -        r = rela->data + i * rela->sec->sh_entsize;
> -
> -        symndx = ELF64_R_SYM(r->r_info);
> +        const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
> +        unsigned int symndx = ELF64_R_SYM(r->r_info);
> +        uint8_t *dest = base->load_addr + r->r_offset;
> +        uint64_t val;
>  
>          if ( symndx > elf->nsym )
>          {
> @@ -179,7 +177,6 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>              return -EINVAL;
>          }
>  
> -        dest = base->load_addr + r->r_offset;
>          val = r->r_addend + elf->sym[symndx].sym->st_value;
>  
>          switch ( ELF64_R_TYPE(r->r_info) )

And this is fine too.

> -- 
> 2.1.4
> 

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