LinuxLists.cc - [PATCH 01/02] Elf: Always define elf_addr

[permalink] [raw]

Subject: [PATCH 02/02] Elf: Align elf notes properly

elf: Align elf notes properly

The kernel currently contains several elf note aligment implementations. Most
implementations follow the spec on 32-bit platforms, but none current aligns
the notes correctly on 64-bit platforms. This patch tries to fix this by
interpreting the 64-bit and 32-bit elf specs as the following:

offset bytes name
0 4 n_namesz -+ -+
4 4 n_descsz | elf note header |
8 4 n_type -+ | elf note entry size - N4
12 N1 name |
N2 N3 desc -+

WS = word size in bytes (4 for 32 bit, 8 for 64 bit)
N1 = roundup(n_namesz + sizeof(elf note header), WS) - sizeof(elf note header)
N2 = sizeof(elf note header) + N1
N3 = roundup(n_descsz, WS)
N4 = sizeof(elf note header) + N1 + N2

The elf note header contains three 32-bit values on 32-bit and 64-bit systems.
The header is followed by name and desc data together with padding. The
alignment and padding varies depending on the word size.

I base my interpretation on the following:

TIS Elf spec v1.2
http://refspecs.freestandards.org/elf/elf.pdf

- Each elf note entry should be an array of 4-byte words. (Page 42)
- Name should be terminated with a '\0' (Page 42)
- Desc and the next note should be aligned to 32 bits. (Page 42)
- The example shows that the '\0' is included in n_namesz. (Page 43)

ELF-64 Object File Format, Version 1.5 Draft 2
http://www.busybox.net/cgi-bin/viewcvs.cgi/trunk/docs/elf-64-gen.pdf?rev=15356

- Each elf note entry should be an array of 8-byte words. (Page 13)
- Name contains a NULL-terminated string. (Page 13)
- Desc and the next note should be aligned to 64 bits. (Page 14)
- "The (name) length does not include the terminating null or the padding"
(Page 14)

The last line is kind of interesting since it conflicts with the 32-bit
example. I'm ignoring that and treating n_namesz the same regardless of
word size. So n_namesz is used in the same fashion for both 32-bit and
64-bit platforms in this patch and the calculations above.

This patch fixes the following:

- i386: 32-bit crash notes aligns elf note header to 32-bit size
This is basically a no-op because 12 is already aligned to 4.
Use roundup() and sizeof(elf_addr_t).

- mips: The terminating NULL was not included in n_namesz.
Use roundup(sizeof(elf note header) + n_namesz, sizeof(elf_addr_t)).
Use sizeof(elf_addr_t).

- powerpc: 32-bit aligment is used regardless of word size.
Use roundup(sizeof(elf note header) + n_namesz, sizeof(elf_addr_t)).
Use sizeof(elf_addr_t).

- x86_64: 32-bit aligment is used regardless of word size.
Use roundup(sizeof(elf note header) + n_namesz, sizeof(elf_addr_t)).
Use sizeof(elf_addr_t).

- binfmt_elf / binfmt_elf_fdpic: 32-bit aligment is always used.
Use roundup(sizeof(elf note header) + n_namesz, sizeof(elf_addr_t)).
Use sizeof(elf_addr_t).

- kcore: 32-bit aligment is always used, elf_buflen calculation seems wrong.
Use roundup(sizeof(elf note header) + n_namesz, sizeof(elf_addr_t)).
Use sizeof(elf_addr_t).

- vmcore: 32-bit aligment is used for both 32-bit and 64-bit code.
Use roundup(sizeof(elf note header) + n_namesz, sizeof(elf_addr_t)).
Use sizeof(elf_addr_t).

Signed-off-by: Magnus Damm <[email protected]>
---

Applies to 2.6.19-rc4.

arch/i386/kernel/crash.c | 16 ++++++++--------
arch/mips/kernel/irixelf.c | 12 ++++++------
arch/powerpc/kernel/crash.c | 16 ++++++++--------
arch/x86_64/kernel/crash.c | 18 +++++++++---------
fs/binfmt_elf.c | 11 ++++++-----
fs/binfmt_elf_fdpic.c | 10 +++++-----
fs/proc/kcore.c | 26 ++++++++++++++------------
fs/proc/vmcore.c | 11 +++++------
8 files changed, 61 insertions(+), 59 deletions(-)

--- 0002/arch/i386/kernel/crash.c
+++ work/arch/i386/kernel/crash.c 2006-11-02 16:59:52.000000000 +0900
@@ -31,7 +31,8 @@
/* This keeps a track of which one is crashing cpu. */
static int crashing_cpu;

-static u32 *append_elf_note(u32 *buf, char *name, unsigned type, void *data,
+static unsigned char *
+append_elf_note(unsigned char *buf, char *name, unsigned type, void *data,
size_t data_len)
{
struct elf_note note;
@@ -40,16 +41,15 @@ static u32 *append_elf_note(u32 *buf, ch
note.n_descsz = data_len;
note.n_type = type;
memcpy(buf, &note, sizeof(note));
- buf += (sizeof(note) +3)/4;
- memcpy(buf, name, note.n_namesz);
- buf += (note.n_namesz + 3)/4;
+ memcpy(buf + sizeof(note), name, note.n_namesz);
+ buf += roundup(sizeof(note) + note.n_namesz, sizeof(elf_addr_t));
memcpy(buf, data, note.n_descsz);
- buf += (note.n_descsz + 3)/4;
+ buf += roundup(note.n_descsz, sizeof(elf_addr_t));

return buf;
}

-static void final_note(u32 *buf)
+static void final_note(unsigned char *buf)
{
struct elf_note note;

@@ -62,7 +62,7 @@ static void final_note(u32 *buf)
static void crash_save_this_cpu(struct pt_regs *regs, int cpu)
{
struct elf_prstatus prstatus;
- u32 *buf;
+ unsigned char *buf;

if ((cpu < 0) || (cpu >= NR_CPUS))
return;
@@ -74,7 +74,7 @@ static void crash_save_this_cpu(struct p
* squirrelled away. ELF notes happen to provide
* all of that, so there is no need to invent something new.
*/
- buf = (u32*)per_cpu_ptr(crash_notes, cpu);
+ buf = (unsigned char *)per_cpu_ptr(crash_notes, cpu);
if (!buf)
return;
memset(&prstatus, 0, sizeof(prstatus));
--- 0003/arch/mips/kernel/irixelf.c
+++ work/arch/mips/kernel/irixelf.c 2006-11-02 16:59:52.000000000 +0900
@@ -1008,9 +1008,9 @@ static int notesize(struct memelfnote *e
{
int sz;

- sz = sizeof(struct elf_note);
- sz += roundup(strlen(en->name), 4);
- sz += roundup(en->datasz, 4);
+ sz = roundup(sizeof(struct elf_note) + strlen(en->name) + 1,
+ sizeof(elf_addr_t));
+ sz += roundup(en->datasz, sizeof(elf_addr_t));

return sz;
}
@@ -1028,16 +1028,16 @@ static int writenote(struct memelfnote *
{
struct elf_note en;

- en.n_namesz = strlen(men->name);
+ en.n_namesz = strlen(men->name) + 1;
en.n_descsz = men->datasz;
en.n_type = men->type;

DUMP_WRITE(&en, sizeof(en));
DUMP_WRITE(men->name, en.n_namesz);
/* XXX - cast from long long to long to avoid need for libgcc.a */
- DUMP_SEEK(roundup((unsigned long)file->f_pos, 4)); /* XXX */
+ DUMP_SEEK(roundup((unsigned long)file->f_pos, sizeof(elf_addr_t)));
DUMP_WRITE(men->data, men->datasz);
- DUMP_SEEK(roundup((unsigned long)file->f_pos, 4)); /* XXX */
+ DUMP_SEEK(roundup((unsigned long)file->f_pos, sizeof(elf_addr_t)));

return 1;

--- 0001/arch/powerpc/kernel/crash.c
+++ work/arch/powerpc/kernel/crash.c 2006-11-02 16:59:52.000000000 +0900
@@ -46,7 +46,8 @@ int crashing_cpu = -1;
static cpumask_t cpus_in_crash = CPU_MASK_NONE;
cpumask_t cpus_in_sr = CPU_MASK_NONE;

-static u32 *append_elf_note(u32 *buf, char *name, unsigned type, void *data,
+static unsigned char *
+append_elf_note(unsigned char *buf, char *name, unsigned type, void *data,
size_t data_len)
{
struct elf_note note;
@@ -55,16 +56,15 @@ static u32 *append_elf_note(u32 *buf, ch
note.n_descsz = data_len;
note.n_type = type;
memcpy(buf, &note, sizeof(note));
- buf += (sizeof(note) +3)/4;
- memcpy(buf, name, note.n_namesz);
- buf += (note.n_namesz + 3)/4;
+ memcpy(buf + sizeof(note), name, note.n_namesz);
+ buf += roundup(sizeof(note) + note.n_namesz, sizeof(elf_addr_t));
memcpy(buf, data, note.n_descsz);
- buf += (note.n_descsz + 3)/4;
+ buf += roundup(note.n_descsz, sizeof(elf_addr_t));

return buf;
}

-static void final_note(u32 *buf)
+static void final_note(unsigned char *buf)
{
struct elf_note note;

@@ -77,7 +77,7 @@ static void final_note(u32 *buf)
static void crash_save_this_cpu(struct pt_regs *regs, int cpu)
{
struct elf_prstatus prstatus;
- u32 *buf;
+ unsigned char *buf;

if ((cpu < 0) || (cpu >= NR_CPUS))
return;
@@ -89,7 +89,7 @@ static void crash_save_this_cpu(struct p
* squirrelled away. ELF notes happen to provide
* all of that that no need to invent something new.
*/
- buf = (u32*)per_cpu_ptr(crash_notes, cpu);
+ buf = (unsigned char *)per_cpu_ptr(crash_notes, cpu);
if (!buf)
return;

--- 0002/arch/x86_64/kernel/crash.c
+++ work/arch/x86_64/kernel/crash.c 2006-11-02 16:59:52.000000000 +0900
@@ -28,8 +28,9 @@
/* This keeps a track of which one is crashing cpu. */
static int crashing_cpu;

-static u32 *append_elf_note(u32 *buf, char *name, unsigned type,
- void *data, size_t data_len)
+static unsigned char *
+append_elf_note(unsigned char *buf, char *name, unsigned type, void *data,
+ size_t data_len)
{
struct elf_note note;

@@ -37,16 +38,15 @@ static u32 *append_elf_note(u32 *buf, ch
note.n_descsz = data_len;
note.n_type = type;
memcpy(buf, &note, sizeof(note));
- buf += (sizeof(note) +3)/4;
- memcpy(buf, name, note.n_namesz);
- buf += (note.n_namesz + 3)/4;
+ memcpy(buf + sizeof(note), name, note.n_namesz);
+ buf += roundup(sizeof(note) + note.n_namesz, sizeof(elf_addr_t));
memcpy(buf, data, note.n_descsz);
- buf += (note.n_descsz + 3)/4;
+ buf += roundup(note.n_descsz, sizeof(elf_addr_t));

return buf;
}

-static void final_note(u32 *buf)
+static void final_note(unsigned char *buf)
{
struct elf_note note;

@@ -59,7 +59,7 @@ static void final_note(u32 *buf)
static void crash_save_this_cpu(struct pt_regs *regs, int cpu)
{
struct elf_prstatus prstatus;
- u32 *buf;
+ unsigned char *buf;

if ((cpu < 0) || (cpu >= NR_CPUS))
return;
@@ -72,7 +72,7 @@ static void crash_save_this_cpu(struct p
* all of that, no need to invent something new.
*/

- buf = (u32*)per_cpu_ptr(crash_notes, cpu);
+ buf = (unsigned char *)per_cpu_ptr(crash_notes, cpu);

if (!buf)
return;
--- 0003/fs/binfmt_elf.c
+++ work/fs/binfmt_elf.c 2006-11-02 16:59:52.000000000 +0900
@@ -1204,9 +1204,9 @@ static int notesize(struct memelfnote *e
{
int sz;

- sz = sizeof(struct elf_note);
- sz += roundup(strlen(en->name) + 1, 4);
- sz += roundup(en->datasz, 4);
+ sz = roundup(sizeof(struct elf_note) + strlen(en->name) + 1,
+ sizeof(elf_addr_t));
+ sz += roundup(en->datasz, sizeof(elf_addr_t));

return sz;
}
@@ -1216,8 +1216,9 @@ static int notesize(struct memelfnote *e

static int alignfile(struct file *file, loff_t *foffset)
{
- static const char buf[4] = { 0, };
- DUMP_WRITE(buf, roundup(*foffset, 4) - *foffset, foffset);
+ static const char buf[sizeof(elf_addr_t)] = { 0, };
+ DUMP_WRITE(buf, roundup(*foffset,
+ sizeof(elf_addr_t)) - *foffset, foffset);
return 1;
}

--- 0003/fs/binfmt_elf_fdpic.c
+++ work/fs/binfmt_elf_fdpic.c 2006-11-02 16:59:52.000000000 +0900
@@ -1220,9 +1220,9 @@ static int notesize(struct memelfnote *e
{
int sz;

- sz = sizeof(struct elf_note);
- sz += roundup(strlen(en->name) + 1, 4);
- sz += roundup(en->datasz, 4);
+ sz = roundup(sizeof(struct elf_note) + strlen(en->name) + 1,
+ sizeof(elf_addr_t));
+ sz += roundup(en->datasz, sizeof(elf_addr_t));

return sz;
}
@@ -1245,9 +1245,9 @@ static int writenote(struct memelfnote *
DUMP_WRITE(&en, sizeof(en));
DUMP_WRITE(men->name, en.n_namesz);
/* XXX - cast from long long to long to avoid need for libgcc.a */
- DUMP_SEEK(roundup((unsigned long)file->f_pos, 4)); /* XXX */
+ DUMP_SEEK(roundup((unsigned long)file->f_pos, sizeof(elf_addr_t)));
DUMP_WRITE(men->data, men->datasz);
- DUMP_SEEK(roundup((unsigned long)file->f_pos, 4)); /* XXX */
+ DUMP_SEEK(roundup((unsigned long)file->f_pos, sizeof(elf_addr_t)));

return 1;
}
--- 0002/fs/proc/kcore.c
+++ work/fs/proc/kcore.c 2006-11-02 16:59:52.000000000 +0900
@@ -22,6 +22,7 @@
#include <asm/uaccess.h>
#include <asm/io.h>

+#define CORE_STR "CORE"

static int open_kcore(struct inode * inode, struct file * filp)
{
@@ -82,10 +83,11 @@ static size_t get_kcore_size(int *nphdr,
}
*elf_buflen = sizeof(struct elfhdr) +
(*nphdr + 2)*sizeof(struct elf_phdr) +
- 3 * (sizeof(struct elf_note) + 4) +
- sizeof(struct elf_prstatus) +
- sizeof(struct elf_prpsinfo) +
- sizeof(struct task_struct);
+ 3 * roundup(sizeof(struct elf_note) + strlen(CORE_STR) + 1,
+ sizeof(elf_addr_t)) +
+ roundup(sizeof(struct elf_prstatus), sizeof(elf_addr_t)) +
+ roundup(sizeof(struct elf_prpsinfo), sizeof(elf_addr_t)) +
+ roundup(sizeof(struct task_struct), sizeof(elf_addr_t));
*elf_buflen = PAGE_ALIGN(*elf_buflen);
return size + *elf_buflen;
}
@@ -99,9 +101,9 @@ static int notesize(struct memelfnote *e
{
int sz;

- sz = sizeof(struct elf_note);
- sz += roundup((strlen(en->name) + 1), 4);
- sz += roundup(en->datasz, 4);
+ sz = roundup(sizeof(struct elf_note) + strlen(en->name) + 1,
+ sizeof(elf_addr_t));
+ sz += roundup(en->datasz, sizeof(elf_addr_t));

return sz;
} /* end notesize() */
@@ -124,9 +126,9 @@ static char *storenote(struct memelfnote
DUMP_WRITE(men->name, en.n_namesz);

/* XXX - cast from long long to long to avoid need for libgcc.a */
- bufp = (char*) roundup((unsigned long)bufp,4);
+ bufp = (char*) roundup((unsigned long)bufp, sizeof(elf_addr_t));
DUMP_WRITE(men->data, men->datasz);
- bufp = (char*) roundup((unsigned long)bufp,4);
+ bufp = (char*) roundup((unsigned long)bufp, sizeof(elf_addr_t));

#undef DUMP_WRITE

@@ -210,7 +212,7 @@ static void elf_kcore_store_hdr(char *bu
nhdr->p_offset = offset;

/* set up the process status */
- notes[0].name = "CORE";
+ notes[0].name = CORE_STR;
notes[0].type = NT_PRSTATUS;
notes[0].datasz = sizeof(struct elf_prstatus);
notes[0].data = &prstatus;
@@ -221,7 +223,7 @@ static void elf_kcore_store_hdr(char *bu
bufp = storenote(&notes[0], bufp);

/* set up the process info */
- notes[1].name = "CORE";
+ notes[1].name = CORE_STR;
notes[1].type = NT_PRPSINFO;
notes[1].datasz = sizeof(struct elf_prpsinfo);
notes[1].data = &prpsinfo;
@@ -238,7 +240,7 @@ static void elf_kcore_store_hdr(char *bu
bufp = storenote(&notes[1], bufp);

/* set up the task structure */
- notes[2].name = "CORE";
+ notes[2].name = CORE_STR;
notes[2].type = NT_TASKSTRUCT;
notes[2].datasz = sizeof(struct task_struct);
notes[2].data = current;
--- 0001/fs/proc/vmcore.c
+++ work/fs/proc/vmcore.c 2006-11-02 17:01:12.000000000 +0900
@@ -7,6 +7,7 @@
*
*/

+#include <linux/kernel.h>
#include <linux/mm.h>
#include <linux/proc_fs.h>
#include <linux/user.h>
@@ -255,9 +256,8 @@ static int __init merge_note_headers_elf
for (j = 0; j < max_sz; j += sz) {
if (nhdr_ptr->n_namesz == 0)
break;
- sz = sizeof(Elf64_Nhdr) +
- ((nhdr_ptr->n_namesz + 3) & ~3) +
- ((nhdr_ptr->n_descsz + 3) & ~3);
+ sz = roundup(sizeof(Elf64_Nhdr) + nhdr_ptr->n_namesz,
+ 8) + roundup(nhdr_ptr->n_descsz, 8);
real_sz += sz;
nhdr_ptr = (Elf64_Nhdr*)((char*)nhdr_ptr + sz);
}
@@ -336,9 +336,8 @@ static int __init merge_note_headers_elf
for (j = 0; j < max_sz; j += sz) {
if (nhdr_ptr->n_namesz == 0)
break;
- sz = sizeof(Elf32_Nhdr) +
- ((nhdr_ptr->n_namesz + 3) & ~3) +
- ((nhdr_ptr->n_descsz + 3) & ~3);
+ sz = roundup(sizeof(Elf32_Nhdr) + nhdr_ptr->n_namesz,
+ 4) + roundup(nhdr_ptr->n_descsz, 4);
real_sz += sz;
nhdr_ptr = (Elf32_Nhdr*)((char*)nhdr_ptr + sz);
}

2006-11-02 10:44:36

by Jakub Jelinek

[permalink] [raw]

Subject: Re: [PATCH 01/02] Elf: Always define elf_addr_t in linux/elf.h

On Thu, Nov 02, 2006 at 07:19:42PM +0900, Magnus Damm wrote:
> --- 0001/include/linux/elf.h
> +++ work/include/linux/elf.h 2006-11-02 15:44:10.000000000 +0900
> @@ -352,12 +352,16 @@ typedef struct elf64_note {
> Elf64_Word n_type; /* Content type */
> } Elf64_Nhdr;
>
> +typedef Elf64_Off elf64_addr;
> +typedef Elf32_Off elf32_addr;
> +

What are these typedefs useful for? Isn't it better just to
use Elf32_Addr and Elf64_Addr in the #defines below?

> #if ELF_CLASS == ELFCLASS32
>
> extern Elf32_Dyn _DYNAMIC [];
> #define elfhdr elf32_hdr
> #define elf_phdr elf32_phdr
> #define elf_note elf32_note
> +#define elf_addr_t elf32_addr
>
> #else
>
> @@ -365,6 +369,7 @@ extern Elf64_Dyn _DYNAMIC [];
> #define elfhdr elf64_hdr
> #define elf_phdr elf64_phdr
> #define elf_note elf64_note
> +#define elf_addr_t elf64_addr
>
> #endif

Jakub

2006-11-02 10:51:07

[permalink] [raw]

Subject: Re: [PATCH 01/02] Elf: Always define elf_addr_t in linux/elf.h

On 11/2/06, Jakub Jelinek <[email protected]> wrote:
> On Thu, Nov 02, 2006 at 07:19:42PM +0900, Magnus Damm wrote:
> > --- 0001/include/linux/elf.h
> > +++ work/include/linux/elf.h 2006-11-02 15:44:10.000000000 +0900
> > @@ -352,12 +352,16 @@ typedef struct elf64_note {
> > Elf64_Word n_type; /* Content type */
> > } Elf64_Nhdr;
> >
> > +typedef Elf64_Off elf64_addr;
> > +typedef Elf32_Off elf32_addr;
> > +
>
> What are these typedefs useful for? Isn't it better just to
> use Elf32_Addr and Elf64_Addr in the #defines below?

Sure, good idea. I just added them to follow the "style" of the rest
of the file.

Thanks,

/ magnus

2006-11-09 14:02:54

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

Magnus Damm <[email protected]> writes:

> elf: Align elf notes properly
>
> The kernel currently contains several elf note aligment implementations. Most
> implementations follow the spec on 32-bit platforms, but none current aligns
> the notes correctly on 64-bit platforms. This patch tries to fix this by
> interpreting the 64-bit and 32-bit elf specs as the following:
>
> offset bytes name
> 0 4 n_namesz -+ -+
> 4 4 n_descsz | elf note header |
> 8 4 n_type -+ | elf note entry size - N4
> 12 N1 name |
> N2 N3 desc -+
>
> WS = word size in bytes (4 for 32 bit, 8 for 64 bit)
> N1 = roundup(n_namesz + sizeof(elf note header), WS) - sizeof(elf note header)
> N2 = sizeof(elf note header) + N1
> N3 = roundup(n_descsz, WS)
> N4 = sizeof(elf note header) + N1 + N2
>
> The elf note header contains three 32-bit values on 32-bit and 64-bit systems.
> The header is followed by name and desc data together with padding. The
> alignment and padding varies depending on the word size.

I see your point and I disagree. The notes in a kernel generated
core dump do not vary in size. Find me some implementation evidence that
anyone ever added the extra 4 bytes of alignment to the description and the
padding fields and I will be ready to consider this. Currently this
just appears to be reading a draft spec that doesn't match reality.

Eric

2006-11-10 01:07:55

by Simon Horman

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

On Thu, Nov 09, 2006 at 07:00:22AM -0700, Eric W. Biederman wrote:
> Magnus Damm <[email protected]> writes:
>
> > elf: Align elf notes properly
> >
> > The kernel currently contains several elf note aligment implementations. Most
> > implementations follow the spec on 32-bit platforms, but none current aligns
> > the notes correctly on 64-bit platforms. This patch tries to fix this by
> > interpreting the 64-bit and 32-bit elf specs as the following:
> >
> > offset bytes name
> > 0 4 n_namesz -+ -+
> > 4 4 n_descsz | elf note header |
> > 8 4 n_type -+ | elf note entry size - N4
> > 12 N1 name |
> > N2 N3 desc -+
> >
> > WS = word size in bytes (4 for 32 bit, 8 for 64 bit)
> > N1 = roundup(n_namesz + sizeof(elf note header), WS) - sizeof(elf note header)
> > N2 = sizeof(elf note header) + N1
> > N3 = roundup(n_descsz, WS)
> > N4 = sizeof(elf note header) + N1 + N2
> >
> > The elf note header contains three 32-bit values on 32-bit and 64-bit systems.
> > The header is followed by name and desc data together with padding. The
> > alignment and padding varies depending on the word size.
>
> I see your point and I disagree. The notes in a kernel generated
> core dump do not vary in size. Find me some implementation evidence that
> anyone ever added the extra 4 bytes of alignment to the description and the
> padding fields and I will be ready to consider this. Currently this
> just appears to be reading a draft spec that doesn't match reality.

Or perhaps a spec that hasn't been implemented correctly.
I guess that the real question is, what padding is correct?

--
Horms
H: http://www.vergenet.net/~horms/
W: http://www.valinux.co.jp/en/

2006-11-10 03:52:43

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

Hi Eric,

Thanks for your answer.

On 11/9/06, Eric W. Biederman <[email protected]> wrote:
> Magnus Damm <[email protected]> writes:
>
> > elf: Align elf notes properly
> >
> > The kernel currently contains several elf note aligment implementations. Most
> > implementations follow the spec on 32-bit platforms, but none current aligns
> > the notes correctly on 64-bit platforms. This patch tries to fix this by
> > interpreting the 64-bit and 32-bit elf specs as the following:
> >
> > offset bytes name
> > 0 4 n_namesz -+ -+
> > 4 4 n_descsz | elf note header |
> > 8 4 n_type -+ | elf note entry size - N4
> > 12 N1 name |
> > N2 N3 desc -+
> >
> > WS = word size in bytes (4 for 32 bit, 8 for 64 bit)
> > N1 = roundup(n_namesz + sizeof(elf note header), WS) - sizeof(elf note header)
> > N2 = sizeof(elf note header) + N1
> > N3 = roundup(n_descsz, WS)
> > N4 = sizeof(elf note header) + N1 + N2
> >
> > The elf note header contains three 32-bit values on 32-bit and 64-bit systems.
> > The header is followed by name and desc data together with padding. The
> > alignment and padding varies depending on the word size.
>
> I see your point and I disagree. The notes in a kernel generated
> core dump do not vary in size. Find me some implementation evidence that
> anyone ever added the extra 4 bytes of alignment to the description and the
> padding fields and I will be ready to consider this. Currently this
> just appears to be reading a draft spec that doesn't match reality.

I'm not sure you see all my points. The important parts are the
offsets - offset 0 and offset N2 in the description above. The should
be aligned somehow. Exactly how to align them depends on if the 64-bit
spec is valid or not.

My points are:

- Some kdump code rounds up the size of "elf note header" today. This
is unneccessary for 32 bit alignment and plain wrong for 64 bit
alignment. So I think that the code is strange and should be changed
regardless if the 64-bit spec is valid or not.

- Many implementations incorrectly calculate N2 as: roundup(sizeof(elf
note header)) + roundup(n_namesz).

- You say that the size of the notes do not vary and therefore this is
a non-issue. I agree that the size does not vary, but I believe that
the aligment _is_ an issue. One example is the N2 calculation above,
but more importantly the vmcore code that merges the elf note sections
into one. You know, if you have more than one cpu you will end up with
more than one crash note. And if you run Xen you will have even more
crash notes.

- On top of this I think it would be nice if all this code could be
unified to avoid code duplication. But we need to straighten out this
and agree on how the aligment should work before the code can be
merged into one implementation.

Thanks,

/ magnus

2006-11-10 04:00:13

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

On 11/10/06, Horms <[email protected]> wrote:
> On Thu, Nov 09, 2006 at 07:00:22AM -0700, Eric W. Biederman wrote:
> > Magnus Damm <[email protected]> writes:
> >
> > > elf: Align elf notes properly
> > >
> > > The kernel currently contains several elf note aligment implementations. Most
> > > implementations follow the spec on 32-bit platforms, but none current aligns
> > > the notes correctly on 64-bit platforms. This patch tries to fix this by
> > > interpreting the 64-bit and 32-bit elf specs as the following:
> > >
> > > offset bytes name
> > > 0 4 n_namesz -+ -+
> > > 4 4 n_descsz | elf note header |
> > > 8 4 n_type -+ | elf note entry size - N4
> > > 12 N1 name |
> > > N2 N3 desc -+
> > >
> > > WS = word size in bytes (4 for 32 bit, 8 for 64 bit)
> > > N1 = roundup(n_namesz + sizeof(elf note header), WS) - sizeof(elf note header)
> > > N2 = sizeof(elf note header) + N1
> > > N3 = roundup(n_descsz, WS)
> > > N4 = sizeof(elf note header) + N1 + N2
> > >
> > > The elf note header contains three 32-bit values on 32-bit and 64-bit systems.
> > > The header is followed by name and desc data together with padding. The
> > > alignment and padding varies depending on the word size.
> >
> > I see your point and I disagree. The notes in a kernel generated
> > core dump do not vary in size. Find me some implementation evidence that
> > anyone ever added the extra 4 bytes of alignment to the description and the
> > padding fields and I will be ready to consider this. Currently this
> > just appears to be reading a draft spec that doesn't match reality.
>
> Or perhaps a spec that hasn't been implemented correctly.
> I guess that the real question is, what padding is correct?

I see no point in aligning to 32-bit boundaries on 64-bit platforms.
Their intention was most likely to align to the word size IMO, so this
is most likely a "thinko" left over from whoever ported the code from
32-bit to 64-bit.

How we chose to align on 64-bit platforms is another issue, either we
fix it soon or live with the fact that we are not following the 64-bit
draft spec. Either way is fine with me.

/ magnus

2006-11-10 05:11:32

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

"Magnus Damm" <[email protected]> writes:

> I'm not sure you see all my points. The important parts are the
> offsets - offset 0 and offset N2 in the description above. The should
> be aligned somehow. Exactly how to align them depends on if the 64-bit
> spec is valid or not.
>
> My points are:
>
> - Some kdump code rounds up the size of "elf note header" today. This
> is unneccessary for 32 bit alignment and plain wrong for 64 bit
> alignment. So I think that the code is strange and should be changed
> regardless if the 64-bit spec is valid or not.

Sure that is reasonable, if correct.

> - Many implementations incorrectly calculate N2 as: roundup(sizeof(elf
> note header)) + roundup(n_namesz).

I am not certain that is incorrect. roundup(sizeof(elf note header), 4) +
roundup(n_namesize, 4) will yield something that is properly 4 byte aligned.
I do agree that implementation is not correct for 8 byte alignment. 8 byte
alignment does not appear to be in widespread use in the wild.

> - You say that the size of the notes do not vary and therefore this is
> a non-issue. I agree that the size does not vary, but I believe that
> the aligment _is_ an issue. One example is the N2 calculation above,
> but more importantly the vmcore code that merges the elf note sections
> into one. You know, if you have more than one cpu you will end up with
> more than one crash note. And if you run Xen you will have even more
> crash notes.

Sure that is clearly an issue.

> - On top of this I think it would be nice if all this code could be
> unified to avoid code duplication. But we need to straighten out this
> and agree on how the aligment should work before the code can be
> merged into one implementation.

Sure.

To verify your claim that 8 byte alignment is correct I checked the
core dump code in fs/binfmt_elf.c in the linux kernel. That always
uses 4 byte alignment. Therefore it appears clear that only doing
4 byte alignment is not a local misreading of the spec, and is used in
other implementations. If you can find an implementation that uses
8 byte alignment I am willing to consider it.

The current situation is that the linux kernel generated application
core dumps use 4 byte alignment so I expect that is what existing
applications such as gdb expect.

Therefore we use 4 byte alignment unless it can be shown that the
linux core dumps are a fluke and should be fixed.

Eric

2006-11-10 06:54:00

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

On 11/10/06, Eric W. Biederman <[email protected]> wrote:
> "Magnus Damm" <[email protected]> writes:
>
> > I'm not sure you see all my points. The important parts are the
> > offsets - offset 0 and offset N2 in the description above. The should
> > be aligned somehow. Exactly how to align them depends on if the 64-bit
> > spec is valid or not.
> >
> > My points are:
> >
> > - Some kdump code rounds up the size of "elf note header" today. This
> > is unneccessary for 32 bit alignment and plain wrong for 64 bit
> > alignment. So I think that the code is strange and should be changed
> > regardless if the 64-bit spec is valid or not.
>
> Sure that is reasonable, if correct.
>
> > - Many implementations incorrectly calculate N2 as: roundup(sizeof(elf
> > note header)) + roundup(n_namesz).
>
> I am not certain that is incorrect. roundup(sizeof(elf note header), 4) +
> roundup(n_namesize, 4) will yield something that is properly 4 byte aligned.
> I do agree that implementation is not correct for 8 byte alignment. 8 byte
> alignment does not appear to be in widespread use in the wild.

You are correct that it only matters if we are interested in 8 byte
alignment. So it should be a non-issue for the 4-byte aligment case.

> > - You say that the size of the notes do not vary and therefore this is
> > a non-issue. I agree that the size does not vary, but I believe that
> > the aligment _is_ an issue. One example is the N2 calculation above,
> > but more importantly the vmcore code that merges the elf note sections
> > into one. You know, if you have more than one cpu you will end up with
> > more than one crash note. And if you run Xen you will have even more
> > crash notes.
>
> Sure that is clearly an issue.
>
> > - On top of this I think it would be nice if all this code could be
> > unified to avoid code duplication. But we need to straighten out this
> > and agree on how the aligment should work before the code can be
> > merged into one implementation.
>
> Sure.
>
> To verify your claim that 8 byte alignment is correct I checked the
> core dump code in fs/binfmt_elf.c in the linux kernel. That always
> uses 4 byte alignment. Therefore it appears clear that only doing
> 4 byte alignment is not a local misreading of the spec, and is used in
> other implementations. If you can find an implementation that uses
> 8 byte alignment I am willing to consider it.

Yes, fs/binfmt_elf.c is one of the files that my patch modifies. There
are several elf note implementations in the kernel, all seem to use
4-byte aligment.

Implementations that use 8-byte alignment:

binutils-2.16.1/bfd/elf.c: elf_core_write_note() is using
log_file_align which is set to 3 on some 64-bit platforms. 8-byte
alignment in some cases.

binutils-2.16.1/binutils/readelf.c: process_corefile_note_segment() is
always using 4-byte alignment though.

> The current situation is that the linux kernel generated application
> core dumps use 4 byte alignment so I expect that is what existing
> applications such as gdb expect.

Most applications probably expect 4-byte aligned data. OTOH, I just
came across HP's ELF-64 Object File Format document. It says that
8-byte alignment should be used:

http://devresource.hp.com/drc/STK/docs/refs/elf-64-hp.pdf

So now we have two documents that say 8-byte alignment should be used.

> Therefore we use 4 byte alignment unless it can be shown that the
> linux core dumps are a fluke and should be fixed.

Ok. Vivek, Dave, anyone? Comments?

/ magnus

2006-11-10 14:50:23

by Vivek Goyal

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

On Fri, Nov 10, 2006 at 03:53:57PM +0900, Magnus Damm wrote:
[..]
> >Sure.
> >
> >To verify your claim that 8 byte alignment is correct I checked the
> >core dump code in fs/binfmt_elf.c in the linux kernel. That always
> >uses 4 byte alignment. Therefore it appears clear that only doing
> >4 byte alignment is not a local misreading of the spec, and is used in
> >other implementations. If you can find an implementation that uses
> >8 byte alignment I am willing to consider it.
>
> Yes, fs/binfmt_elf.c is one of the files that my patch modifies. There
> are several elf note implementations in the kernel, all seem to use
> 4-byte aligment.
>
> Implementations that use 8-byte alignment:
>
> binutils-2.16.1/bfd/elf.c: elf_core_write_note() is using
> log_file_align which is set to 3 on some 64-bit platforms. 8-byte
> alignment in some cases.
>
> binutils-2.16.1/binutils/readelf.c: process_corefile_note_segment() is
> always using 4-byte alignment though.
>
> >The current situation is that the linux kernel generated application
> >core dumps use 4 byte alignment so I expect that is what existing
> >applications such as gdb expect.
>
> Most applications probably expect 4-byte aligned data. OTOH, I just
> came across HP's ELF-64 Object File Format document. It says that
> 8-byte alignment should be used:
>
> http://devresource.hp.com/drc/STK/docs/refs/elf-64-hp.pdf
>
> So now we have two documents that say 8-byte alignment should be used.
>
> >Therefore we use 4 byte alignment unless it can be shown that the
> >linux core dumps are a fluke and should be fixed.
>
> Ok. Vivek, Dave, anyone? Comments?
>

IMHO, I think we should go by the specs (8byte boundary alignment on 64bit
platforms) until and unless it can be proven that specs are wrong. This
probably will mean that we will break things for sometime (until and unless
it is fixed in tool chain and probably will also break the capability to use
an older kernel for capturing dump). But that's unavoidable if we want to be
compliant to specs.

Thanks
Vivek

2006-11-10 16:03:29

by Dave Anderson

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

>
> > >Therefore we use 4 byte alignment unless it can be shown that the
> > >linux core dumps are a fluke and should be fixed.
> >
> > Ok. Vivek, Dave, anyone? Comments?
> >
>
> IMHO, I think we should go by the specs (8byte boundary alignment on 64bit
> platforms) until and unless it can be proven that specs are wrong. This
> probably will mean that we will break things for sometime (until and unless
> it is fixed in tool chain and probably will also break the capability to use
> an older kernel for capturing dump). But that's unavoidable if we want to be
> compliant to specs.
>
> Thanks
> Vivek

IMHO, why break things if it's not necessary? As I understand it, you can
still take the course of least resistance and implement 64-bit xen/kdump
vmcores with 4-byte alignment -- and everybody's happy, right?

Unlike other tools that could potentially be broken, the crash utility will have
to maintain backwards compatibility for all the other 4-byte aligned 64-bit
vmcores out there. So to me, it's more a PITA than anything else, and
I'll just adapt it to whatever's out there...

Thanks,
Dave

2006-11-10 16:13:11

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

Vivek Goyal <[email protected]> writes:
>
> IMHO, I think we should go by the specs (8byte boundary alignment on 64bit
> platforms) until and unless it can be proven that specs are wrong. This
> probably will mean that we will break things for sometime (until and unless
> it is fixed in tool chain and probably will also break the capability to use
> an older kernel for capturing dump). But that's unavoidable if we want to be
> compliant to specs.

I just looked a little more, and the notes gcc generates on x86_64 are only 4
byte aligned. (.note.ABI-tag)

The linux kernel gcc, gdb. I think that is enough to say that notes need to
be 4 byte aligned on Linux. The core ELF spec also calls out 4 byte alignment
(although it does not mention ELFCLASS64).

I think the evidence is that someone intended to the alignment to go to 8 bytes
with the move to 64bits but it did not catch on in the real world.

So yes I believe the evidence is quite strong that the spec is wrong.
(Not on some rare platforms certainly but in general).

Eric

2006-11-10 23:37:26

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

Magnus Damm wrote:
> I see no point in aligning to 32-bit boundaries on 64-bit platforms.
> Their intention was most likely to align to the word size IMO, so this
> is most likely a "thinko" left over from whoever ported the code from
> 32-bit to 64-bit.

I don't think so. Since Elf64 notes still have 32-bit values in them,
32-bit alignment seems the most sensible. It would certainly be an
irritation to have Elf32 and Elf64 Notes with basically the same
definition, but with different alignments.

J

2006-11-10 23:39:15

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

Eric W. Biederman wrote:
> To verify your claim that 8 byte alignment is correct I checked the
> core dump code in fs/binfmt_elf.c in the linux kernel. That always
> uses 4 byte alignment. Therefore it appears clear that only doing
> 4 byte alignment is not a local misreading of the spec, and is used in
> other implementations. If you can find an implementation that uses
> 8 byte alignment I am willing to consider it.
>

I wrote the Elf core code, so I may be carrying the same misreading into
multiple places. In my defence, I think I wrote that before there were
any 64-bit Linux ports (hm, Alpha might have been around).

J

2006-11-10 23:39:30

by David Miller

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

From: Jeremy Fitzhardinge <[email protected]>
Date: Fri, 10 Nov 2006 15:37:19 -0800

> Magnus Damm wrote:
> > I see no point in aligning to 32-bit boundaries on 64-bit platforms.
> > Their intention was most likely to align to the word size IMO, so this
> > is most likely a "thinko" left over from whoever ported the code from
> > 32-bit to 64-bit.
>
> I don't think so. Since Elf64 notes still have 32-bit values in them,
> 32-bit alignment seems the most sensible. It would certainly be an
> irritation to have Elf32 and Elf64 Notes with basically the same
> definition, but with different alignments.

I think Elf64 notes very much would need 64-bit alignment, especially
if there are u64 objects in there. Otherwise it would not be possible
to directly dereference such objects without taking unaligned faults
on several types of RISC cpus.

2006-11-11 00:26:51

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

David Miller wrote:
> I think Elf64 notes very much would need 64-bit alignment, especially
> if there are u64 objects in there. Otherwise it would not be possible
> to directly dereference such objects without taking unaligned faults
> on several types of RISC cpus.
>

That doesn't appear to have been a problem.

J

2006-11-11 00:43:49

by David Miller

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

From: Jeremy Fitzhardinge <[email protected]>
Date: Fri, 10 Nov 2006 16:26:46 -0800

> David Miller wrote:
> > I think Elf64 notes very much would need 64-bit alignment, especially
> > if there are u64 objects in there. Otherwise it would not be possible
> > to directly dereference such objects without taking unaligned faults
> > on several types of RISC cpus.
> >
>
> That doesn't appear to have been a problem.

We should be OK with the elf note header since n_namesz, n_descsz, and
n_type are 32-bit types even on Elf64. But for the contents embedded
in the note, I am not convinced that there are no potential issues.

2006-11-11 01:20:56

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

David Miller wrote:
> We should be OK with the elf note header since n_namesz, n_descsz, and
> n_type are 32-bit types even on Elf64. But for the contents embedded
> in the note, I am not convinced that there are no potential issues

PT_NOTE segments are not generally mmaped directly, nor are they
generally very large. There should be no problem for a note-using
program to load/copy the notes into memory with appropriate alignment.
I guess a lot of the contents of core elf notes are register dumps and
so on, so debuggers must be already dealing with this.

J

2006-11-13 00:48:35

by Simon Horman

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

On Fri, Nov 10, 2006 at 04:43:49PM -0800, David Miller wrote:
> From: Jeremy Fitzhardinge <[email protected]>
> Date: Fri, 10 Nov 2006 16:26:46 -0800
>
> > David Miller wrote:
> > > I think Elf64 notes very much would need 64-bit alignment, especially
> > > if there are u64 objects in there. Otherwise it would not be possible
> > > to directly dereference such objects without taking unaligned faults
> > > on several types of RISC cpus.
> > >
> >
> > That doesn't appear to have been a problem.
>
> We should be OK with the elf note header since n_namesz, n_descsz, and
> n_type are 32-bit types even on Elf64. But for the contents embedded
> in the note, I am not convinced that there are no potential issues.

The interesting thing is that from my reading of the elf64 spec,
name ends up being 32 bit alightned, and desc ends up being 64 bit
aligned. Do you think the former could be a problem? I am thinking not
as its always a string.

--
Horms
H: http://www.vergenet.net/~horms/
W: http://www.valinux.co.jp/en/

2006-11-13 01:47:45

by David Miller

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

From: Horms <[email protected]>
Date: Mon, 13 Nov 2006 09:23:03 +0900

> On Fri, Nov 10, 2006 at 04:43:49PM -0800, David Miller wrote:
> > From: Jeremy Fitzhardinge <[email protected]>
> > Date: Fri, 10 Nov 2006 16:26:46 -0800
> >
> > > David Miller wrote:
> > > > I think Elf64 notes very much would need 64-bit alignment, especially
> > > > if there are u64 objects in there. Otherwise it would not be possible
> > > > to directly dereference such objects without taking unaligned faults
> > > > on several types of RISC cpus.
> > > >
> > >
> > > That doesn't appear to have been a problem.
> >
> > We should be OK with the elf note header since n_namesz, n_descsz, and
> > n_type are 32-bit types even on Elf64. But for the contents embedded
> > in the note, I am not convinced that there are no potential issues.
>
> The interesting thing is that from my reading of the elf64 spec,
> name ends up being 32 bit alightned, and desc ends up being 64 bit
> aligned. Do you think the former could be a problem? I am thinking not
> as its always a string.

Right, it should be fine.

2006-11-13 02:16:32

[permalink] [raw]

Subject: Re: [PATCH 02/02] Elf: Align elf notes properly

On 11/11/06, Jeremy Fitzhardinge <[email protected]> wrote:
> David Miller wrote:
> > We should be OK with the elf note header since n_namesz, n_descsz, and
> > n_type are 32-bit types even on Elf64. But for the contents embedded
> > in the note, I am not convinced that there are no potential issues
>
> PT_NOTE segments are not generally mmaped directly, nor are they
> generally very large. There should be no problem for a note-using
> program to load/copy the notes into memory with appropriate alignment.
> I guess a lot of the contents of core elf notes are register dumps and
> so on, so debuggers must be already dealing with this.

Someone apparently thought that 32-bit alignment was a good thing and
put it in the spec for the 32-bit format. You argue that most programs
copy instead of mmap() which sounds correct, but if someone wants to
mmap() then our current 32-bit aligned 64-bit elf note implementation
is broken. Which may or may not be ok.

So why was 32-bit alignment put in the 32-bit spec? I suspect the
reason was to support direct access of note contents. Either using
mmap() or read() and direct access. Remeber that the notes could
contain anything which may require properly aligned data for direct
access. I think this was the reason the word size alignment was put in
the spec for in the first place.

Thanks,

/ magnus

2006-11-13 03:05:20