[PATCH v3 0/2] binfmt_elf, coredump: Log the reason of the failed core dumps

[PATCH v3 0/2] binfmt_elf, coredump: Log the reason of the failed core dumps

[Roman Kisel]

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.

[V3]
  - Standartized the existing logging to report TGID and comm consistently
  - Fixed compiler warnings for the 32-bit systems (used %zd in the format strings)

[V2]
  https://lore.kernel.org/all/20240712215223.605363-1-romank@linux.microsoft.com/
  - 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/20240617234133.1167523-1-romank@linux.microsoft.com/

Roman Kisel (2):
  coredump: Standartize and fix logging
  binfmt_elf, coredump: Log the reason of the failed core dumps

 fs/binfmt_elf.c          |  48 +++++++++----
 fs/coredump.c            | 150 +++++++++++++++++++++++++++------------
 include/linux/coredump.h |  30 +++++++-
 kernel/signal.c          |  21 +++++-
 4 files changed, 188 insertions(+), 61 deletions(-)


base-commit: 831bcbcead6668ebf20b64fdb27518f1362ace3a
-- 
2.45.2

[PATCH v3 1/2] coredump: Standartize and fix logging

[Roman Kisel]

The coredump code does not log the process ID and the comm
consistently, logs unescaped comm when it does log it, and
does not always use the ratelimited logging. That makes it
harder to analyze logs and puts the system at the risk of
spamming the system log incase something crashes many times
over and over again.

Fix that by logging TGID and comm (escaped) consistently and
using the ratelimited logging always.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 fs/coredump.c            | 43 +++++++++++++++-------------------------
 include/linux/coredump.h | 22 ++++++++++++++++++++
 2 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/fs/coredump.c b/fs/coredump.c
index a57a06b80f57..19d3343b93c6 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -586,8 +586,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		struct subprocess_info *sub_info;
 
 		if (ispipe < 0) {
-			printk(KERN_WARNING "format_corename failed\n");
-			printk(KERN_WARNING "Aborting core\n");
+			coredump_report_failure("format_corename failed, aborting core");
 			goto fail_unlock;
 		}
 
@@ -607,27 +606,21 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			 * right pid if a thread in a multi-threaded
 			 * core_pattern process dies.
 			 */
-			printk(KERN_WARNING
-				"Process %d(%s) has RLIMIT_CORE set to 1\n",
-				task_tgid_vnr(current), current->comm);
-			printk(KERN_WARNING "Aborting core\n");
+			coredump_report_failure("RLIMIT_CORE is set to 1, aborting core");
 			goto fail_unlock;
 		}
 		cprm.limit = RLIM_INFINITY;
 
 		dump_count = atomic_inc_return(&core_dump_count);
 		if (core_pipe_limit && (core_pipe_limit < dump_count)) {
-			printk(KERN_WARNING "Pid %d(%s) over core_pipe_limit\n",
-			       task_tgid_vnr(current), current->comm);
-			printk(KERN_WARNING "Skipping core dump\n");
+			coredump_report_failure("over core_pipe_limit, skipping core dump");
 			goto fail_dropcount;
 		}
 
 		helper_argv = kmalloc_array(argc + 1, sizeof(*helper_argv),
 					    GFP_KERNEL);
 		if (!helper_argv) {
-			printk(KERN_WARNING "%s failed to allocate memory\n",
-			       __func__);
+			coredump_report_failure("%s failed to allocate memory", __func__);
 			goto fail_dropcount;
 		}
 		for (argi = 0; argi < argc; argi++)
@@ -644,8 +637,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 
 		kfree(helper_argv);
 		if (retval) {
-			printk(KERN_INFO "Core dump to |%s pipe failed\n",
-			       cn.corename);
+			coredump_report_failure("|%s pipe failed", cn.corename);
 			goto close_fail;
 		}
 	} else {
@@ -658,10 +650,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			goto fail_unlock;
 
 		if (need_suid_safe && cn.corename[0] != '/') {
-			printk(KERN_WARNING "Pid %d(%s) can only dump core "\
-				"to fully qualified path!\n",
-				task_tgid_vnr(current), current->comm);
-			printk(KERN_WARNING "Skipping core dump\n");
+			coredump_report_failure(
+				"this process can only dump core to a fully qualified path, skipping core dump");
 			goto fail_unlock;
 		}
 
@@ -730,13 +720,13 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		idmap = file_mnt_idmap(cprm.file);
 		if (!vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, inode),
 				    current_fsuid())) {
-			pr_info_ratelimited("Core dump to %s aborted: cannot preserve file owner\n",
-					    cn.corename);
+			coredump_report_failure("Core dump to %s aborted: "
+				"cannot preserve file owner", cn.corename);
 			goto close_fail;
 		}
 		if ((inode->i_mode & 0677) != 0600) {
-			pr_info_ratelimited("Core dump to %s aborted: cannot preserve file permissions\n",
-					    cn.corename);
+			coredump_report_failure("Core dump to %s aborted: "
+				"cannot preserve file permissions", cn.corename);
 			goto close_fail;
 		}
 		if (!(cprm.file->f_mode & FMODE_CAN_WRITE))
@@ -757,7 +747,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		 * have this set to NULL.
 		 */
 		if (!cprm.file) {
-			pr_info("Core dump to |%s disabled\n", cn.corename);
+			coredump_report_failure("Core dump to |%s disabled", cn.corename);
 			goto close_fail;
 		}
 		if (!dump_vma_snapshot(&cprm))
@@ -983,11 +973,10 @@ void validate_coredump_safety(void)
 {
 	if (suid_dumpable == SUID_DUMP_ROOT &&
 	    core_pattern[0] != '/' && core_pattern[0] != '|') {
-		pr_warn(
-"Unsafe core_pattern used with fs.suid_dumpable=2.\n"
-"Pipe handler or fully qualified core dump path required.\n"
-"Set kernel.core_pattern before fs.suid_dumpable.\n"
-		);
+
+		coredump_report_failure("Unsafe core_pattern used with fs.suid_dumpable=2: "
+			"pipe handler or fully qualified core dump path required. "
+			"Set kernel.core_pattern before fs.suid_dumpable.");
 	}
 }
 
diff --git a/include/linux/coredump.h b/include/linux/coredump.h
index 0904ba010341..45e598fe3476 100644
--- a/include/linux/coredump.h
+++ b/include/linux/coredump.h
@@ -43,8 +43,30 @@ 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);
+
+/*
+ * Logging for the coredump code, ratelimited.
+ * The TGID and comm fields are added to the message.
+ */
+
+#define __COREDUMP_PRINTK(Level, Format, ...) \
+	do {	\
+		char comm[TASK_COMM_LEN];	\
+	\
+		get_task_comm(comm, current);	\
+		printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n",	\
+			task_tgid_vnr(current), (int)strlen(comm), comm, ##__VA_ARGS__);	\
+	} while (0)	\
+
+#define coredump_report(fmt, ...) __COREDUMP_PRINTK(KERN_INFO, fmt, ##__VA_ARGS__)
+#define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
+
 #else
 static inline void do_coredump(const kernel_siginfo_t *siginfo) {}
+
+#define coredump_report(...)
+#define coredump_report_failure(...)
+
 #endif
 
 #if defined(CONFIG_COREDUMP) && defined(CONFIG_SYSCTL)
-- 
2.45.2

[PATCH v3 2/2] binfmt_elf, coredump: Log the reason of the failed core dumps

[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.
Report the size of the data written to aid in diagnosing the user mode
helper.

Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
---
 fs/binfmt_elf.c          |  48 +++++++++++++-----
 fs/coredump.c            | 107 ++++++++++++++++++++++++++++++++-------
 include/linux/coredump.h |   8 ++-
 kernel/signal.c          |  21 +++++++-
 4 files changed, 150 insertions(+), 34 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index a43897b03ce9..0fa5a8af247e 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1994,8 +1994,10 @@ 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)) {
+		coredump_report_failure("Error collecting note info");
 		goto end_coredump;
+	}
 
 	has_dumped = 1;
 
@@ -2010,8 +2012,10 @@ 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) {
+			coredump_report_failure("Error allocating program headers note entry");
 			goto end_coredump;
+		}
 
 		fill_elf_note_phdr(phdr4note, sz, offset);
 		offset += sz;
@@ -2025,18 +2029,24 @@ static int elf_core_dump(struct coredump_params *cprm)
 
 	if (e_phnum == PN_XNUM) {
 		shdr4extnum = kmalloc(sizeof(*shdr4extnum), GFP_KERNEL);
-		if (!shdr4extnum)
+		if (!shdr4extnum) {
+			coredump_report_failure("Error allocating extra program headers");
 			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))) {
+		coredump_report_failure("Error emitting the ELF headers");
 		goto end_coredump;
+	}
 
-	if (!dump_emit(cprm, phdr4note, sizeof(*phdr4note)))
+	if (!dump_emit(cprm, phdr4note, sizeof(*phdr4note))) {
+		coredump_report_failure("Error emitting the program header for notes");
 		goto end_coredump;
+	}
 
 	/* Write program headers for segments dump */
 	for (i = 0; i < cprm->vma_count; i++) {
@@ -2059,20 +2069,28 @@ 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))) {
+			coredump_report_failure("Error emitting program headers");
 			goto end_coredump;
+		}
 	}
 
-	if (!elf_core_write_extra_phdrs(cprm, offset))
+	if (!elf_core_write_extra_phdrs(cprm, offset)) {
+		coredump_report_failure("Error writing out extra program headers");
 		goto end_coredump;
+	}
 
 	/* write out the notes section */
-	if (!write_note_info(&info, cprm))
+	if (!write_note_info(&info, cprm)) {
+		coredump_report_failure("Error writing out notes");
 		goto end_coredump;
+	}
 
 	/* For cell spufs */
-	if (elf_coredump_extra_notes_write(cprm))
+	if (elf_coredump_extra_notes_write(cprm)) {
+		coredump_report_failure("Error writing out extra notes");
 		goto end_coredump;
+	}
 
 	/* Align to page */
 	dump_skip_to(cprm, dataoff);
@@ -2080,16 +2098,22 @@ 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)) {
+			coredump_report_failure("Error writing out the process memory");
 			goto end_coredump;
+		}
 	}
 
-	if (!elf_core_write_extra_data(cprm))
+	if (!elf_core_write_extra_data(cprm)) {
+		coredump_report_failure("Error writing out extra data");
 		goto end_coredump;
+	}
 
 	if (e_phnum == PN_XNUM) {
-		if (!dump_emit(cprm, shdr4extnum, sizeof(*shdr4extnum)))
+		if (!dump_emit(cprm, shdr4extnum, sizeof(*shdr4extnum))) {
+			coredump_report_failure("Error emitting extra program headers");
 			goto end_coredump;
+		}
 	}
 
 end_coredump:
diff --git a/fs/coredump.c b/fs/coredump.c
index 19d3343b93c6..8c8f6748efac 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -464,7 +464,17 @@ static bool dump_interrupted(void)
 	 * but then we need to teach dump_write() to restart and clear
 	 * TIF_SIGPENDING.
 	 */
-	return fatal_signal_pending(current) || freezing(current);
+	if (fatal_signal_pending(current)) {
+		coredump_report_failure("interrupted: fatal signal pending");
+		return true;
+	}
+
+	if (freezing(current)) {
+		coredump_report_failure("interrupted: freezing");
+		return true;
+	}
+
+	return false;
 }
 
 static void wait_for_dump_helpers(struct file *file)
@@ -519,7 +529,7 @@ static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
 	return err;
 }
 
-void do_coredump(const kernel_siginfo_t *siginfo)
+int do_coredump(const kernel_siginfo_t *siginfo)
 {
 	struct core_state core_state;
 	struct core_name cn;
@@ -527,7 +537,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 	struct linux_binfmt * binfmt;
 	const struct cred *old_cred;
 	struct cred *cred;
-	int retval = 0;
+	int retval;
 	int ispipe;
 	size_t *argv = NULL;
 	int argc = 0;
@@ -551,14 +561,20 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 	audit_core_dumps(siginfo->si_signo);
 
 	binfmt = mm->binfmt;
-	if (!binfmt || !binfmt->core_dump)
+	if (!binfmt || !binfmt->core_dump) {
+		retval = -ENOEXEC;
 		goto fail;
-	if (!__get_dumpable(cprm.mm_flags))
+	}
+	if (!__get_dumpable(cprm.mm_flags)) {
+		retval = -EACCES;
 		goto fail;
+	}
 
 	cred = prepare_creds();
-	if (!cred)
+	if (!cred) {
+		retval = -EPERM;
 		goto fail;
+	}
 	/*
 	 * We cannot trust fsuid as being the "true" uid of the process
 	 * nor do we know its entire history. We only know it was tainted
@@ -587,6 +603,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 
 		if (ispipe < 0) {
 			coredump_report_failure("format_corename failed, aborting core");
+			retval = ispipe;
 			goto fail_unlock;
 		}
 
@@ -607,6 +624,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 			 * core_pattern process dies.
 			 */
 			coredump_report_failure("RLIMIT_CORE is set to 1, aborting core");
+			retval = -EPERM;
 			goto fail_unlock;
 		}
 		cprm.limit = RLIM_INFINITY;
@@ -614,6 +632,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		dump_count = atomic_inc_return(&core_dump_count);
 		if (core_pipe_limit && (core_pipe_limit < dump_count)) {
 			coredump_report_failure("over core_pipe_limit, skipping core dump");
+			retval = -E2BIG;
 			goto fail_dropcount;
 		}
 
@@ -621,6 +640,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 					    GFP_KERNEL);
 		if (!helper_argv) {
 			coredump_report_failure("%s failed to allocate memory", __func__);
+			retval = -ENOMEM;
 			goto fail_dropcount;
 		}
 		for (argi = 0; argi < argc; argi++)
@@ -646,12 +666,16 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		int open_flags = O_CREAT | O_WRONLY | O_NOFOLLOW |
 				 O_LARGEFILE | O_EXCL;
 
-		if (cprm.limit < binfmt->min_coredump)
+		if (cprm.limit < binfmt->min_coredump) {
+			coredump_report_failure("over coredump resource limit, skipping core dump");
+			retval = -E2BIG;
 			goto fail_unlock;
+		}
 
 		if (need_suid_safe && cn.corename[0] != '/') {
 			coredump_report_failure(
 				"this process can only dump core to a fully qualified path, skipping core dump");
+			retval = -EPERM;
 			goto fail_unlock;
 		}
 
@@ -697,20 +721,28 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		} else {
 			cprm.file = filp_open(cn.corename, open_flags, 0600);
 		}
-		if (IS_ERR(cprm.file))
+		if (IS_ERR(cprm.file)) {
+			retval = PTR_ERR(cprm.file);
 			goto fail_unlock;
+		}
 
 		inode = file_inode(cprm.file);
-		if (inode->i_nlink > 1)
+		if (inode->i_nlink > 1) {
+			retval = -EMLINK;
 			goto close_fail;
-		if (d_unhashed(cprm.file->f_path.dentry))
+		}
+		if (d_unhashed(cprm.file->f_path.dentry)) {
+			retval = -EEXIST;
 			goto close_fail;
+		}
 		/*
 		 * AK: actually i see no reason to not allow this for named
 		 * pipes etc, but keep the previous behaviour for now.
 		 */
-		if (!S_ISREG(inode->i_mode))
+		if (!S_ISREG(inode->i_mode)) {
+			retval = -EISDIR;
 			goto close_fail;
+		}
 		/*
 		 * Don't dump core if the filesystem changed owner or mode
 		 * of the file during file creation. This is an issue when
@@ -722,17 +754,22 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 				    current_fsuid())) {
 			coredump_report_failure("Core dump to %s aborted: "
 				"cannot preserve file owner", cn.corename);
+			retval = -EPERM;
 			goto close_fail;
 		}
 		if ((inode->i_mode & 0677) != 0600) {
 			coredump_report_failure("Core dump to %s aborted: "
 				"cannot preserve file permissions", cn.corename);
+			retval = -EPERM;
 			goto close_fail;
 		}
-		if (!(cprm.file->f_mode & FMODE_CAN_WRITE))
+		if (!(cprm.file->f_mode & FMODE_CAN_WRITE)) {
+			retval = -EACCES;
 			goto close_fail;
-		if (do_truncate(idmap, cprm.file->f_path.dentry,
-				0, 0, cprm.file))
+		}
+		retval = do_truncate(idmap, cprm.file->f_path.dentry,
+				0, 0, cprm.file);
+		if (retval)
 			goto close_fail;
 	}
 
@@ -748,10 +785,15 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		 */
 		if (!cprm.file) {
 			coredump_report_failure("Core dump to |%s disabled", cn.corename);
+			retval = -EPERM;
 			goto close_fail;
 		}
-		if (!dump_vma_snapshot(&cprm))
+		if (!dump_vma_snapshot(&cprm)) {
+			coredump_report_failure("Can't get VMA snapshot for core dump |%s",
+				cn.corename);
+			retval = -EACCES;
 			goto close_fail;
+		}
 
 		file_start_write(cprm.file);
 		core_dumped = binfmt->core_dump(&cprm);
@@ -767,9 +809,21 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 		}
 		file_end_write(cprm.file);
 		free_vma_snapshot(&cprm);
+	} else {
+		coredump_report_failure("Core dump to %s%s has been interrupted",
+			ispipe ? "|" : "", cn.corename);
+		retval = -EAGAIN;
+		goto fail;
 	}
+	coredump_report(
+		"written to %s%s: VMAs: %d, size %zu; core: %lld bytes, pos %lld",
+		ispipe ? "|" : "", cn.corename,
+		cprm.vma_count, cprm.vma_data_size, cprm.written, cprm.pos);
 	if (ispipe && core_pipe_limit)
 		wait_for_dump_helpers(cprm.file);
+
+	retval = 0;
+
 close_fail:
 	if (cprm.file)
 		filp_close(cprm.file, NULL);
@@ -784,7 +838,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
 fail_creds:
 	put_cred(cred);
 fail:
-	return;
+	return retval;
 }
 
 /*
@@ -804,8 +858,16 @@ static int __dump_emit(struct coredump_params *cprm, const void *addr, int nr)
 	if (dump_interrupted())
 		return 0;
 	n = __kernel_write(file, addr, nr, &pos);
-	if (n != nr)
+	if (n != nr) {
+		if (n < 0)
+			coredump_report_failure("failed when writing out, error %zd", n);
+		else
+			coredump_report_failure(
+				"partially written out, only %zd(of %d) bytes written",
+				n, nr);
+
 		return 0;
+	}
 	file->f_pos = pos;
 	cprm->written += n;
 	cprm->pos += n;
@@ -818,9 +880,16 @@ static int __dump_skip(struct coredump_params *cprm, size_t nr)
 	static char zeroes[PAGE_SIZE];
 	struct file *file = cprm->file;
 	if (file->f_mode & FMODE_LSEEK) {
-		if (dump_interrupted() ||
-		    vfs_llseek(file, nr, SEEK_CUR) < 0)
+		int ret;
+
+		if (dump_interrupted())
 			return 0;
+
+		ret = vfs_llseek(file, nr, SEEK_CUR);
+		if (ret < 0) {
+			coredump_report_failure("failed when seeking, error %d", ret);
+			return 0;
+		}
 		cprm->pos += nr;
 		return 1;
 	} else {
diff --git a/include/linux/coredump.h b/include/linux/coredump.h
index 45e598fe3476..edeb8532ce0f 100644
--- a/include/linux/coredump.h
+++ b/include/linux/coredump.h
@@ -42,7 +42,7 @@ 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);
 
 /*
  * Logging for the coredump code, ratelimited.
@@ -62,7 +62,11 @@ extern void do_coredump(const kernel_siginfo_t *siginfo);
 #define coredump_report_failure(fmt, ...) __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
 
 #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;
+}
 
 #define coredump_report(...)
 #define coredump_report_failure(...)
diff --git a/kernel/signal.c b/kernel/signal.c
index 1f9dd41c04be..4d1ed397b175 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -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,24 @@ 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)
+				coredump_report_failure("coredump has not been created, error %d",
+					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 in the kernel for coredumps isn't commonplace, and
+				 * the user must've built the kernel with the custom config so
+				 * let them know all works as desired.
+				 */
+				coredump_report("no coredump collected as "
+					"that is disabled in the kernel configuration");
+			}
 		}
 
 		/*
-- 
2.45.2

Re: [PATCH v3 0/2] binfmt_elf, coredump: Log the reason of the failed core dumps

[Kees Cook]

On Thu, Jul 18, 2024 at 11:27:23AM -0700, Roman Kisel wrote:
> 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.
> 
> [V3]
>   - Standartized the existing logging to report TGID and comm consistently
>   - Fixed compiler warnings for the 32-bit systems (used %zd in the format strings)
> 
> [V2]
>   https://lore.kernel.org/all/20240712215223.605363-1-romank@linux.microsoft.com/
>   - 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/20240617234133.1167523-1-romank@linux.microsoft.com/
> 
> Roman Kisel (2):
>   coredump: Standartize and fix logging
>   binfmt_elf, coredump: Log the reason of the failed core dumps
> 
>  fs/binfmt_elf.c          |  48 +++++++++----
>  fs/coredump.c            | 150 +++++++++++++++++++++++++++------------
>  include/linux/coredump.h |  30 +++++++-
>  kernel/signal.c          |  21 +++++-
>  4 files changed, 188 insertions(+), 61 deletions(-)

This looks good to me! I'll put this in -next once the merge window
closes. Thanks!

-Kees

-- 
Kees Cook

Re: [PATCH v3 0/2] binfmt_elf, coredump: Log the reason of the failed core dumps

[Roman Kisel]

On 7/19/2024 10:26 AM, Kees Cook wrote:
> On Thu, Jul 18, 2024 at 11:27:23AM -0700, Roman Kisel wrote:
>> 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.
>>
>> [V3]
>>    - Standartized the existing logging to report TGID and comm consistently
>>    - Fixed compiler warnings for the 32-bit systems (used %zd in the format strings)
>>
>> [V2]
>>    https://lore.kernel.org/all/20240712215223.605363-1-romank@linux.microsoft.com/
>>    - 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/20240617234133.1167523-1-romank@linux.microsoft.com/
>>
>> Roman Kisel (2):
>>    coredump: Standartize and fix logging
>>    binfmt_elf, coredump: Log the reason of the failed core dumps
>>
>>   fs/binfmt_elf.c          |  48 +++++++++----
>>   fs/coredump.c            | 150 +++++++++++++++++++++++++++------------
>>   include/linux/coredump.h |  30 +++++++-
>>   kernel/signal.c          |  21 +++++-
>>   4 files changed, 188 insertions(+), 61 deletions(-)
> 
> This looks good to me! I'll put this in -next once the merge window
> closes. Thanks!
> 
Kees, thank you for your guidance!

> -Kees
> 

-- 
Thank you,
Roman

Re: [PATCH v3 1/2] coredump: Standartize and fix logging

[Roman Kisel]

On 7/19/2024 10:33 AM, Allen Pais wrote:
> 
> 
>> On Jul 18, 2024, at 11:27 AM, Roman Kisel <romank@linux.microsoft.com> 
>> wrote:
>>
>> The coredump code does not log the process ID and the comm
>> consistently, logs unescaped comm when it does log it, and
>> does not always use the ratelimited logging. That makes it
>> harder to analyze logs and puts the system at the risk of
>> spamming the system log incase something crashes many times
>> over and over again.
>>
>> Fix that by logging TGID and comm (escaped) consistently and
>> using the ratelimited logging always.
>>
>> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> 
> LGTM.
> 
> Tested-by: Allen Pais <apais@linux.microsoft.com 
> <mailto:apais@linux.microsoft.com>>
Allen, thank you for your help!

> 
> Thanks.
> 
> 
>> ---
>> fs/coredump.c            | 43 +++++++++++++++-------------------------
>> include/linux/coredump.h | 22 ++++++++++++++++++++
>> 2 files changed, 38 insertions(+), 27 deletions(-)
>>
>> diff --git a/fs/coredump.c b/fs/coredump.c
>> index a57a06b80f57..19d3343b93c6 100644
>> --- a/fs/coredump.c
>> +++ b/fs/coredump.c
>> @@ -586,8 +586,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>> struct subprocess_info *sub_info;
>>
>> if (ispipe < 0) {
>> -printk(KERN_WARNING "format_corename failed\n");
>> -printk(KERN_WARNING "Aborting core\n");
>> +coredump_report_failure("format_corename failed, aborting core");
>> goto fail_unlock;
>> }
>>
>> @@ -607,27 +606,21 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>> * right pid if a thread in a multi-threaded
>> * core_pattern process dies.
>> */
>> -printk(KERN_WARNING
>> -"Process %d(%s) has RLIMIT_CORE set to 1\n",
>> -task_tgid_vnr(current), current->comm);
>> -printk(KERN_WARNING "Aborting core\n");
>> +coredump_report_failure("RLIMIT_CORE is set to 1, aborting core");
>> goto fail_unlock;
>> }
>> cprm.limit = RLIM_INFINITY;
>>
>> dump_count = atomic_inc_return(&core_dump_count);
>> if (core_pipe_limit && (core_pipe_limit < dump_count)) {
>> -printk(KERN_WARNING "Pid %d(%s) over core_pipe_limit\n",
>> -      task_tgid_vnr(current), current->comm);
>> -printk(KERN_WARNING "Skipping core dump\n");
>> +coredump_report_failure("over core_pipe_limit, skipping core dump");
>> goto fail_dropcount;
>> }
>>
>> helper_argv = kmalloc_array(argc + 1, sizeof(*helper_argv),
>>    GFP_KERNEL);
>> if (!helper_argv) {
>> -printk(KERN_WARNING "%s failed to allocate memory\n",
>> -      __func__);
>> +coredump_report_failure("%s failed to allocate memory", __func__);
>> goto fail_dropcount;
>> }
>> for (argi = 0; argi < argc; argi++)
>> @@ -644,8 +637,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>>
>> kfree(helper_argv);
>> if (retval) {
>> -printk(KERN_INFO "Core dump to |%s pipe failed\n",
>> -      cn.corename);
>> +coredump_report_failure("|%s pipe failed", cn.corename);
>> goto close_fail;
>> }
>> } else {
>> @@ -658,10 +650,8 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>> goto fail_unlock;
>>
>> if (need_suid_safe && cn.corename[0] != '/') {
>> -printk(KERN_WARNING "Pid %d(%s) can only dump core "\
>> -"to fully qualified path!\n",
>> -task_tgid_vnr(current), current->comm);
>> -printk(KERN_WARNING "Skipping core dump\n");
>> +coredump_report_failure(
>> +"this process can only dump core to a fully qualified path, skipping 
>> core dump");
>> goto fail_unlock;
>> }
>>
>> @@ -730,13 +720,13 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>> idmap = file_mnt_idmap(cprm.file);
>> if (!vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, inode),
>>    current_fsuid())) {
>> -pr_info_ratelimited("Core dump to %s aborted: cannot preserve file 
>> owner\n",
>> -   cn.corename);
>> +coredump_report_failure("Core dump to %s aborted: "
>> +"cannot preserve file owner", cn.corename);
>> goto close_fail;
>> }
>> if ((inode->i_mode & 0677) != 0600) {
>> -pr_info_ratelimited("Core dump to %s aborted: cannot preserve file 
>> permissions\n",
>> -   cn.corename);
>> +coredump_report_failure("Core dump to %s aborted: "
>> +"cannot preserve file permissions", cn.corename);
>> goto close_fail;
>> }
>> if (!(cprm.file->f_mode & FMODE_CAN_WRITE))
>> @@ -757,7 +747,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>> * have this set to NULL.
>> */
>> if (!cprm.file) {
>> -pr_info("Core dump to |%s disabled\n", cn.corename);
>> +coredump_report_failure("Core dump to |%s disabled", cn.corename);
>> goto close_fail;
>> }
>> if (!dump_vma_snapshot(&cprm))
>> @@ -983,11 +973,10 @@ void validate_coredump_safety(void)
>> {
>> if (suid_dumpable == SUID_DUMP_ROOT &&
>>    core_pattern[0] != '/' && core_pattern[0] != '|') {
>> -pr_warn(
>> -"Unsafe core_pattern used with fs.suid_dumpable=2.\n"
>> -"Pipe handler or fully qualified core dump path required.\n"
>> -"Set kernel.core_pattern before fs.suid_dumpable.\n"
>> -);
>> +
>> +coredump_report_failure("Unsafe core_pattern used with 
>> fs.suid_dumpable=2: "
>> +"pipe handler or fully qualified core dump path required. "
>> +"Set kernel.core_pattern before fs.suid_dumpable.");
>> }
>> }
>>
>> diff --git a/include/linux/coredump.h b/include/linux/coredump.h
>> index 0904ba010341..45e598fe3476 100644
>> --- a/include/linux/coredump.h
>> +++ b/include/linux/coredump.h
>> @@ -43,8 +43,30 @@ 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);
>> +
>> +/*
>> + * Logging for the coredump code, ratelimited.
>> + * The TGID and comm fields are added to the message.
>> + */
>> +
>> +#define __COREDUMP_PRINTK(Level, Format, ...) \
>> +do {\
>> +char comm[TASK_COMM_LEN];\
>> +\
>> +get_task_comm(comm, current);\
>> +printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n",\
>> +task_tgid_vnr(current), (int)strlen(comm), comm, ##__VA_ARGS__);\
>> +} while (0)\
>> +
>> +#define coredump_report(fmt, ...) __COREDUMP_PRINTK(KERN_INFO, fmt, 
>> ##__VA_ARGS__)
>> +#define coredump_report_failure(fmt, ...) 
>> __COREDUMP_PRINTK(KERN_WARNING, fmt, ##__VA_ARGS__)
>> +
>> #else
>> static inline void do_coredump(const kernel_siginfo_t *siginfo) {}
>> +
>> +#define coredump_report(...)
>> +#define coredump_report_failure(...)
>> +
>> #endif
>>
>> #if defined(CONFIG_COREDUMP) && defined(CONFIG_SYSCTL)
>> -- 
>> 2.45.2
> 

-- 
Thank you,
Roman

Re: [PATCH v3 0/2] binfmt_elf, coredump: Log the reason of the failed core dumps

[Kees Cook]

On Thu, 18 Jul 2024 11:27:23 -0700, Roman Kisel wrote:
> 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.
> 
> [...]

Applied to for-next/execve, thanks!

[1/2] coredump: Standartize and fix logging
      https://git.kernel.org/kees/c/c114e9948c2b
[2/2] binfmt_elf, coredump: Log the reason of the failed core dumps
      https://git.kernel.org/kees/c/fb97d2eb542f

Take care,

-- 
Kees Cook