Re: [PATCH v7 1/9] coredump: massage format_corname()

["Serge E. Hallyn" <serge@hallyn.com>]

On Thu, May 15, 2025 at 12:03:34AM +0200, Christian Brauner wrote:
> We're going to extend the coredump code in follow-up patches.
> Clean it up so we can do this more easily.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Not my wheelhouse, but this is a nice cleanup.

Acked-by: Serge Hallyn <serge@hallyn.com>

> ---
>  fs/coredump.c | 41 ++++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/coredump.c b/fs/coredump.c
> index d740a0411266..368751d98781 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -76,9 +76,15 @@ static char core_pattern[CORENAME_MAX_SIZE] = "core";
>  static int core_name_size = CORENAME_MAX_SIZE;
>  unsigned int core_file_note_size_limit = CORE_FILE_NOTE_SIZE_DEFAULT;
>  
> +enum coredump_type_t {
> +	COREDUMP_FILE = 1,
> +	COREDUMP_PIPE = 2,
> +};
> +
>  struct core_name {
>  	char *corename;
>  	int used, size;
> +	enum coredump_type_t core_type;
>  };
>  
>  static int expand_corename(struct core_name *cn, int size)
> @@ -218,18 +224,21 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm,
>  {
>  	const struct cred *cred = current_cred();
>  	const char *pat_ptr = core_pattern;
> -	int ispipe = (*pat_ptr == '|');
>  	bool was_space = false;
>  	int pid_in_pattern = 0;
>  	int err = 0;
>  
>  	cn->used = 0;
>  	cn->corename = NULL;
> +	if (*pat_ptr == '|')
> +		cn->core_type = COREDUMP_PIPE;
> +	else
> +		cn->core_type = COREDUMP_FILE;
>  	if (expand_corename(cn, core_name_size))
>  		return -ENOMEM;
>  	cn->corename[0] = '\0';
>  
> -	if (ispipe) {
> +	if (cn->core_type == COREDUMP_PIPE) {
>  		int argvs = sizeof(core_pattern) / 2;
>  		(*argv) = kmalloc_array(argvs, sizeof(**argv), GFP_KERNEL);
>  		if (!(*argv))
> @@ -247,7 +256,7 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm,
>  		 * Split on spaces before doing template expansion so that
>  		 * %e and %E don't get split if they have spaces in them
>  		 */
> -		if (ispipe) {
> +		if (cn->core_type == COREDUMP_PIPE) {
>  			if (isspace(*pat_ptr)) {
>  				if (cn->used != 0)
>  					was_space = true;
> @@ -353,7 +362,7 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm,
>  				 * Installing a pidfd only makes sense if
>  				 * we actually spawn a usermode helper.
>  				 */
> -				if (!ispipe)
> +				if (cn->core_type != COREDUMP_PIPE)
>  					break;
>  
>  				/*
> @@ -384,12 +393,12 @@ static int format_corename(struct core_name *cn, struct coredump_params *cprm,
>  	 * If core_pattern does not include a %p (as is the default)
>  	 * and core_uses_pid is set, then .%pid will be appended to
>  	 * the filename. Do not do this for piped commands. */
> -	if (!ispipe && !pid_in_pattern && core_uses_pid) {
> +	if (!(cn->core_type == COREDUMP_PIPE) && !pid_in_pattern && core_uses_pid) {
>  		err = cn_printf(cn, ".%d", task_tgid_vnr(current));
>  		if (err)
>  			return err;
>  	}
> -	return ispipe;
> +	return 0;
>  }
>  
>  static int zap_process(struct signal_struct *signal, int exit_code)
> @@ -583,7 +592,6 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  	const struct cred *old_cred;
>  	struct cred *cred;
>  	int retval = 0;
> -	int ispipe;
>  	size_t *argv = NULL;
>  	int argc = 0;
>  	/* require nonrelative corefile path and be extra careful */
> @@ -632,19 +640,18 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  
>  	old_cred = override_creds(cred);
>  
> -	ispipe = format_corename(&cn, &cprm, &argv, &argc);
> +	retval = format_corename(&cn, &cprm, &argv, &argc);
> +	if (retval < 0) {
> +		coredump_report_failure("format_corename failed, aborting core");
> +		goto fail_unlock;
> +	}
>  
> -	if (ispipe) {
> +	if (cn.core_type == COREDUMP_PIPE) {
>  		int argi;
>  		int dump_count;
>  		char **helper_argv;
>  		struct subprocess_info *sub_info;
>  
> -		if (ispipe < 0) {
> -			coredump_report_failure("format_corename failed, aborting core");
> -			goto fail_unlock;
> -		}
> -
>  		if (cprm.limit == 1) {
>  			/* See umh_coredump_setup() which sets RLIMIT_CORE = 1.
>  			 *
> @@ -695,7 +702,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  			coredump_report_failure("|%s pipe failed", cn.corename);
>  			goto close_fail;
>  		}
> -	} else {
> +	} else if (cn.core_type == COREDUMP_FILE) {
>  		struct mnt_idmap *idmap;
>  		struct inode *inode;
>  		int open_flags = O_CREAT | O_WRONLY | O_NOFOLLOW |
> @@ -823,13 +830,13 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  		file_end_write(cprm.file);
>  		free_vma_snapshot(&cprm);
>  	}
> -	if (ispipe && core_pipe_limit)
> +	if ((cn.core_type == COREDUMP_PIPE) && core_pipe_limit)
>  		wait_for_dump_helpers(cprm.file);
>  close_fail:
>  	if (cprm.file)
>  		filp_close(cprm.file, NULL);
>  fail_dropcount:
> -	if (ispipe)
> +	if (cn.core_type == COREDUMP_PIPE)
>  		atomic_dec(&core_dump_count);
>  fail_unlock:
>  	kfree(argv);
> 
> -- 
> 2.47.2
>