Re: [PATCH v3] binfmt_elf: Dump smaller VMAs first in ELF cores

["Eric W. Biederman" <ebiederm@xmission.com>]

Brian Mak <makb@juniper.net> writes:

> Large cores may be truncated in some scenarios, such as with daemons
> with stop timeouts that are not large enough or lack of disk space. This
> impacts debuggability with large core dumps since critical information
> necessary to form a usable backtrace, such as stacks and shared library
> information, are omitted.
>
> We attempted to figure out which VMAs are needed to create a useful
> backtrace, and it turned out to be a non-trivial problem. Instead, we
> try simply sorting the VMAs by size, which has the intended effect.
>
> By sorting VMAs by dump size and dumping in that order, we have a
> simple, yet effective heuristic.

To make finding the history easier I would include:
v1: https://lkml.kernel.org/r/CB8195AE-518D-44C9-9841-B2694A5C4002@juniper.net
v2: https://lkml.kernel.org/r/C21B229F-D1E6-4E44-B506-A5ED4019A9DE@juniper.net

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

As Kees has already picked this up this is quite possibly silly.
But *shrug* that was when I was out.


> Signed-off-by: Brian Mak <makb@juniper.net>
> ---
>
> Hi all,
>
> Still need to run rr tests on this, per Kees Cook's suggestion, will
> update back once done. GDB and readelf show that this patch works
> without issue though.
>
> Thanks,
> Brian Mak
>
> v3: Edited commit message to better convey alternative solution as
>     non-trivial
>
>     Moved sorting logic to fs/coredump.c to make it in place
>
>     Above edits suggested by Eric Biederman <ebiederm@xmission.com>
>
> v2: Edited commit message to include more reasoning for sorting VMAs
>     
>     Removed conditional VMA sorting with debugfs knob
>     
>     Above edits suggested by Eric Biederman <ebiederm@xmission.com>
>
>  fs/coredump.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 7f12ff6ad1d3..33c5ac53ab31 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -18,6 +18,7 @@
>  #include <linux/personality.h>
>  #include <linux/binfmts.h>
>  #include <linux/coredump.h>
> +#include <linux/sort.h>
>  #include <linux/sched/coredump.h>
>  #include <linux/sched/signal.h>
>  #include <linux/sched/task_stack.h>
> @@ -1191,6 +1192,18 @@ static void free_vma_snapshot(struct coredump_params *cprm)
>  	}
>  }
>  
> +static int cmp_vma_size(const void *vma_meta_lhs_ptr, const void *vma_meta_rhs_ptr)
> +{
> +	const struct core_vma_metadata *vma_meta_lhs = vma_meta_lhs_ptr;
> +	const struct core_vma_metadata *vma_meta_rhs = vma_meta_rhs_ptr;
> +
> +	if (vma_meta_lhs->dump_size < vma_meta_rhs->dump_size)
> +		return -1;
> +	if (vma_meta_lhs->dump_size > vma_meta_rhs->dump_size)
> +		return 1;
> +	return 0;
> +}
> +
>  /*
>   * Under the mmap_lock, take a snapshot of relevant information about the task's
>   * VMAs.
> @@ -1253,5 +1266,8 @@ static bool dump_vma_snapshot(struct coredump_params *cprm)
>  		cprm->vma_data_size += m->dump_size;
>  	}
>  
> +	sort(cprm->vma_meta, cprm->vma_count, sizeof(*cprm->vma_meta),
> +		cmp_vma_size, NULL);
> +
>  	return true;
>  }
>
> base-commit: eb5e56d1491297e0881c95824e2050b7c205f0d4