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 ADD76C04FFE for ; Fri, 3 May 2024 01:10:15 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3EDA66B009E; Thu, 2 May 2024 21:10:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 39D656B009F; Thu, 2 May 2024 21:10:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 23E6A6B00A0; Thu, 2 May 2024 21:10:15 -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 0646B6B009E for ; Thu, 2 May 2024 21:10:15 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id B31031A02D9 for ; Fri, 3 May 2024 01:10:14 +0000 (UTC) X-FDA: 82075303548.08.AD990B9 Received: from mail-ua1-f53.google.com (mail-ua1-f53.google.com [209.85.222.53]) by imf11.hostedemail.com (Postfix) with ESMTP id 06FFC40002 for ; Fri, 3 May 2024 01:10:12 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=jcwaBLRy; spf=pass (imf11.hostedemail.com: domain of allen.lkml@gmail.com designates 209.85.222.53 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=1714698613; 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=jcivoYXjhNV3GkTXLRXF43T/wLWOG6MyPhRAIhBIkA4=; b=DvLqs/H3z4jjSF1m7biM193kYAFLFHDun9jCcwfmxIMEWcFgIJ+V84409VuW7d4qYhvRy0 4gCHTWJfYzgyd95svg//VlWD5vf+VGo2gKY5WtN8izBsc2dpnizR6/7XUoh4LuG9pzbmaG Wlv7DaBvIO3WIMApwwrqFDq0b1ugntY= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=jcwaBLRy; spf=pass (imf11.hostedemail.com: domain of allen.lkml@gmail.com designates 209.85.222.53 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=1714698613; a=rsa-sha256; cv=none; b=7minYo7aSDV6B0oS3SyNo84Cn3bE7RVCtM+PTnCUbjnvh/IIZWAU90u1WEXFnmKAXrZ6ty bAWg5aulSxlX3o45jSvaIJDXg2nqjUM2kLEcqeaVL3kOeMz7jVBD30EXb3fRnO7jGS9a8E MAhF8DD27F5TTo0UgFrTnpvMMcqfkZQ= Received: by mail-ua1-f53.google.com with SMTP id a1e0cc1a2514c-7f3317ff3c2so730658241.0 for ; Thu, 02 May 2024 18:10:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1714698612; x=1715303412; 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=jcivoYXjhNV3GkTXLRXF43T/wLWOG6MyPhRAIhBIkA4=; b=jcwaBLRyd25ZW32Ym0umlwXFGRPRDBCH4eUlMLFnI37jk3cCEA29CdtgeqzGPXeYI8 sN3x2gU30AkCf4DK4kWhguztM7ZPyW5NSFA/i56SwHLGrmDAGqaONvjm5A2Y1gqnWr4e HqkvCWkdxhr4VncHhpaN2LkTwkOOxTNNw/SlZj9Z97LuPcs9tEeHGXGC1hwxdZcILlhr Pq4HUgfUhS9Fp/xGhTJuA2EKkCgMglV+UldtRI8HixbyVHDTIWfpmuUpBulZ1UDHqsqv rtT3GnWN0z16Bk1frpujmXoGIGC/LG97aHEdAjFouDTRM13zJ0WR5dzzGhGMLznVuCl3 B2SA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714698612; x=1715303412; 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=jcivoYXjhNV3GkTXLRXF43T/wLWOG6MyPhRAIhBIkA4=; b=nyVOIkV7kZgVfJo554DoGVDYrnWOFUTW/kwmMHtN+T3G8rSpaROVY3eiG5CLqTIIHE X6yQJY4etDQA1+LWx4bRUF2cFjMedQPmfyRyD2549NBmVtLNVsKGVZ05BcG9CanWFBH4 orWtlevcQmpfXc0+mbOi2nR/vlk4iV5uY4kWOi2F6rrgkGqzad8jvTc86cPcLARyglh8 RobJdrb3abs//XeZ1qYL/b+UwJKjBzCM9/7l2buw6iUMeZoUFJojDLIYGlQcPYo0Nrm9 QMMntA0kxf87LPCqmOALlK4JCHZI1BxsX4juC8LnlcmlKhHg7cE9w17B/wOMNEdhX4JG mvhA== X-Forwarded-Encrypted: i=1; AJvYcCVcJeMDUF3DbjVjB1KAwLO22x1jM6aNh/9b6mbz8QnI6WVcA0Zd3joUubgJf91vqZ9FvEH5R4A4Sale3eG0cUSDFiI= X-Gm-Message-State: AOJu0Yx3hoem/5MokCkV0M+hmjDvLx6TBEBgSpy+7t+TlnaMe+F7AqDc QgSjG7/528RNdfU1NgoerrnDnfAPBfQwuYstUDWYWf5sdXZSIl8UG8ehHHdl63yivD/zIvvEjUY XWdAzBQ22PQBgu0ZjGHvimxaqfB4= X-Google-Smtp-Source: AGHT+IGQuUbN3qIwl8Y0J9GY/pX9vDj5/9GMce0s9P+xLlRsaNlRD6am8i0nfaOWGEBADNEEsGR+8lUuXCazrT+nr6U= X-Received: by 2002:a05:6122:546:b0:4df:1ef7:ac92 with SMTP id y6-20020a056122054600b004df1ef7ac92mr1457417vko.6.1714698611929; Thu, 02 May 2024 18:10:11 -0700 (PDT) MIME-Version: 1.0 References: <20240502235603.19290-1-apais@linux.microsoft.com> <202405021743.D06C96516@keescook> In-Reply-To: <202405021743.D06C96516@keescook> From: Allen Date: Thu, 2 May 2024 18:10:00 -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-Rspam-User: X-Rspamd-Queue-Id: 06FFC40002 X-Rspamd-Server: rspam06 X-Stat-Signature: 71n63wy4ezfyy6eoh7e8ad5uy14u6cho X-HE-Tag: 1714698612-145540 X-HE-Meta: U2FsdGVkX1+NGxW0MM/ij1KadEz/6/gAf5mR8eWAIKuRj33KD6B7K+WdNlKeTgPq4Uhn6jrVs5DdjhH02V9VUsYj53pXogrwDotewAR+wQLGyzkM71OFrUlMcMtPYzeyGDY28N+HTaGVBrN/eKUgWtixcxXJgJejMUMGcj/kR8YUYhsNw80+KSuLFFkEDu24IELGovXMfB+Ep+dqgvmrmxSzTw46eBix7RZ4GgRVc3PLsei3AAaqzaydkQAEyrRxbJoxsSHgU0J2ameQYwX1vQxzRARHyNYsnXca/GP5fhIIUFvBuTm6eSWymhIWCBjrvy2FX6KWRD1rKPZiOlNlHn6Lxn/KzFjsvz+UgyNXAoX/oYAByeOpX1VNqUbdvz9Lxy2e1j6TZfjoywNDfIgf7BPBK8D5j5oAYUp1DMk+zD0hNWdegQyY/w2A+fvVy6NzinbhYehghu7dwgCJOKVUcVkCyj0XVuUfm0hhHxdmKeA3/0IWBqnWHKEOonWrrgxruJFkDRInjfFTt1+6428QNSCrzjC+YH/wcW0iziIbysFoVSdqZ7Z2bbe6MYYH915q9Mq/epu18GUhtiWf9X5tPuqRR/NynkfwB5Hvm3sX6YPZmKdlbVvPqYG9VafmSSl3sh+IOUCSDIVn8y8EI+lkUa60MsYhYWW7OURP/fI7N6sbeK10McyfcEHkF7oC1mZFyYuKvpoh4XfB5keWZr9vPjWAavy6LKTVd8dUTF3yViXao0rfyGtP+8oM7rjzPhym+pvpfXxkemRNCSd4PVlatvMdw4D00qoypn7VlsA4Z3LrXHFnIzs72OG/G6xMoItrRW3Cvcpbrqna3NK2rPfAPqCUU15ZSXRf+gsOl2Czo8VFjaYAVNKLRlFxmWrW2qCWfLJVE0QyT4olH8GOQpznVrFA7gaGtaI8QhM9PEkBxrJkenKjQ3EVc1YoLgpyZ9s8RzoxJzEWJnllRg14xAn WZDe+EYU z2WNqMhPSemQShFVjD2Nc7V9GEEQ6QUulgZwu9+DLU0rXwHNSYj5pA7ete5CDkMh7Y0AKmMJiCi2QPwlZNjZ1iMdtNUgYB/Z9t1n7tpksiXbWU3n+UwSJOs1TVsAExTiIzfk13ktz+/C4nWbLDHG6oA5UQ+qh0GChXdVux4GP45UdoCnU8IiESvEDR7d7Qmu4pIvSpjjFrrEnVxpdzdVafPgRKRpqUvjLszMqZUBx2+2dzWoxIGTMJa3IhZJGKM5gozNxUA6l1rmyZgT6YBgX3ZvINOhUBFHMOif2NVXC6ua4aoGBvJ7fYtW2upvdOAqeQ6a+sEAOLwZM6RDB5lRiz9BiXnz75g3QbgWS11IbVMg0g91FJRSvx1NBDXj9seIyAUjtbzCAnfsiwUpHw7357VtHtQ== 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;?? > > > > 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