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 2BDF0C433EF for ; Wed, 9 Feb 2022 07:31:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AE4FC6B0074; Wed, 9 Feb 2022 02:31:18 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A93A76B0075; Wed, 9 Feb 2022 02:31:18 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 90E8A6B007D; Wed, 9 Feb 2022 02:31:18 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0103.hostedemail.com [216.40.44.103]) by kanga.kvack.org (Postfix) with ESMTP id 80B166B0074 for ; Wed, 9 Feb 2022 02:31:18 -0500 (EST) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 319D58249980 for ; Wed, 9 Feb 2022 07:31:18 +0000 (UTC) X-FDA: 79122420636.16.67E4FB1 Received: from mail-ed1-f45.google.com (mail-ed1-f45.google.com [209.85.208.45]) by imf09.hostedemail.com (Postfix) with ESMTP id 945D7140007; Wed, 9 Feb 2022 07:31:17 +0000 (UTC) Received: by mail-ed1-f45.google.com with SMTP id m11so3183584edi.13; Tue, 08 Feb 2022 23:31:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=LV46nVT6ZyUm4CkAW3e5eCUPfVZ1+2GfU1mW0n/r+DU=; b=H69Zufbut5euCO/mhsNgnHpm5HF95kNT8FFxBZPneQ36ZWcmxcqtAf0tovWvk5UY84 8b6UpfZbNy3Gu5IHyK/TYgsAU1nzGxbSUHiA7EffiG+jw9meOn8M6ORG29l/5wJF4ifi GtIkq8IwUXA7SUFwyEsA/z9ZR44S1uCNfPCx/VU5T6Q/9THAZjOJKTm1yo8j60i7oEGG e8O/Z0BvHhQ26ExvMPnywoN4pZ6qPEqD1RcnnIY8Wuc13XlpahSe/apkDCqC3Ws5uASI yW2xN5gTADriFkzeFBjhBzZ5k1owEfBxOby84JyMnqThuX68uU1pFxcv02y9L4H0h/+I o6ew== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to; bh=LV46nVT6ZyUm4CkAW3e5eCUPfVZ1+2GfU1mW0n/r+DU=; b=jRyvsQoNw0UO8W2eZIzCtuLOHWVuVx52KZmYQYF3sS45psoNx1ihayw+6Uk1FMO1tF CelV3zO6CTzU2k+jAOLXfgwkgfZKab6TlpczAkoizjc55I35y0M41ZKKPqnn9RCRmUsj AVCLyMy+4U3zjhMpcwOmyFuO73ZmWw7XznlsxlzgkWaTuh+RfDPfXuy3JlxOPUhSk33X 3mM7WzsohJh2j2XXmtL2zm2VjwIVnWpdgRI5jbw4XjmopR22hkiXfab9BDr8BbSqXIv8 l0yvFmFgzenip5DC9uXFJrER5afCqjKRhg0sSO0SYBpnv6M0dhM2ANxMHkw3rKx+5mYr UHLQ== X-Gm-Message-State: AOAM530mNTzSozfgWx47SWbT2seePvj1pR+bBbkMDRjuEUDDWFQxqqud IhGi3qQyN8Nd5EvKlaNX7kw= X-Google-Smtp-Source: ABdhPJwxxXeD0s8j9xGnQpug+hmPsXoxK3Md0tSKC+zT37eaK5mRNiORfFe11V+D/e4B71FM7EzZUw== X-Received: by 2002:a05:6402:35d4:: with SMTP id z20mr1072349edc.13.1644391876048; Tue, 08 Feb 2022 23:31:16 -0800 (PST) Received: from dumbo ([2a0b:f4c2:1::1]) by smtp.gmail.com with ESMTPSA id ak15sm3300369ejc.73.2022.02.08.23.31.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Feb 2022 23:31:15 -0800 (PST) Received: from cavok by dumbo with local (Exim 4.94.2) (envelope-from ) id 1nHhRZ-00051Y-B3; Wed, 09 Feb 2022 08:31:13 +0100 Date: Wed, 9 Feb 2022 08:31:13 +0100 From: Domenico Andreoli To: Luis Chamberlain Cc: Tong Zhang , "Eric W. Biederman" , David Airlie , Andrew Morton , amir73il@gmail.com, Andy Shevchenko , Arnd Bergmann , bcrl@kvack.org, benh@kernel.crashing.org, clemens@ladisch.de, crope@iki.fi, dgilbert@interlog.com, Greg KH , jack@suse.cz, jani.nikula@intel.com, jani.nikula@linux.intel.com, "James E.J. Bottomley" , jlbec@evilplan.org, john.ogness@linutronix.de, joonas.lahtinen@linux.intel.com, Joseph Qi , julia.lawall@inria.fr, Kees Cook , kernel@tuxforce.de, Linux Memory Management List , mark@fasheh.com, "Martin K. Petersen" , mm-commits@vger.kernel.org, nixiaoming@huawei.com, penguin-kernel@i-love.sakura.ne.jp, peterz@infradead.org, phil@philpotter.co.uk, pjt@google.com, pmladek@suse.com, rafael@kernel.org, rodrigo.vivi@intel.com, rostedt@goodmis.org, senozhatsky@chromium.org, sre@kernel.org, steve@sk2.org, surenb@google.com, torvalds@linux-foundation.org, tytso@mit.edu, Al Viro , wangqing@vivo.com, Iurii Zaikin , open list Subject: Re: [PATCH] Fix regression due to "fs: move binfmt_misc sysctl to its own file" Message-ID: References: <20220121221021.60533b009c357d660791476e@linux-foundation.org> <20220122061228.nmuo75sDn%akpm@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 945D7140007 X-Stat-Signature: zigjkfs9jt3yeqge74dkh7ks8j4kg9ts X-Rspam-User: Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=H69Zufbu; spf=pass (imf09.hostedemail.com: domain of domenico.andreoli.it@gmail.com designates 209.85.208.45 as permitted sender) smtp.mailfrom=domenico.andreoli.it@gmail.com; dmarc=none X-Rspamd-Server: rspam05 X-HE-Tag: 1644391877-399951 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: On Tue, Feb 08, 2022 at 09:20:42AM -0800, Luis Chamberlain wrote: > On Mon, Feb 07, 2022 at 02:53:21PM -0800, Tong Zhang wrote: > > On Mon, Feb 7, 2022 at 1:46 PM Luis Chamberlain wrote: > > > > > > OK I think the issue here should have been that declaring this on > > > fs/binfmt_misc.c creates the chicken and the egg issue, and so we > > > need to do this on built-in code. Instead of your patch can you try > > > this instead, it just always creates the sysctl mount always now > > > so long as proc sysctl is enabled. It does not do the unregistration > > > as we always want the path present as it used to be before as well. > > > > > > diff --git a/fs/file_table.c b/fs/file_table.c > > > index 57edef16dce4..4f4739c9405c 100644 > > > --- a/fs/file_table.c > > > +++ b/fs/file_table.c > > > @@ -119,6 +119,7 @@ static struct ctl_table fs_stat_sysctls[] = { > > > static int __init init_fs_stat_sysctls(void) > > > { > > > register_sysctl_init("fs", fs_stat_sysctls); > > > + register_sysctl_mount_point("fs/binfmt_misc"); > > > return 0; > > > } > > > fs_initcall(init_fs_stat_sysctls); > > > > I'm looking at the original code, and it seems that if we don't select > > CONFIG_BINFMT_MISC at all, > > this file won't be created. This would suggest an IF MACRO around > > > + register_sysctl_mount_point("fs/binfmt_misc"); > > and it should looks like > > +#if IS_ENABLED(CONFIG_BINFMT_MISC) > > + register_sysctl_mount_point("fs/binfmt_misc"); > > +#endif > > or if you prefer original style: > > #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE) > > Or better yet using IS_ENABLED() to avoid the ifdef cruft: > > diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c > index c07f35719ee3..4b8f1b11a7c8 100644 > --- a/fs/binfmt_misc.c > +++ b/fs/binfmt_misc.c > @@ -817,20 +817,16 @@ static struct file_system_type bm_fs_type = { > }; > MODULE_ALIAS_FS("binfmt_misc"); > > -static struct ctl_table_header *binfmt_misc_header; > - > static int __init init_misc_binfmt(void) > { > int err = register_filesystem(&bm_fs_type); > if (!err) > insert_binfmt(&misc_format); > - binfmt_misc_header = register_sysctl_mount_point("fs/binfmt_misc"); > return 0; > } > > static void __exit exit_misc_binfmt(void) > { > - unregister_sysctl_table(binfmt_misc_header); > unregister_binfmt(&misc_format); > unregister_filesystem(&bm_fs_type); > } > diff --git a/fs/file_table.c b/fs/file_table.c > index 57edef16dce4..4969021fa676 100644 > --- a/fs/file_table.c > +++ b/fs/file_table.c > @@ -119,6 +119,8 @@ static struct ctl_table fs_stat_sysctls[] = { > static int __init init_fs_stat_sysctls(void) > { > register_sysctl_init("fs", fs_stat_sysctls); > + if (IS_ENABLED(CONFIG_BINFMT_MISC)) > + register_sysctl_mount_point("fs/binfmt_misc"); > return 0; > } > fs_initcall(init_fs_stat_sysctls); Apologies for the late reply, I was notified the patch is added to the mm patchset and thought to be late for any update but now I'm not seeing it anywhere. Maybe it's been dropped? Forgot of IS_ENABLED(), I would surely use it in the v2. Not clear what's the benefit in registering the mount point in fs/file_table.c vs. kernel/sysctl.c. I'd keep it close to where people was used to find it, unless there is a good reason otherwise. Could you please elaborate? Thanks for your review! Dom -- rsa4096: 3B10 0CA1 8674 ACBA B4FE FCD2 CE5B CF17 9960 DE13 ed25519: FFB4 0CC3 7F2E 091D F7DA 356E CC79 2832 ED38 CB05