Received-SPF: pass (google.com: domain of linux-kernel+bounces-167132-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240502235603.19290-1-apais@linux.microsoft.com> <202405021743.D06C96516@keescook>
In-Reply-To: <202405021743.D06C96516@keescook>
From: Allen <allen.lkml@gmail.com>
Date: Thu, 2 May 2024 18:10:00 -0700
Message-ID: <CAOMdWSLa-dp34aq3RepQABpnGs-TnyQgUxFm--MHVHFuVYTgFg@mail.gmail.com>
Subject: Re: [PATCH v3] fs/coredump: Enable dynamic configuration of max file
 note size
To: Kees Cook <keescook@chromium.org>
Cc: Allen Pais <apais@linux.microsoft.com>, linux-fsdevel@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-mm@kvack.org, viro@zeniv.linux.org.uk, 
	brauner@kernel.org, jack@suse.cz, ebiederm@xmission.com, mcgrof@kernel.org, 
	j.granados@samsung.com
Content-Type: text/plain; charset="UTF-8"

> > Introduce the capability to dynamically configure the maximum file
> > note size for ELF core dumps via sysctl. This enhancement removes
> > the previous static limit of 4MB, allowing system administrators to
> > adjust the size based on system-specific requirements or constraints.
> >
> > - Remove hardcoded `MAX_FILE_NOTE_SIZE` from `fs/binfmt_elf.c`.
> > - Define `max_file_note_size` in `fs/coredump.c` with an initial value
> >   set to 4MB.
> > - Declare `max_file_note_size` as an external variable in
> >   `include/linux/coredump.h`.
> > - Add a new sysctl entry in `kernel/sysctl.c` to manage this setting
> >   at runtime.
>
> The above bullet points should be clear from the patch itself. The
> commit is really more about rationale and examples (which you have
> below). I'd remove the bullets.

Sure, I have it modified to:

fs/coredump: Enable dynamic configuration of max file note size

    Introduce the capability to dynamically configure the maximum file
    note size for ELF core dumps via sysctl.

    Why is this being done?
    We have observed that during a crash when there are more than 65k mmaps
    in memory, the existing fixed limit on the size of the ELF notes section
    becomes a bottleneck. The notes section quickly reaches its capacity,
    leading to incomplete memory segment information in the resulting coredump.
    This truncation compromises the utility of the coredumps, as crucial
    information about the memory state at the time of the crash might be
    omitted.

    This enhancement removes the previous static limit of 4MB, allowing
    system administrators to adjust the size based on system-specific
    requirements or constraints.

    Eg:
    $ sysctl -a | grep core_file_note_size_max
    kernel.core_file_note_size_max = 4194304
......
>
> >
> > $ sysctl -a | grep core_file_note_size_max
> > kernel.core_file_note_size_max = 4194304
> >
> > $ sysctl -n kernel.core_file_note_size_max
> > 4194304
> >
> > $echo 519304 > /proc/sys/kernel/core_file_note_size_max
> >
> > $sysctl -n kernel.core_file_note_size_max
> > 519304
> >
> > Attempting to write beyond the ceiling value of 16MB
> > $echo 17194304 > /proc/sys/kernel/core_file_note_size_max
> > bash: echo: write error: Invalid argument
> >
> > Why is this being done?
> > We have observed that during a crash when there are more than 65k mmaps
> > in memory, the existing fixed limit on the size of the ELF notes section
> > becomes a bottleneck. The notes section quickly reaches its capacity,
> > leading to incomplete memory segment information in the resulting coredump.
> > This truncation compromises the utility of the coredumps, as crucial
> > information about the memory state at the time of the crash might be
> > omitted.
>
> I'd make this the first paragraph of the commit log. "We have this
> problem" goes first, then "Here's what we did to deal with it", then you
> examples. :)
>
 Done.

> >
> > Signed-off-by: Vijay Nag <nagvijay@microsoft.com>
> > Signed-off-by: Allen Pais <apais@linux.microsoft.com>
> >
> > ---
> > Chagnes in v3:
> >    - Fix commit message to reflect the correct sysctl knob [Kees]
> >    - Add a ceiling for maximum pssible note size(16M) [Allen]
> >    - Add a pr_warn_once() [Kees]
> > Changes in v2:
> >    - Move new sysctl to fs/coredump.c [Luis & Kees]
> >    - rename max_file_note_size to core_file_note_size_max [kees]
> >    - Capture "why this is being done?" int he commit message [Luis & Kees]
> > ---
> >  fs/binfmt_elf.c          |  8 ++++++--
> >  fs/coredump.c            | 15 +++++++++++++++
> >  include/linux/coredump.h |  1 +
> >  3 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 5397b552fbeb..5294f8f3a9a8 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -1564,7 +1564,6 @@ static void fill_siginfo_note(struct memelfnote *note, user_siginfo_t *csigdata,
> >       fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata);
> >  }
> >
> > -#define MAX_FILE_NOTE_SIZE (4*1024*1024)
> >  /*
> >   * Format of NT_FILE note:
> >   *
> > @@ -1592,8 +1591,13 @@ static int fill_files_note(struct memelfnote *note, struct coredump_params *cprm
> >
> >       names_ofs = (2 + 3 * count) * sizeof(data[0]);
> >   alloc:
> > -     if (size >= MAX_FILE_NOTE_SIZE) /* paranoia check */
> > +     /* paranoia check */
> > +     if (size >= core_file_note_size_max) {
> > +             pr_warn_once("coredump Note size too large: %u "
> > +             "(does kernel.core_file_note_size_max sysctl need adjustment?)\n",
>
> The string can be on a single line (I think scripts/check_patch.pl will
> warn about this, as well as the indentation of "size" below...

 It does warn, but if I leave it as a single line, there's still a warning:
WARNING: line length of 135 exceeds 100 columns, which is why I
split it into multiple lines.

>
> > +             size);
> >               return -EINVAL;
> > +     }
> >       size = round_up(size, PAGE_SIZE);
> >       /*
> >        * "size" can be 0 here legitimately.
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index be6403b4b14b..ffaed8c1b3b0 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -56,10 +56,16 @@
> >  static bool dump_vma_snapshot(struct coredump_params *cprm);
> >  static void free_vma_snapshot(struct coredump_params *cprm);
> >
> > +#define MAX_FILE_NOTE_SIZE (4*1024*1024)
> > +/* Define a reasonable max cap */
> > +#define MAX_ALLOWED_NOTE_SIZE (16*1024*1024)
>
> Let's call this CORE_FILE_NOTE_SIZE_DEFAULT and
> CORE_FILE_NOTE_SIZE_MAX to match the sysctl.
>

 Sure, will update it in v4.

> > +
> >  static int core_uses_pid;
> >  static unsigned int core_pipe_limit;
> >  static char core_pattern[CORENAME_MAX_SIZE] = "core";
> >  static int core_name_size = CORENAME_MAX_SIZE;
> > +unsigned int core_file_note_size_max = MAX_FILE_NOTE_SIZE;
> > +unsigned int core_file_note_size_allowed = MAX_ALLOWED_NOTE_SIZE;
>
> The latter can be static and const.
>
> For the note below, perhaps add:
>
> static const unsigned int core_file_note_size_min = CORE_FILE_NOTE_SIZE_DEFAULT;
>

 core_file_note_size_min will be used in fs/binfmt_elf.c at:

    if (size >= core_file_note_size_min) ,
did you mean
static const unsigned int core_file_note_size_allowed =
CORE_FILE_NOTE_SIZE_MAX;??
> >
> >  struct core_name {
> >       char *corename;
> > @@ -1020,6 +1026,15 @@ static struct ctl_table coredump_sysctls[] = {
> >               .mode           = 0644,
> >               .proc_handler   = proc_dointvec,
> >       },
> > +     {
> > +             .procname       = "core_file_note_size_max",
> > +             .data           = &core_file_note_size_max,
> > +             .maxlen         = sizeof(unsigned int),
> > +             .mode           = 0644,
> > +             .proc_handler   = proc_douintvec_minmax,
> > +             .extra1         = &core_file_note_size_max,
>
> This means you can never shrink it if you raise it from the default.
> Let's use the core_file_note_size_min above.

Sure, will fix it in v4.
>
> > +             .extra2         = &core_file_note_size_allowed,
> > +     },
> >  };
> >
> >  static int __init init_fs_coredump_sysctls(void)
> > diff --git a/include/linux/coredump.h b/include/linux/coredump.h
> > index d3eba4360150..14c057643e7f 100644
> > --- a/include/linux/coredump.h
> > +++ b/include/linux/coredump.h
> > @@ -46,6 +46,7 @@ static inline void do_coredump(const kernel_siginfo_t *siginfo) {}
> >  #endif
> >
> >  #if defined(CONFIG_COREDUMP) && defined(CONFIG_SYSCTL)
> > +extern unsigned int core_file_note_size_max;
> >  extern void validate_coredump_safety(void);
> >  #else
> >  static inline void validate_coredump_safety(void) {}
> > --
> > 2.17.1
> >
>
> I think v4 will be all good to go, assuming no one else pops up. :)
> Thanks for the changes!

Thank you for the reviews. Will send out v4 soon.

-- 
       - Allen