On Thu, May 02, 2024 at 06:40:58PM -0700, Allen wrote:
> > > > 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@xxxxxxxxxxxxx>
> > > > Signed-off-by: Allen Pais <apais@xxxxxxxxxxxxxxxxxxx>
> > > >
> > > > ---
> > > > 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;??
> > > >
>
> Kees,
>
> My bad, I misunderstood what you asked for. Here is the final diff,
> if it looks fine,
> i can send out a v4.
>
> Note, there is a warning issued by checkpatch.pl (WARNING: line length
> of 134 exceeds 100 columns)
For strings that should be fine. You'll want the ", size);" part on the
next line though.
> for the pr_warn_once() and adding const trigger a build
> warning(warning: initialization discards
> 'const' qualifier from pointer target type), which is why i dropped it.
Yeah, that's a common pattern for sysctl. You can fix it with a cast.
For example:
static const unsigned long nlm_grace_period_min = 0;
static const unsigned long nlm_grace_period_max = 240;
...
.proc_handler = proc_doulongvec_minmax,
.extra1 = (unsigned long *) &nlm_grace_period_min,
.extra2 = (unsigned long *) &nlm_grace_period_max,
But yeah, looks good.
--
Kees Cook
