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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2024-06-17 16:41:30 [-0700], Roman Kisel wrote:
> 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.

How much of this happened and how much of this is just "let me handle
everything that could go wrong".
The cases where it was interrupted without a hint probably deserve a
note rather then leaving a half of coredump back.

> Signed-off-by: Roman Kisel <romank@linux.microsoft.com>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index a57a06b80f57..a7200c9024c6 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -777,9 +807,18 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  		}
>  		file_end_write(cprm.file);
>  		free_vma_snapshot(&cprm);
> +	} else {
> +		pr_err("Core dump to |%s has been interrupted\n", cn.corename);
> +		retval = -EAGAIN;
> +		goto fail;
>  	}
> +	pr_info("Core dump to |%s: vma_count %d, vma_data_size %lu, written %lld bytes, pos %lld\n",
> +		cn.corename, cprm.vma_count, cprm.vma_data_size, cprm.written, cprm.pos);

Probably too noisy in the default case. The offsets probably don't
matter unless you debug.

>  	if (ispipe && core_pipe_limit)
>  		wait_for_dump_helpers(cprm.file);
> +
> +	retval = 0;
> +
>  close_fail:
>  	if (cprm.file)
>  		filp_close(cprm.file, NULL);
> diff --git a/include/linux/coredump.h b/include/linux/coredump.h
> index 0904ba010341..8b29be758a87 100644
> --- a/include/linux/coredump.h
> +++ b/include/linux/coredump.h
> @@ -42,9 +42,9 @@ 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) {}

This probably does not compile.

>  #endif
>  
>  #if defined(CONFIG_COREDUMP) && defined(CONFIG_SYSCTL)
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 1f9dd41c04be..f2ecf29a994d 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2675,6 +2675,7 @@ bool get_signal(struct ksignal *ksig)
>  	struct sighand_struct *sighand = current->sighand;
>  	struct signal_struct *signal = current->signal;
>  	int signr;
> +	int ret;
>  
>  	clear_notify_signal();
>  	if (unlikely(task_work_pending(current)))
> @@ -2891,7 +2892,9 @@ 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, error %d\n", ret);

So you preserve the error code just for one additional note.

>  		}
>  
>  		/*

Sebastian