2005-09-05 17:53:32

by Chaskiel Grundman

[permalink] [raw]
Subject: (alpha) process_reloc_for_got confuses r_offset and r_addend

arch/alpha/kernel/module.c:process_reloc_for_got(), which figures out how
big the .got section for a module should be, appears to be confusing
r_offset (the file offset that the relocation needs to be applied to) with
r_addend (the offset of the relocation's actual target address from the
address of the relocation's symbol). Because of this, one .got entry is
allocated for each relocation instead of one each unique symbol/addend.

In the module I am working with, this causes the .got section to be almost
10 times larger than it needs to be (75544 bytes instead of 7608 bytes).
As the .got is accessed with global-pointer-relative instructions, it
needs to be within the 64k gp "zone", and a 75544 byte .got clearly does
not fit. The result of this is that relocation overflows are detected
during module load and the load is aborted.

Does anyone see anything wrong with this analysis? I tested a patch that
makes the obvious change to struct got_entry/process_reloc_for_got and it
seems to work ok.

(Please cc me on replies. thanks)


2005-09-05 18:34:08

by Jesper Juhl

[permalink] [raw]
Subject: Re: (alpha) process_reloc_for_got confuses r_offset and r_addend

On 9/5/05, Chaskiel Grundman <[email protected]> wrote:
> arch/alpha/kernel/module.c:process_reloc_for_got(), which figures out how
> big the .got section for a module should be, appears to be confusing
> r_offset (the file offset that the relocation needs to be applied to) with
> r_addend (the offset of the relocation's actual target address from the
> address of the relocation's symbol). Because of this, one .got entry is
> allocated for each relocation instead of one each unique symbol/addend.
>
> In the module I am working with, this causes the .got section to be almost
> 10 times larger than it needs to be (75544 bytes instead of 7608 bytes).
> As the .got is accessed with global-pointer-relative instructions, it
> needs to be within the 64k gp "zone", and a 75544 byte .got clearly does
> not fit. The result of this is that relocation overflows are detected
> during module load and the load is aborted.
>
> Does anyone see anything wrong with this analysis? I tested a patch that
> makes the obvious change to struct got_entry/process_reloc_for_got and it
> seems to work ok.
>
> (Please cc me on replies. thanks)

Why not post the patch you made for review as well?

--
Jesper Juhl <[email protected]>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html

2005-09-05 18:42:40

by Chaskiel Grundman

[permalink] [raw]
Subject: Re: (alpha) process_reloc_for_got confuses r_offset and r_addend

On Mon, 5 Sep 2005, Jesper Juhl wrote:
> Why not post the patch you made for review as well?

In part because if the analysis is wrong, then the patch surely is.

but mostly because I didn't want to post my message with the bland subject
that the faq recommends for patches.

--- linux-2.6.12.5/arch/alpha/kernel/module.c 2005-08-14 20:20:18.000000000 -0400
+++ linux/arch/alpha/kernel/module.c 2005-09-05 12:38:43.000000000 -0400
@@ -47,7 +47,7 @@

struct got_entry {
struct got_entry *next;
- Elf64_Addr r_offset;
+ Elf64_Sxword r_addend;
int got_offset;
};

@@ -57,14 +57,14 @@
{
unsigned long r_sym = ELF64_R_SYM (rela->r_info);
unsigned long r_type = ELF64_R_TYPE (rela->r_info);
- Elf64_Addr r_offset = rela->r_offset;
+ Elf64_Sxword r_addend = rela->r_addend;
struct got_entry *g;

if (r_type != R_ALPHA_LITERAL)
return;

for (g = chains + r_sym; g ; g = g->next)
- if (g->r_offset == r_offset) {
+ if (g->r_addend == r_addend) {
if (g->got_offset == 0) {
g->got_offset = *poffset;
*poffset += 8;
@@ -74,7 +74,7 @@

g = kmalloc (sizeof (*g), GFP_KERNEL);
g->next = chains[r_sym].next;
- g->r_offset = r_offset;
+ g->r_addend = r_addend;
g->got_offset = *poffset;
*poffset += 8;
chains[r_sym].next = g;

2005-09-06 21:10:31

by Richard Henderson

[permalink] [raw]
Subject: Re: (alpha) process_reloc_for_got confuses r_offset and r_addend

This patch is correct.


r~

On Mon, Sep 05, 2005 at 02:42:36PM -0400, Chaskiel Grundman wrote:
> On Mon, 5 Sep 2005, Jesper Juhl wrote:
> > Why not post the patch you made for review as well?
>
> In part because if the analysis is wrong, then the patch surely is.
>
> but mostly because I didn't want to post my message with the bland subject
> that the faq recommends for patches.
>
> --- linux-2.6.12.5/arch/alpha/kernel/module.c 2005-08-14 20:20:18.000000000 -0400
> +++ linux/arch/alpha/kernel/module.c 2005-09-05 12:38:43.000000000 -0400
> @@ -47,7 +47,7 @@
>
> struct got_entry {
> struct got_entry *next;
> - Elf64_Addr r_offset;
> + Elf64_Sxword r_addend;
> int got_offset;
> };
>
> @@ -57,14 +57,14 @@
> {
> unsigned long r_sym = ELF64_R_SYM (rela->r_info);
> unsigned long r_type = ELF64_R_TYPE (rela->r_info);
> - Elf64_Addr r_offset = rela->r_offset;
> + Elf64_Sxword r_addend = rela->r_addend;
> struct got_entry *g;
>
> if (r_type != R_ALPHA_LITERAL)
> return;
>
> for (g = chains + r_sym; g ; g = g->next)
> - if (g->r_offset == r_offset) {
> + if (g->r_addend == r_addend) {
> if (g->got_offset == 0) {
> g->got_offset = *poffset;
> *poffset += 8;
> @@ -74,7 +74,7 @@
>
> g = kmalloc (sizeof (*g), GFP_KERNEL);
> g->next = chains[r_sym].next;
> - g->r_offset = r_offset;
> + g->r_addend = r_addend;
> g->got_offset = *poffset;
> *poffset += 8;
> chains[r_sym].next = g;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/