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 0EC54C4345F for ; Fri, 3 May 2024 23:32:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 84ABE6B0087; Fri, 3 May 2024 19:32:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7FAEC6B0088; Fri, 3 May 2024 19:32:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6C2676B0089; Fri, 3 May 2024 19:32:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 52A4F6B0087 for ; Fri, 3 May 2024 19:32:00 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id AE742161273 for ; Fri, 3 May 2024 23:31:59 +0000 (UTC) X-FDA: 82078684758.10.A386156 Received: from mail-pg1-f174.google.com (mail-pg1-f174.google.com [209.85.215.174]) by imf13.hostedemail.com (Postfix) with ESMTP id C781020011 for ; Fri, 3 May 2024 23:31:57 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=HytYz6Wm; dmarc=pass (policy=none) header.from=chromium.org; spf=pass (imf13.hostedemail.com: domain of keescook@chromium.org designates 209.85.215.174 as permitted sender) smtp.mailfrom=keescook@chromium.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1714779117; 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=ZQWkXaFuCvDccNEejO0sjEajyPVeed872Xyd5w++i9E=; b=REv9WMD60r/ZoWC2yuepkWI3SSptaTYgxwGkop228sDUU3nwUm1QsOAT0qgveIiCM/OWrq cFPqAEzK/2N9r0BNUPa0EP+09SIzqnMrOA+VunPFnfY0QE/R6/tx8mRIka5+SWRhYi4AsR IaU2AavADgKIwn6QWnJNjyThosANRto= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1714779117; a=rsa-sha256; cv=none; b=jvMErSQPuMqt/7ies1KGnKYPvAMoFZc3gMcPTbQO8ftrZdV37r7F9MvOEJkXXwVoy2WLtO iB5kSX/wG8BPAJDGjFkSHEwrQLKoLyFoV55cIRQvhexQ+Oj0tMuQZ2Vs/8TXMNYM0DE/1V ajdDWpuX2JWZdO90JDPIJcIQ/PzbPnM= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=chromium.org header.s=google header.b=HytYz6Wm; dmarc=pass (policy=none) header.from=chromium.org; spf=pass (imf13.hostedemail.com: domain of keescook@chromium.org designates 209.85.215.174 as permitted sender) smtp.mailfrom=keescook@chromium.org Received: by mail-pg1-f174.google.com with SMTP id 41be03b00d2f7-5d42e7ab8a9so108220a12.3 for ; Fri, 03 May 2024 16:31:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1714779116; x=1715383916; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=ZQWkXaFuCvDccNEejO0sjEajyPVeed872Xyd5w++i9E=; b=HytYz6WmIbdH8gcTSjlNBm3gETa8qKrzxmEd1Tnf+DPDVuc18c9xo6y45lFx1cHzc2 ZlVYDBWok+7UjAZwoBI6d64ccxYSdDHvGtuoh9LGJrE/EztmqklpomGjRbA6X7WTynvq wTvMpipPsCRzLElcGaUk7rifjw9gCxCzx0UzQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714779116; x=1715383916; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ZQWkXaFuCvDccNEejO0sjEajyPVeed872Xyd5w++i9E=; b=abFHgtIUR2OnOk8tgfao4B5WN2+LOEQaIE+VGik8+4eeifulVpk2vubgGdJ6wMmbNp huPpi3Qsf5qVl2FG50RBQrZtv4j0YIu2akYluhek5qcTdY/oSXOjk+sWnLQ42m2+mnyj F38Txn3Sdq4+1bUtYzvnQi+FEWV16lZEfYz9mVNfUJ0DyJj2z9RHbEhtNrDABCxL5QoO bJ5OWIxJnZOhifpTOUtuS6vDEAtMHY9yYcjXncGAV7g3WyE+SYkJVtFvtVdLjNXnVIH6 sIknwBIuh9w5zplbMARPs0ZvZ+l0nMTnke4lT2w+U9mvaZmT0IZIMbyMpzmLZtu1jo0n VBhA== X-Forwarded-Encrypted: i=1; AJvYcCXCS34/syi1Scdi8mDWHswR6NsaqJW/U/raWLmSXPJFUR1ZfvWYndhcS9a4JhiKzk3RcxSH/YFRX8h2WRkpCPs2NWI= X-Gm-Message-State: AOJu0YwyNu0iZBFl063SDlc/TsW9lcZXfy7kkA/AW017MfPCfPjPe74k YEhJ6WVdj2QRwkvmlU9FQG239C2VUpUJvCu31XjnC2eJUvt9GRPjfml18bfCgA== X-Google-Smtp-Source: AGHT+IF+MTBM20asd8P4jmLte2hDSW6ZEMf3snsAauJ6scuYiqSJW3M/akt8KGKIOSjQ8ZRPpzsPNw== X-Received: by 2002:a17:90b:19c1:b0:2b1:535f:c3dc with SMTP id nm1-20020a17090b19c100b002b1535fc3dcmr4598762pjb.26.1714779116508; Fri, 03 May 2024 16:31:56 -0700 (PDT) Received: from www.outflux.net ([198.0.35.241]) by smtp.gmail.com with ESMTPSA id q6-20020a17090a938600b002b273cbbdf1sm3667938pjo.49.2024.05.03.16.31.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 May 2024 16:31:55 -0700 (PDT) Date: Fri, 3 May 2024 16:31:55 -0700 From: Kees Cook To: Allen 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 Subject: Re: [PATCH v3] fs/coredump: Enable dynamic configuration of max file note size Message-ID: <202405031629.D95BE0F@keescook> References: <20240502235603.19290-1-apais@linux.microsoft.com> <202405021743.D06C96516@keescook> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: C781020011 X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: xdcmejxiqekmjszrtnyyhh7uc7p4cydj X-HE-Tag: 1714779117-618475 X-HE-Meta: U2FsdGVkX1/eunDtRKzjqMYrwLLoYVvWsuMmQSeO3N/hzdkM5F9UMhw3ldUKlHbthi+J+7NxcXdTj7GEaue9rP/Q0MePYzMqgM/mH466S8yE4JcMIhC8D6GskPAs7c/MvGBI4M1+PqA5D3mbXzmsRublTSbq4vdFk5Eo/tz1eBENYFkxOrarQqYy/VF9Rm6edjScwvn0/lYYn2w9x+g6xkP5R/2rkEugHMsJNqxYy8VqXMwgKQLa7Q6rHf9wwhHmjLbbyBEaQz0g7pG2dM/1ZwoFL1agDsxRj1e6CKgmVJg/Xp6LzDQseHKwPzZbEXm6w5YHKw+YHZrpPLjaR7FDPI/Dk+Q1eKUgvf5CkDLxCS0Pg78NwxorzuoD3kU52Q/vxxrcZW1xrTacswZumhK29oGewdwtHREonNackopSCeawJT0+HBvdjtTEJ//qmlojnEsOcuLnhSxLeAPIdEv54Gyi1wUAITaRioFrrTO2t1+xlDePgccxdIToMB9GmP4zwXqbgN8HPGajt4r3nRZFJGxGzkK+whEpRdvPWas316gxuZYw2hdPL8TRqwZUW4AXnsYKicQeLMslknif0f69tk3orku2Lz9RonOYuanLR5mkSlUfbAtK+WZ2rZhKilSKEEeqMD0FYWc8yaIvutn2O+4iug/37F3VxOULMLBa9uzKWLEyKA6iOkUFBX9eXj8/gqWCEM3V8WJ3dhxp3ILT0XV0ZuMHDhlXjm0z+7XjOI0uwqpvIuBAWXFx2Lh0Fuuo3xdHZRF3jVDRtTEUXkHWBzuGIOmqijSmpMUE0Pe22EWZZxbAVBmGdXBxiCcNeCP5cOu0PK2Sf3bviwmM17jeZnMXLSKU6uM2dMN9Xk1inTYl/t0N0gKzFlg/A5jEpH3A6UOza1zEVe1m+WOQL9tQV7r3MEv3tu/s5u6Do4Kdge3B6JnB5nUpISg6/uibwh5leoGYmUQlctEfsrEk4Ot Ct5CZuC/ om+S4kOcvWUTUkUwKIkqEcZwTmNDv0oX3PB9sEc5jQKIbZztG1vjNc3bAWPxglCNXMeVhufK5OwvbckSugcwTPMwzXH8Teg3a9Wouc6UAtT+NhQlI/jwaVEmRRnhAEOAs4Qze3u9LLTjdTkBQo4RX2wowmxGNFQLCOlt3dI5v0gsovKve3JBlOPKY2ERRWD8Fgcm4CcWWXOrX+4/4sDMVrt96z6J1BonunOjXp4pS68VCtKN/CpFQYP6HdT1GAdp28oSBt3nEq9Wmx8Rea8bRDG/s9IGBw1II2nMEC/gnjOZ39RlSGs1o5noCT0s9h4POJfHTHzzpiOEP3Jh5QiyEQhzOIh5HR8psbepug8ypYsmm/39+acIc5p/fLi61YjbdzDwVZtc/uExgeaGMEzuXBVF/maOKtrEDF+o5BqPgJe8PCQ+9zv15NengvSkgtzWD9Oqw 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: 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 > > > > 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 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