Skip to contentSkip to conversation

binfmt_elf, coredump: Log the reason of the failed core dumps

PATCH Superseded Roman Kisel 3 participants 5 messages Jul 12, 2024 – Jul 14, 2024

Subsystems (3)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

v2 cover binfmt_elf, coredump: Log the reason of the failed core dumps

A powerful way to diagnose crashes is to analyze the core dump produced upon the failure. Missing or malformed core dump files hinder these investigations. I'd like to propose changes that add logging as to why the kernel would not finish writing out the core dump file.

To help in diagnosing the user mode helper not writing out the entire coredump contents, the changes also log short statistics on the dump collection. I'd advocate for keeping this at the info level on these grounds.

For validation, I built the kernel and a simple user space to exercize the new code.

[V2]
- Used _ratelimited to avoid spamming the system log
- Added comm and PID to the log messages
- Added logging to the failure paths in dump_interrupted, dump_skip, and dump_emit
- Fixed compiler warnings produced when CONFIG_COREDUMP is disabled

[V1]
https://lore.kernel.org/all/[email protected]/

Roman Kisel (1):
binfmt_elf, coredump: Log the reason of the failed core dumps

 fs/binfmt_elf.c          |  60 ++++++++++++++++-----
 fs/coredump.c            | 109 ++++++++++++++++++++++++++++++++-------
 include/linux/coredump.h |   8 ++-
 kernel/signal.c          |  22 +++++++-
 4 files changed, 165 insertions(+), 34 deletions(-)

base-commit: 831bcbcead6668ebf20b64fdb27518f1362ace3a

Signature · 1 line
2.45.2
#1 lore raw

PATCH v2 1/1 binfmt_elf, coredump: Log the reason of the failed core dumps

Missing, failed, or corrupted core dumps might impede crash investigations. To improve reliability of that process and consequently the programs themselves, one needs to trace the path from producing a core dumpfile to analyzing it. That path starts from the core dump file written to the disk by the kernel or to the standard input of a user mode helper program to which the kernel streams the coredump contents. There are cases where the kernel will interrupt writing the core out or produce a truncated/not-well-formed core dump without leaving a note.

Add logging for the core dump collection failure paths to be able to reason what has gone wrong when the core dump is malformed or missing.

---
 fs/binfmt_elf.c          |  60 ++++++++++++++++-----
 fs/coredump.c            | 109 ++++++++++++++++++++++++++++++++-------
 include/linux/coredump.h |   8 ++-
 kernel/signal.c          |  22 +++++++-
 4 files changed, 165 insertions(+), 34 deletions(-)
fs/binfmt_elf.c +48 -12
index a43897b03ce9..cfe84b9436af 100644
@@ -1994,8 +1994,11 @@ static int elf_core_dump(struct coredump_params *cprm)
 	 * Collect all the non-memory information about the process for the
 	 * notes.  This also sets up the file header.
 	 */
-	if (!fill_note_info(&elf, e_phnum, &info, cprm))
+	if (!fill_note_info(&elf, e_phnum, &info, cprm)) {
+		pr_err_ratelimited("Error collecting note info, core dump of %s(PID %d) failed\n",
+			current->comm, current->pid);
 		goto end_coredump;
+	}
 
 	has_dumped = 1;
 
@@ -2010,8 +2013,11 @@ static int elf_core_dump(struct coredump_params *cprm)
 		sz += elf_coredump_extra_notes_size();
 
 		phdr4note = kmalloc(sizeof(*phdr4note), GFP_KERNEL);
-		if (!phdr4note)
+		if (!phdr4note) {
+			pr_err_ratelimited("Error allocating program headers note entry, core dump of %s(PID %d) failed\n",
+				current->comm, current->pid);
 			goto end_coredump;
+		}
 
 		fill_elf_note_phdr(phdr4note, sz, offset);
 		offset += sz;
@@ -2025,18 +2031,27 @@ static int elf_core_dump(struct coredump_params *cprm)
 
 	if (e_phnum == PN_XNUM) {
 		shdr4extnum = kmalloc(sizeof(*shdr4extnum), GFP_KERNEL);
-		if (!shdr4extnum)
+		if (!shdr4extnum) {
+			pr_err_ratelimited("Error allocating extra program headers, core dump of %s(PID %d) failed\n",
+				current->comm, current->pid);
 			goto end_coredump;
+		}
 		fill_extnum_info(&elf, shdr4extnum, e_shoff, segs);
 	}
 
 	offset = dataoff;
 
-	if (!dump_emit(cprm, &elf, sizeof(elf)))
+	if (!dump_emit(cprm, &elf, sizeof(elf))) {
+		pr_err_ratelimited("Error emitting the ELF header, core dump of %s(PID %d) failed\n",
+			current->comm, current->pid);
 		goto end_coredump;
+	}
 
-	if (!dump_emit(cprm, phdr4note, sizeof(*phdr4note)))
+	if (!dump_emit(cprm, phdr4note, sizeof(*phdr4note))) {
+		pr_err_ratelimited("Error emitting the program header for notes, core dump of %s(PID %d) failed\n",
+			current->comm, current->pid);
 		goto end_coredump;
+	}
 
 	/* Write program headers for segments dump */
 	for (i = 0; i < cprm->vma_count; i++) {
@@ -2059,20 +2074,32 @@ static int elf_core_dump(struct coredump_params *cprm)
 			phdr.p_flags |= PF_X;
 		phdr.p_align = ELF_EXEC_PAGESIZE;
 
-		if (!dump_emit(cprm, &phdr, sizeof(phdr)))
+		if (!dump_emit(cprm, &phdr, sizeof(phdr))) {
+			pr_err_ratelimited("Error emitting program headers, core dump of %s(PID %d) failed\n",
+				current->comm, current->pid);
 			goto end_coredump;
+		}
 	}
 
-	if (!elf_core_write_extra_phdrs(cprm, offset))
+	if (!elf_core_write_extra_phdrs(cprm, offset)) {
+		pr_err_ratelimited("Error writing out extra program headers, core dump of %s(PID %d) failed\n",
+			current->comm, current->pid);
 		goto end_coredump;
+	}
 
 	/* write out the notes section */
-	if (!write_note_info(&info, cprm))
+	if (!write_note_info(&info, cprm)) {
+		pr_err_ratelimited("Error writing out notes, core dump of %s(PID %d) failed\n",
+			current->comm, current->pid);
 		goto end_coredump;
+	}
 
 	/* For cell spufs */
-	if (elf_coredump_extra_notes_write(cprm))
+	if (elf_coredump_extra_notes_write(cprm)) {
+		pr_err_ratelimited("Error writing out extra notes, core dump of %s(PID %d) failed\n",
+			current->comm, current->pid);
 		goto end_coredump;
+	}
 
 	/* Align to page */
 	dump_skip_to(cprm, dataoff);
@@ -2080,16 +2107,25 @@ static int elf_core_dump(struct coredump_params *cprm)
 	for (i = 0; i < cprm->vma_count; i++) {
 		struct core_vma_metadata *meta = cprm->vma_meta + i;
 
-		if (!dump_user_range(cprm, meta->start, meta->dump_size))
+		if (!dump_user_range(cprm, meta->start, meta->dump_size)) {
+			pr_err_ratelimited("Error writing out the process memory, core dump of %s(PID %d) failed\n",
+				current->comm, current->pid);
 			goto end_coredump;
+		}
 	}
 
-	if (!elf_core_write_extra_data(cprm))
+	if (!elf_core_write_extra_data(cprm)) {
+		pr_err_ratelimited("Error writing out extra data, core dump of %s(PID %d) failed\n",
+			current->comm, current->pid);
 		goto end_coredump;
+	}
 
 	if (e_phnum == PN_XNUM) {
-		if (!dump_emit(cprm, shdr4extnum, sizeof(*shdr4extnum)))
+		if (!dump_emit(cprm, shdr4extnum, sizeof(*shdr4extnum))) {
+			pr_err_ratelimited("Error emitting extra program headers, core dump of %s(PID %d) failed\n",
+				current->comm, current->pid);
 			goto end_coredump;
+		}
 	}
 
 end_coredump:
fs/coredump.c +90 -19

243 lines not on this page · raw message

include/linux/coredump.h +6 -2
index 0904ba010341..e97c027ce2c9 100644
@@ -42,9 +42,13 @@ extern int dump_emit(struct coredump_params *cprm, const void *addr, int nr);
 extern int dump_align(struct coredump_params *cprm, int align);
 int dump_user_range(struct coredump_params *cprm, unsigned long start,
 		    unsigned long len);
-extern void do_coredump(const kernel_siginfo_t *siginfo);
+extern int do_coredump(const kernel_siginfo_t *siginfo);
 #else
-static inline void do_coredump(const kernel_siginfo_t *siginfo) {}
+static inline int do_coredump(const kernel_siginfo_t *siginfo)
+{
+	/* Coredump support is not available, can't fail. */
+	return 0;
+}
 #endif
 
 #if defined(CONFIG_COREDUMP) && defined(CONFIG_SYSCTL)
kernel/signal.c +21 -1
index 1f9dd41c04be..3af77048c305 100644
@@ -2880,6 +2880,8 @@ bool get_signal(struct ksignal *ksig)
 		current->flags |= PF_SIGNALED;
 
 		if (sig_kernel_coredump(signr)) {
+			int ret;
+
 			if (print_fatal_signals)
 				print_fatal_signal(signr);
 			proc_coredump_connector(current);
@@ -2891,7 +2893,25 @@ bool get_signal(struct ksignal *ksig)
 			 * first and our do_group_exit call below will use
 			 * that value and ignore the one we pass it.
 			 */
-			do_coredump(&ksig->info);
+			ret = do_coredump(&ksig->info);
+			if (ret)
+				pr_err("coredump has not been created for %s(PID %d), error %d\n",
+					current->comm, current->pid, ret);
+			else if (!IS_ENABLED(CONFIG_COREDUMP)) {
+				/*
+				 * Coredumps are not available, can't fail collecting
+				 * the coredump.
+				 *
+				 * Leave a note though that the coredump is going to be
+				 * not created. This is not an error or a warning as disabling
+				 * support for coredumps isn't commonplace, and the user
+				 * must've built the kernel with the custom config so they know
+				 * that is the case. Let them know all works as prescribed.
+				 */
+				pr_info_ratelimited("No coredump collected for %s(PID %d); "
+					"no coredump support available in the kernel configuration\n",
+					current->comm, current->pid);
+			}
 		}
 
 		/*
Signature · 1 line
2.45.2
#2 lore raw
kernel test robot noticed the following build warnings:

Hi Roman,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 831bcbcead6668ebf20b64fdb27518f1362ace3a]

url:    https://github.com/intel-lab-lkp/linux/commits/Roman-Kisel/binfmt_elf-coredump-Log-the-reason-of-the-failed-core-dumps/20240713-055749
base:   831bcbcead6668ebf20b64fdb27518f1362ace3a
patch link:    https://lore.kernel.org/r/20240712215223.605363-2-romank%40linux.microsoft.com
patch subject: [PATCH v2 1/1] binfmt_elf, coredump: Log the reason of the failed core dumps
config: arm-allnoconfig (https://download.01.org/0day-ci/archive/20240713/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project a0c6b8aef853eedaa0980f07c0a502a5a8a9740e)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240713/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All warnings (new ones prefixed by >>):

In file included from fs/coredump.c:6:
In file included from include/linux/mm.h:2253:
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]

     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> fs/coredump.c:875:34: warning: format specifies type 'long' but the argument has type 'ssize_t' (aka 'int') [-Wformat]
     874 |                         pr_err_ratelimited("Core dump of %s(PID %d) failed when writing out, error %ld\n",
         |                                                                                                    ~~~
         |                                                                                                    %zd
     875 |                                 current->comm, current->pid, n);
         |                                                              ^
   include/linux/printk.h:672:45: note: expanded from macro 'pr_err_ratelimited'
     672 |         printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |                                            ~~~     ^~~~~~~~~~~
   include/linux/printk.h:658:17: note: expanded from macro 'printk_ratelimited'
     658 |                 printk(fmt, ##__VA_ARGS__);                             \
         |                        ~~~    ^~~~~~~~~~~
   include/linux/printk.h:464:60: note: expanded from macro 'printk'
     464 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
         |                                                     ~~~    ^~~~~~~~~~~
   include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
     436 |                 _p_func(_fmt, ##__VA_ARGS__);                           \
         |                         ~~~~    ^~~~~~~~~~~
   fs/coredump.c:878:34: warning: format specifies type 'long' but the argument has type 'ssize_t' (aka 'int') [-Wformat]
     877 |                         pr_err_ratelimited("Core dump of %s(PID %d) partially written out, only %ld(of %d) bytes written\n",
         |                                                                                                 ~~~
         |                                                                                                 %zd
     878 |                                 current->comm, current->pid, n, nr);
         |                                                              ^
   include/linux/printk.h:672:45: note: expanded from macro 'pr_err_ratelimited'
     672 |         printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
         |                                            ~~~     ^~~~~~~~~~~
   include/linux/printk.h:658:17: note: expanded from macro 'printk_ratelimited'
     658 |                 printk(fmt, ##__VA_ARGS__);                             \
         |                        ~~~    ^~~~~~~~~~~
   include/linux/printk.h:464:60: note: expanded from macro 'printk'
     464 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
         |                                                     ~~~    ^~~~~~~~~~~
   include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
     436 |                 _p_func(_fmt, ##__VA_ARGS__);                           \
         |                         ~~~~    ^~~~~~~~~~~

3 warnings generated.

vim +875 fs/coredump.c

854
855 /*
856 * Core dumping helper functions. These are the only things you should
857 * do on a core-file: use only these functions to write out all the
858 * necessary info.
859 */
860 static int __dump_emit(struct coredump_params *cprm, const void *addr, int nr)
861 {
862 struct file *file = cprm->file;
863 loff_t pos = file->f_pos;
864 ssize_t n;
865 if (cprm->written + nr > cprm->limit)
866 return 0;
867
868
869 if (dump_interrupted())
870 return 0;
871 n = __kernel_write(file, addr, nr, &pos);
872 if (n != nr) {
873 if (n < 0)
874 pr_err_ratelimited("Core dump of %s(PID %d) failed when writing out, error %ld\n",

 > 875					current->comm, current->pid, n);

876 else
877 pr_err_ratelimited("Core dump of %s(PID %d) partially written out, only %ld(of %d) bytes written\n",
878 current->comm, current->pid, n, nr);
879
880 return 0;
881 }
882 file->f_pos = pos;
883 cprm->written += n;
884 cprm->pos += n;
885
886 return 1;
887 }
888

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
maintainer
#3 lore raw
In reply to Roman Kisel
Quoted 25 lines · Roman Kisel

Missing, failed, or corrupted core dumps might impede crash investigations. To improve reliability of that process and consequently the programs themselves, one needs to trace the path from producing a core dumpfile to analyzing it. That path starts from the core dump file written to the disk by the kernel or to the standard input of a user mode helper program to which the kernel streams the coredump contents. There are cases where the kernel will interrupt writing the core out or produce a truncated/not-well-formed core dump without leaving a note.

Add logging for the core dump collection failure paths to be able to reason what has gone wrong when the core dump is malformed or missing.

---
 fs/binfmt_elf.c          |  60 ++++++++++++++++-----
 fs/coredump.c            | 109 ++++++++++++++++++++++++++++++++-------
 include/linux/coredump.h |   8 ++-
 kernel/signal.c          |  22 +++++++-
 4 files changed, 165 insertions(+), 34 deletions(-)
fs/binfmt_elf.c +0 -0
index a43897b03ce9..cfe84b9436af 100644

fs/binfmt_elf.c static int elf_core_dump(struct coredump_params *cprm) · line 1999

Quoted 7 lines
fs/binfmt_elf.c +3 -1
@@ -1994,8 +1994,11 @@ static int elf_core_dump(struct coredump_params *cprm)
 	 * Collect all the non-memory information about the process for the
 	 * notes.  This also sets up the file header.
 	 */
-	if (!fill_note_info(&elf, e_phnum, &info, cprm))
+	if (!fill_note_info(&elf, e_phnum, &info, cprm)) {
+		pr_err_ratelimited("Error collecting note info, core dump of %s(PID %d) failed\n",
+			current->comm, current->pid);

A couple things come to mind for me as I scanned through this:

- Do we want to report pid or tgid?
- Do we want to report the global value or the current pid namespace mapping?

Because I notice that the existing code:

fs/binfmt_elf.c static int elf_core_dump(struct coredump_params *cprm) · line 2001

Quoted 2 lines
fs/binfmt_elf.c +0 -0
 			printk(KERN_WARNING "Pid %d(%s) over core_pipe_limit\n",
 			       task_tgid_vnr(current), current->comm);

Is reporting tgid for current's pid namespace. We should be consistent.

I think all of this might need cleaning up first before adding new reports. We should consolidate the reporting into a single function so this is easier to extend in the future. Right now the proposed patch is hand-building the report, and putting pid/comm in different places (at the end, at the beginning, with/without "of", etc), which is really just boilerplate repetition.

How about creating:

static void coredump_report_failure(const char *msg)

{
	char comm[TASK_COMM_LEN];

	task_get_comm(current, comm);

	pr_warn_ratelimited("coredump: %d(%*pE): %s\n",
			    task_tgid_vnr(current), strlen(comm), comm, msg);
}

Then in a new first patch, convert all the existing stuff:

	printk(KERN_WARNING ...)
	pr_info(...)
	etc

Since even the existing warnings are inconsistent and don't escape newlines, etc. :)

Then in patch 2 use this to add the new warnings?

Signature · 1 line
Kees Cook
#4 lore raw
Quoted 40 lines · Kees Cook
Quoted 25 lines · Roman Kisel

Missing, failed, or corrupted core dumps might impede crash investigations. To improve reliability of that process and consequently the programs themselves, one needs to trace the path from producing a core dumpfile to analyzing it. That path starts from the core dump file written to the disk by the kernel or to the standard input of a user mode helper program to which the kernel streams the coredump contents. There are cases where the kernel will interrupt writing the core out or produce a truncated/not-well-formed core dump without leaving a note.

Add logging for the core dump collection failure paths to be able to reason what has gone wrong when the core dump is malformed or missing.

---
  fs/binfmt_elf.c          |  60 ++++++++++++++++-----
  fs/coredump.c            | 109 ++++++++++++++++++++++++++++++++-------
  include/linux/coredump.h |   8 ++-
  kernel/signal.c          |  22 +++++++-
  4 files changed, 165 insertions(+), 34 deletions(-)
fs/binfmt_elf.c +0 -0
index a43897b03ce9..cfe84b9436af 100644

fs/binfmt_elf.c static int elf_core_dump(struct coredump_params *cprm) · line 1999

Quoted 7 lines
fs/binfmt_elf.c +3 -1
@@ -1994,8 +1994,11 @@ static int elf_core_dump(struct coredump_params *cprm)
  	 * Collect all the non-memory information about the process for the
  	 * notes.  This also sets up the file header.
  	 */
-	if (!fill_note_info(&elf, e_phnum, &info, cprm))
+	if (!fill_note_info(&elf, e_phnum, &info, cprm)) {
+		pr_err_ratelimited("Error collecting note info, core dump of %s(PID %d) failed\n",
+			current->comm, current->pid);

A couple things come to mind for me as I scanned through this:

- Do we want to report pid or tgid?
- Do we want to report the global value or the current pid namespace mapping?

Quoted 7 lines

Because I notice that the existing code:

fs/binfmt_elf.c static int elf_core_dump(struct coredump_params *cprm) · line 2001

Quoted 2 lines
fs/binfmt_elf.c +0 -0
  			printk(KERN_WARNING "Pid %d(%s) over core_pipe_limit\n",
  			       task_tgid_vnr(current), current->comm);

Is reporting tgid for current's pid namespace. We should be consistent.

Thanks, will update the code to be consistent with the existing logging.

Quoted 6 lines

I think all of this might need cleaning up first before adding new reports. We should consolidate the reporting into a single function so this is easier to extend in the future. Right now the proposed patch is hand-building the report, and putting pid/comm in different places (at the end, at the beginning, with/without "of", etc), which is really just boilerplate repetition.

100% agreed.

Quoted 15 lines

How about creating:

static void coredump_report_failure(const char *msg)

{
	char comm[TASK_COMM_LEN];

	task_get_comm(current, comm);

	pr_warn_ratelimited("coredump: %d(%*pE): %s\n",
			    task_tgid_vnr(current), strlen(comm), comm, msg);
}

Then in a new first patch, convert all the existing stuff:

Quoted 8 lines
	printk(KERN_WARNING ...)
	pr_info(...)
	etc

Since even the existing warnings are inconsistent and don't escape newlines, etc. :)

Then in patch 2 use this to add the new warnings?

Absolutely love that! Couldn't possibly appreciate your help more :)

Quoted 1 line
Signature · 2 lines
Thank you,
Roman