From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 29289C4345F for ; Fri, 3 May 2024 01:41:17 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6EC726B0082; Thu, 2 May 2024 21:41:16 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 675396B00A0; Thu, 2 May 2024 21:41:16 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4C7AF6B00A1; Thu, 2 May 2024 21:41:16 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 28AAE6B0082 for ; Thu, 2 May 2024 21:41:16 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id C54FFC0973 for ; Fri, 3 May 2024 01:41:15 +0000 (UTC) X-FDA: 82075381710.17.7E1FFA8 Received: from mail-vk1-f182.google.com (mail-vk1-f182.google.com [209.85.221.182]) by imf26.hostedemail.com (Postfix) with ESMTP id 14D82140007 for ; Fri, 3 May 2024 01:41:13 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=EqoHloMU; spf=pass (imf26.hostedemail.com: domain of allen.lkml@gmail.com designates 209.85.221.182 as permitted sender) smtp.mailfrom=allen.lkml@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714700474; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=VlRJGegZy6w5LS1pjceiS9E4YpXz8vFlDYvpW3ED23Q=; b=d2j8TxilkYDF4ds98iUR39ORaoXQUHspW9gc/uHMCN+I2dqs9aLSHi+AzLthkcMpJRhzCi 9wxig51igbBeQQ9Tu/Bhp7XCYwhLSH9zeg9AXARQzZPCtT3kqcd5Mfr3giE7gTugD3o9x4 3L0rCD1xuXI7zfVMG3AcEy4jALN66WI= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=EqoHloMU; spf=pass (imf26.hostedemail.com: domain of allen.lkml@gmail.com designates 209.85.221.182 as permitted sender) smtp.mailfrom=allen.lkml@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714700474; a=rsa-sha256; cv=none; b=Lly6sC6JI5xSgCwIh1Ri2nVg1Pug5lJYxBbykuAbOwWU7rdCO/SnYFICQtgN60EvLoc3CN asTZkBJOtV7FOZNUsCwyGAYjAjCmrCH3Me6D/B2OgOo/mAkGeKiAFMqRfS8fGYRW8Ouibe mv6NcUk10bOg+YrmFsuudUhhQ2eP/oY= Received: by mail-vk1-f182.google.com with SMTP id 71dfb90a1353d-4df0bf2db8fso645517e0c.1 for ; Thu, 02 May 2024 18:41:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714700473; x=1715305273; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=VlRJGegZy6w5LS1pjceiS9E4YpXz8vFlDYvpW3ED23Q=; b=EqoHloMUdmwNlt8DwdtPAZfR33kYyTaPuV7qWGSobws8cum/lPhvgFfLmrO31b51Ay V62AULSWU0FOzgXe7A2eRAxboIpdogDKzSj6xjdCbE+nZrQiWZXkw34XQ15jF7jnmZCv mbsi5IuRrUvt2RgnuAzWfv/mx9x0oR+eRZLmcxIaf+WPBGHf7ixSJbYPrrH0mNB/fvYf 1APiMJ/TUuAmksWKFFidOISMxgaU+VBbOoyt4nrsKtyWn3CTrbdLXDzcOrU266Tsk+ix xaXrHmbmgy1V9H8jnU4FTfWDLD4qzk3aJa6fEi1WdImU6R5hrdf32eMnx2nl614P46hd pndw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714700473; x=1715305273; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=VlRJGegZy6w5LS1pjceiS9E4YpXz8vFlDYvpW3ED23Q=; b=moL7CuPJ4OU5jq2x60ogE5prHrz5fvUnqfnwNcOVfcN3wkkXyxt1j2X9vOsCoCb8ca OAX5o3Duqe8hSj5HDDIg2kP+2m/7Y4CWuQTKdCVSQIDlucure7L66qYoyrW1aqAv/z0k yD1r/Q0SWe4N8fEYKn3DO3G8JB8IWFnSeqtVmYD1lj7IHs8E5ukGX6usgjCWi+MccuUv /HB07IvTFh6hXg0/LWi8UX88t9DxxrsTw+U4lkCc9MDRp9BsHVMHqRvPbksMvLFcHlfl juMcEMhjvLKy3d8E7qqTWWkIOyCOkjM2mt3UlGXVjz56+CBvXuIb1bWIh1c/BCTWnPBM sf4Q== X-Forwarded-Encrypted: i=1; AJvYcCUXOFdXD5psJ55nQfc3VNoj986RAh64qGo3JahPfL6cFf4hduSFxb6wbcjJ014EMRs/YrEnqCxGDd7lJ/RBfVk/158= X-Gm-Message-State: AOJu0YyTp4FZz1LskNKntu9TfdY/6PB3gdK/gQKEqU8YXVuWC6Kh/PUy CIiWXnT3u0YiP2uHGI5osubzDQu/ZUgnZ0a32lR1zpeHG3SFF5+ZgerWLl8Ax9Fcyc/dWm0007y UQf5skLgmB2l+cLHMKp80ElZNz+I= X-Google-Smtp-Source: AGHT+IFBeWjdtaBlerRRX5wmmVtg2RU/hEPFQQBEd4Yx452buoaBo5IsvlOtb3rX30HdP6R0GMkt8aPyuEQNfNrCIUw= X-Received: by 2002:a05:6122:99d:b0:4d3:398f:8633 with SMTP id g29-20020a056122099d00b004d3398f8633mr1485097vkd.10.1714700471440; Thu, 02 May 2024 18:41:11 -0700 (PDT) MIME-Version: 1.0 References: <20240502235603.19290-1-apais@linux.microsoft.com> <202405021743.D06C96516@keescook> In-Reply-To: From: Allen Date: Thu, 2 May 2024 18:40:58 -0700 Message-ID: Subject: Re: [PATCH v3] fs/coredump: Enable dynamic configuration of max file note size To: Kees Cook Cc: Allen Pais , 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" X-Stat-Signature: ik3b89ba4txqp4ax4qetdmi41pnpb17h X-Rspam-User: X-Rspamd-Queue-Id: 14D82140007 X-Rspamd-Server: rspam05 X-HE-Tag: 1714700473-364142 X-HE-Meta: U2FsdGVkX19G4Cxd3FqOky/Bz/IwdvUsXHx35UBY8Lb99UGjYHEeMlzuFqZEAE1TZh+Ti/K1/hpCJe7zBObECHQ8NxxzU6qriFAXx/owIc26UxVuDosCl7uMAo0cvVfVLqv13ZelBJLLiQeaDXZu59TqcBKX+Mr1WOC4Hn1TsHb8Dinh464aGijZeNEMa9gDk3cDtJYaADQZQVa7RCwgFi0LKmLPscEFYdbxWGTNPCVbkrlYtDm14659XvOeThDWfpeDAu1KvtJbqxbbfFdzm7jpO59zjUOAcDugmuyValtQbvzMDyT97psPb6NXremLT6GUBBYCJ+5Fy5Lh4JRK+ptdgG+ycICADwYjR12Nao6fC4Lyg5PdBOyKZyJNnnpru8XtQ+uYYLqN0lKkJ0zwkCbUgXO9VqY0LL6U3sKWkuguhc1kN3m761zlDB3L7F+Wi4pd0KVLFBWolNt3ZRWFSmvyBg1f/OIomt4RE8mS2NtjtAg5yJfa06DWSjHuoLLxOxNsi7FP4QKFaYguw4FnTqq8Tc1RQYlTLTSvme/8DMQ5kZ/tNUl1dWb9QsU1mbNVcmJmxR7k9oJT4zYHPx5Z7P5RNlQu7kOpwjQCmlgfbefTDeo85QF3ZejNuG/RH6SQq9VuMSACKgRF+0qOAmbkLomMJnKKcfjhlM4/fM2Khei8vEqNwcI6OuB+TYsCF+HuCm4IH/25WGXvTOBM+beKSi8rzKbXQ0bbKQdZDDu2580wwpnd00oiLJByNslh6rpzvYY7tJUQ+QjNP+pcmGyfl1P/W0EPz7HL6I1Cz80i2YILLi2JSZiqf6he+CXhb8WZuCBkbJgxaSjjXjam4wSy/ohBFrdr4dkQCoZWnoQgXlNsGP4tjyMzJ1SnQ0nv8H+rAx9afI/XenadlUCzozFcejwkt00nvO+LnNOkTNxh46Cqyi573ntwfyF/Ydn56S8NGjZnnT8PuLzpygs/8cN IEZ0Ov8P zcYplsxOhSjdWmKGo+Kyt4rfCipGUSldafQOjd3qjkjpX1K+lU+ZxGAXcViqOfIQv9HQWObJrAh1JvTF837MlcdVPAE2VaaEMgKBheSw0MhWry1JHAjg9ol+UUM2LNseUNMrqV9B2w27eCuazND9yKWvkej9On8ocGkGl72q4MuNv/nsmDhHEUh0/PfqEI24KZrde0QNUMNte55WCVEKo5T9FCv5PXhM2UfhJE+o7hvFaWpnKV4AvBVF8xCHM0INve0/vnfj8EFhEie58dG28rDx87Qrw2GU/TfMwYspW3OmZLd/kqUR/xhQ3EofVtiYHkAi83RFKzjkRab0f8bBInlrvolk41XE2spqO/fVPi527o53K1+B+BAfjaaBAksaQTtdGCtca6hZrFwSJUPNrfKWNCA== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: > > > 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 > > > Signed-off-by: Allen Pais > > > > > > --- > > > 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 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. diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 5397b552fbeb..19bd85d1e42e 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,11 @@ 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_allowed) { + pr_warn_once("coredump Note size too large: %u (does kernel.core_file_note_size_min sysctl need adjustment?\n", 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..69085bb494dc 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 CORE_FILE_NOTE_SIZE_DEFAULT (4*1024*1024) +/* Define a reasonable max cap */ +#define CORE_FILE_NOTE_SIZE_MAX (16*1024*1024) + 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; +static unsigned int core_file_note_size_min = CORE_FILE_NOTE_SIZE_DEFAULT; +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_min", + .data = &core_file_note_size_min, + .maxlen = sizeof(unsigned int), + .mode = 0644, + .proc_handler = proc_douintvec_minmax, + .extra1 = &core_file_note_size_min, + .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..776bde5f9752 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_allowed; extern void validate_coredump_safety(void); #else static inline void validate_coredump_safety(void) {} Thanks, Allen > > > 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 -- - Allen