Hi Christian!
Am Fr., 9. Mai 2025 um 12:25 Uhr schrieb Christian Brauner <brauner@xxxxxxxxxx>:
>
> 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@xxxxxxxxxx>
> ---
> fs/coredump.c | 41 ++++++++++++++++++++++++-----------------
> 1 file changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/fs/coredump.c b/fs/coredump.c
> index d740a0411266..281320ea351f 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))
Shouldn't it be:
if (cn->core_type != COREDUMP_PIPE)
Except this, LGTM
Reviewed-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@xxxxxxxxxxxxx>
> 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
>
