[PATCH] Fix ld.so regression.

[Carlos O'Donell]

On Tue, Apr 10, 2012 at 10:34 AM, David Miller <davem@davemloft.net> wrote:
> The _ELF_DYNAMIC_DO_RELOC simplifications went a bit too far.
> The one extra case we have to handle is when there is a GAP
> between the rel and the pltrel.
>
> The case where this can happen is extremely unfortunate, go
> look at:
>
>        https://wiki.mozilla.org/Elfhack
>
> but please not if you've eaten recently.

Interesting. My opinion is that this is a reflection of how the
machines have chosen to implement relocations. Nothing says you can't
have one relocation process a whole batch of changes. In fact I
remember Power has a single relocation for a large array of OPDs that
need fixing up. Instead of this "crazy hacking" you could add a new
reloction that acts on a sorted batch of offsets.

> Anyways the following patch will fix it.
>
> I didn't plan on updating NEWS with the bug number, since this
> is fixing a bug introduced during development and not present
> in previous releases.  Is that the right way to handle this?

No.

The consensus was that a simple policy is policy that gets followed.

For any bug fixed you put the BZ # into NEWS.

http://sourceware.org/ml/libc-alpha/2012-03/msg00299.html
http://sourceware.org/ml/libc-alpha/2012-02/msg00344.html

> Otherwise, ok to commit?
>
> 2012-04-10  David S. Miller  <davem@davemloft.net>
>
>        [BZ #13967]
>        * elf/dynamic-link.h (_ELF_DYNAMIC_DO_RELOC): Handle the case
>        where there is a gap between DT_REL(A) and DT_JMPREL.
>
> diff --git a/elf/dynamic-link.h b/elf/dynamic-link.h
> index ef01c61..dc0a909 100644
> --- a/elf/dynamic-link.h
> +++ b/elf/dynamic-link.h
> @@ -252,9 +252,10 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp)
>  /* On some machines, notably SPARC, DT_REL* includes DT_JMPREL in its
>    range.  Note that according to the ELF spec, this is completely legal!
>
> -   We are guarenteed that we have one of two situations.  Either DT_JMPREL
> +   We are guarenteed that we have one of three situations.  Either DT_JMPREL
>    comes immediately after DT_REL*, or there is overlap and DT_JMPREL
> -   consumes precisely the very end of the DT_REL*.  */
> +   consumes precisely the very end of the DT_REL*, or DT_JMPREL and DT_REL*
> +   are completely separate and there is a gap between them.  */
>
>  # define _ELF_DYNAMIC_DO_RELOC(RELOC, reloc, map, do_lazy, skip_ifunc, test_rel) \
>   do {                                                                       \
> @@ -275,19 +276,20 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp)
>        && (!test_rel || (map)->l_info[DT_PLTREL]->d_un.d_val == DT_##RELOC)) \
>       {                                                                              \
>        ElfW(Addr) start = D_PTR ((map), l_info[DT_JMPREL]);                  \
> +       ElfW(Addr) size = (map)->l_info[DT_PLTRELSZ]->d_un.d_val;             \
>                                                                              \
> -       if (__builtin_expect (ranges[0].size, 1))                             \
> +       if (ranges[0].start + ranges[0].size == (start + size))               \
>          ranges[0].size = (start - ranges[0].start);                         \

Now that we have a `size', could you rewrite this to be less instructions?

e.g. ranges[0].size -= size;

>        if (! ELF_DURING_STARTUP && ((do_lazy) || ranges[0].size == 0))       \
>          {                                                                   \
>            ranges[1].start = start;                                          \
> -           ranges[1].size = (map)->l_info[DT_PLTRELSZ]->d_un.d_val;          \
> +           ranges[1].size = size;                                            \
>            ranges[1].lazy = (do_lazy);                                       \
>          }                                                                   \
>        else                                                                  \
>          {                                                                   \
>            /* Combine processing the sections.  */                           \
> -           ranges[0].size += (map)->l_info[DT_PLTRELSZ]->d_un.d_val;         \
> +           ranges[0].size += size;                                           \
>          }                                                                   \
>       }                                                                              \
>                                                                              \

OK as is, and OK with the minor optimization.

Cheers,
Carlos.