[PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

[PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

[Denys Vlasenko]

This note has the following format:

long count     -- how many files are mapped
long page_size -- units for file_ofs
array of [COUNT] elements of
   long start
   long end
   long file_ofs
followed by COUNT filenames in ASCII: "FILE1" NUL "FILE2" NUL...

If file name is not available, "" string is encoded.

Changes since previous version: rediffed on top of NT_SIGINFO patches.

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
---
 fs/binfmt_elf.c     |   85 ++++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/elf.h |    1 +
 2 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 6872e45..843d5ea 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1380,6 +1380,72 @@ static void fill_siginfo_note(struct memelfnote *note, siginfo_t *csigdata, sigi
 	fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata);
 }
 
+#define MAX_FILE_NOTE_SIZE (4*1024*1024)
+
+static void fill_files_note(struct memelfnote *note)
+{
+	struct vm_area_struct *vma;
+	struct file *file;
+	unsigned count, word_count, size, remaining;
+	long *data;
+	long *start_end_ofs;
+	char *name;
+
+	count = 0;
+	for (vma = current->mm->mmap; vma != NULL; vma = vma->vm_next) {
+		file = vma->vm_file;
+		if (!file)
+			continue;
+		count++;
+	}
+
+	size = count * 64;
+	word_count = 2 + 3 * count;
+ alloc:
+	if (size >= MAX_FILE_NOTE_SIZE) /* paranoia check */
+		goto err;
+	size = round_up(size, PAGE_SIZE);
+	data = vmalloc(size);
+	if (!data)
+		goto err;
+
+	start_end_ofs = data;
+	name = (void*)&start_end_ofs[word_count];
+	remaining = size - word_count * sizeof(long);
+
+	*start_end_ofs++ = count;
+	*start_end_ofs++ = PAGE_SIZE;
+	for (vma = current->mm->mmap; vma != NULL; vma = vma->vm_next) {
+		const char *filename;
+
+		file = vma->vm_file;
+		if (!file)
+			continue;
+		filename = d_path(&file->f_path, name, remaining);
+		if (IS_ERR(filename)) {
+			if (PTR_ERR(filename) == -ENAMETOOLONG) {
+				vfree(data);
+				size = size * 5 / 4;
+				goto alloc;
+			}
+			/* continue; - WRONG, we must have COUNT elements */
+			filename = "";
+		}
+		/* d_path() fills at the end, move it to front */
+		do
+			remaining--;
+		while ((*name++ = *filename++) != '\0');
+
+		*start_end_ofs++ = vma->vm_start;
+		*start_end_ofs++ = vma->vm_end;
+		*start_end_ofs++ = vma->vm_pgoff;
+	}
+
+	size = name - (char*)data;
+	fill_note(note, "CORE", NT_FILE, size, data);
+ err: ;
+}
+
 #ifdef CORE_DUMP_USE_REGSET
 #include <linux/regset.h>
 
@@ -1395,6 +1461,7 @@ struct elf_note_info {
 	struct memelfnote psinfo;
 	struct memelfnote signote;
 	struct memelfnote auxv;
+	struct memelfnote files;
 	siginfo_t csigdata;
 	size_t size;
 	int thread_notes;
@@ -1575,6 +1642,9 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 	fill_auxv_note(&info->auxv, current->mm);
 	info->size += notesize(&info->auxv);
 
+	fill_files_note(&info->files);
+	info->size += notesize(&info->files);
+
 	return 1;
 }
 
@@ -1605,6 +1675,8 @@ static int write_note_info(struct elf_note_info *info,
 			return 0;
 		if (first && !writenote(&info->auxv, file, foffset))
 			return 0;
+		if (first && !writenote(&info->files, file, foffset))
+			return 0;
 
 		for (i = 1; i < info->thread_notes; ++i)
 			if (t->notes[i].data &&
@@ -1631,6 +1703,7 @@ static void free_note_info(struct elf_note_info *info)
 		kfree(t);
 	}
 	kfree(info->psinfo.data);
+	vfree(info->files.data);
 }
 
 #else
@@ -1707,7 +1780,7 @@ static int elf_note_info_init(struct elf_note_info *info)
 	INIT_LIST_HEAD(&info->thread_list);
 
 	/* Allocate space for ELF notes */
-	info->notes = kmalloc(7 * sizeof(struct memelfnote), GFP_KERNEL);
+	info->notes = kmalloc(8 * sizeof(struct memelfnote), GFP_KERNEL);
 	if (!info->notes)
 		return 0;
 	info->psinfo = kmalloc(sizeof(*info->psinfo), GFP_KERNEL);
@@ -1777,10 +1850,11 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
 	fill_note(info->notes + 1, "CORE", NT_PRPSINFO,
 		  sizeof(*info->psinfo), info->psinfo);
 
-	info->numnote = 2;
+	fill_siginfo_note(info->notes + 2, &info->csigdata, siginfo);
+	fill_auxv_note(info->notes + 3, current->mm);
+	fill_files_note(info->notes + 4);
 
-	fill_siginfo_note(&info->notes[info->numnote++], &info->csigdata, siginfo);
-	fill_auxv_note(&info->notes[info->numnote++], current->mm);
+	info->numnote = 5;
 
 	/* Try to dump the FPU. */
 	info->prstatus->pr_fpvalid = elf_core_copy_task_fpregs(current, regs,
@@ -1842,6 +1916,9 @@ static void free_note_info(struct elf_note_info *info)
 		kfree(list_entry(tmp, struct elf_thread_status, list));
 	}
 
+	/* Free data allocated by fill_files_note(): */
+	vfree(info->notes[4].data);
+
 	kfree(info->prstatus);
 	kfree(info->psinfo);
 	kfree(info->notes);
diff --git a/include/linux/elf.h b/include/linux/elf.h
index dc62da7..59ef406 100644
--- a/include/linux/elf.h
+++ b/include/linux/elf.h
@@ -377,6 +377,7 @@ typedef struct elf64_shdr {
  * in the future to accomodate more fields, don't assume it is fixed!
  */
 #define NT_SIGINFO      0x53494749
+#define NT_FILE         0x46494c45
 #define NT_PRXFPREG     0x46e62b7f      /* copied from gdb5.1/include/elf/common.h */
 #define NT_PPC_VMX	0x100		/* PowerPC Altivec/VMX registers */
 #define NT_PPC_SPE	0x101		/* PowerPC SPE/EVR registers */
-- 
1.7.7.6

Re: [PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

[Oleg Nesterov]

On 09/18, Denys Vlasenko wrote:
>
> Changes since previous version: rediffed on top of NT_SIGINFO patches.

Assuming this is the only change:

Again, I can't judge whether we need this feature or not, although
I tend to trust Denys who actually works with the user-space tools
which play with coredumps.

But, from the correctness pov
Reviewed-by: Oleg Nesterov <oleg@redhat.com>

RE: [PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

[Jonathan M. Foote]

This change will be beneficial for automated vulnerability triage as well. It will allow tools (like http://www.cert.org/vuls/discovery/triage.html) to gather more information about where an application crashed and the addresses in the backtrace. This information can then be used to help determine the severity of the underlying bug.

Jon

-----Original Message-----
From: Oleg Nesterov [mailto:oleg@redhat.com] 
Sent: Tuesday, September 18, 2012 11:37 AM
To: Denys Vlasenko
Cc: linux-kernel@vger.kernel.org; Andrew Morton; Amerigo Wang; Jonathan M. Foote; Roland McGrath; Pedro Alves
Subject: Re: [PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

On 09/18, Denys Vlasenko wrote:
>
> Changes since previous version: rediffed on top of NT_SIGINFO patches.

Assuming this is the only change:

Again, I can't judge whether we need this feature or not, although I tend to trust Denys who actually works with the user-space tools which play with coredumps.

But, from the correctness pov
Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

[Roland McGrath]

The code needs to be macroized a bit so that compat_binfmt_elf.c will
produce a version that encodes 32-bit values correctly so as to be
compatible with the output of a native 32-bit kernel.

It's doubtful that rolling your own single loop actually performs better
than just calling strlen and memcpy.

Since you're just counting to estimate the size of a temporary buffer,
you could skip the initial loop and just estimate based on mm->map_count.
Then collect the count in the main loop and write the first word of the
buffer last.


Thanks,
Roland

Re: [PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

[Oleg Nesterov]

On 09/18, Roland McGrath wrote:
>
> It's doubtful that rolling your own single loop actually performs better
> than just calling strlen and memcpy.



This is needed to advance 'name' if d_path() fails with !ENAMETOOLONG.
Probably needs a comment.

Or perhaps 'filename = ""' case can handle this case itself. In this
case we do not even need strlen(), remaining = name - filename.

Oleg.

Re: [PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

[Oleg Nesterov]

On 09/18, Oleg Nesterov wrote:
>
> On 09/18, Roland McGrath wrote:
> >
> > It's doubtful that rolling your own single loop actually performs better
> > than just calling strlen and memcpy.

Ah, I misread your message, sorry...

> This is needed to advance 'name' if d_path() fails with !ENAMETOOLONG.
> Probably needs a comment.
>
> Or perhaps 'filename = ""' case can handle this case itself. In this
> case we do not even need strlen(), remaining = name - filename.

Yes, but you strlen + memcpy as you suggested will work too.

Oleg.

Re: [PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

[Oleg Nesterov]

On 09/18, Oleg Nesterov wrote:
>
> Or perhaps 'filename = ""' case can handle this case itself. In this
> case we do not even need strlen(), remaining = name - filename.

IOW. Denys, this is up to you and _iirc_ this was already discussed
(just I don't remember the result), but perhaps

	if (IS_ERR(filename)) {
		if (-ENAMETOOLONG) {
			...
			goto retry;
		}
		remaining--;
		*name++ = 0;
	} else {
		remaining = name - filename;
		strcpy(name, filename);
	}

Hmm. And I just noticed that 'filename = ""' case does not verify
there is a room to write a single '\0' :/

So, afaics you also need

	- if (-ENAMETOOLONG)
	+ if (-ENAMETOOLONG || !remaining)

or I missed something.

Oleg.

Re: [PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

[Oleg Nesterov]

On 09/18, Oleg Nesterov wrote:
>
> Hmm. And I just noticed that 'filename = ""' case does not verify
> there is a room to write a single '\0' :/
> 
> So, afaics you also need
> 
> 	- if (-ENAMETOOLONG)
> 	+ if (-ENAMETOOLONG || !remaining)

No, please ignore.

d_path() always starts with prepend("\0", 1), it should always
return ENAMETOOLONG if !remaining.

Oleg.

Re: [PATCH -mm v2] coredump: extend core dump note section to contain file names of mapped files

[Denys Vlasenko]

On Tue, Sep 18, 2012 at 6:58 PM, Roland McGrath <roland@hack.frob.com> wrote:
> The code needs to be macroized a bit so that compat_binfmt_elf.c will
> produce a version that encodes 32-bit values correctly so as to be
> compatible with the output of a native 32-bit kernel.

Just did it...

> It's doubtful that rolling your own single loop actually performs better
> than just calling strlen and memcpy.

I didn't do it for speed, rather for simplicity.

> Since you're just counting to estimate the size of a temporary buffer,
> you could skip the initial loop and just estimate based on mm->map_count.
> Then collect the count in the main loop and write the first word of the
> buffer last.

The format is:

    long count     -- how many files are mapped
    long page_size -- units for file_ofs
    array of [COUNT] elements of
       long start
       long end
       long file_ofs
    followed by COUNT filenames in ASCII: "FILE1" NUL "FILE2" NUL...

In order to fill in file names, we need to know exact number of array elements,
since file names immediately follow that array. IOW:

+       for (vma = current->mm->mmap; vma != NULL; vma = vma->vm_next) {
+               if (it is a file-backed vma)
+                       count++;
+       }
+
+       size = count * 64;
+       word_count = 2 + 3 * count;               <=====
...
+       name = (void*)&start_end_ofs[word_count]; <=====

The marked statements are crucial. We can't use estimated count,
we need exact one here in order for 'name' pointer to be correct.

Otherwise (if we would use count = mm->map_count),
we'd need to memmove block of file names at the end.
And we might end up allocating significant amounts of memory
we don't need if there are tons of anon mappings.
IMO this is worse than the counting loop.

Anyway, I will shortly send a patch which implements your logic.
-- 
vda