From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Brian Norris <briannorris@chromium.org>
Cc: "Rafael J . Wysocki" <rafael@kernel.org>,
linux-kernel@vger.kernel.org,
Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH v2] debugfs: Only clobber mode/uid/gid on remount if asked
Date: Fri, 9 Sep 2022 10:49:28 +0200 [thread overview]
Message-ID: <Yxr+GFUnLVaoiSCn@kroah.com> (raw)
In-Reply-To: <20220908171319.v2.1.Icbd40fce59f55ad74b80e5d435ea233579348a78@changeid>
On Thu, Sep 08, 2022 at 05:13:22PM -0700, Brian Norris wrote:
> Users may have explicitly configured their debugfs permissions; we
> shouldn't overwrite those just because a second mount appeared.
>
> Only clobber if the options were provided at mount time.
>
> Existing behavior:
>
> ## Pre-existing status: debugfs is 0755.
> # chmod 755 /sys/kernel/debug/
> # stat -c '%A' /sys/kernel/debug/
> drwxr-xr-x
>
> ## New mount sets kernel-default permissions:
> # mount -t debugfs none /mnt/foo
> # stat -c '%A' /mnt/foo
> drwx------
>
> ## Unexpected: the original mount changed permissions:
> # stat -c '%A' /sys/kernel/debug
> drwx------
>
> New behavior:
>
> ## Pre-existing status: debugfs is 0755.
> # chmod 755 /sys/kernel/debug/
> # stat -c '%A' /sys/kernel/debug/
> drwxr-xr-x
>
> ## New mount inherits existing permissions:
> # mount -t debugfs none /mnt/foo
> # stat -c '%A' /mnt/foo
> drwxr-xr-x
>
> ## Expected: old mount is unchanged:
> # stat -c '%A' /sys/kernel/debug
> drwxr-xr-x
>
> Full test cases are being submitted to LTP.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> Temporary LTP branch here:
> https://github.com/computersforpeace/ltp/commits/debugfs
>
> I'll put in an LTP pull request once the kernel tree actually contains
> my patches.
>
> Companion patch:
> tracefs: Only clobber mode/uid/gid on remount if asked
> https://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git/commit/?h=trace-v6.0-rc4&id=47311db8e8f33011d90dee76b39c8886120cdda4
>
> Changes in v2:
> * Update commit description
> * Include LTP patch in notes
> * Split from tracefs patch
>
> fs/debugfs/inode.c | 29 ++++++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 3dcf0b8b4e93..1e36ce013631 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -82,6 +82,8 @@ struct debugfs_mount_opts {
> kuid_t uid;
> kgid_t gid;
> umode_t mode;
> + /* Opt_* bitfield. */
> + unsigned int opts;
> };
>
> enum {
> @@ -111,6 +113,7 @@ static int debugfs_parse_options(char *data, struct debugfs_mount_opts *opts)
> kgid_t gid;
> char *p;
>
> + opts->opts = 0;
> opts->mode = DEBUGFS_DEFAULT_MODE;
>
> while ((p = strsep(&data, ",")) != NULL) {
> @@ -145,22 +148,34 @@ static int debugfs_parse_options(char *data, struct debugfs_mount_opts *opts)
> * but traditionally debugfs has ignored all mount options
> */
> }
> +
> + opts->opts |= BIT(token);
> }
>
> return 0;
> }
>
> -static int debugfs_apply_options(struct super_block *sb)
> +static int debugfs_apply_options(struct super_block *sb, bool remount)
Functions with random boolean arguments are impossible to understand and
maintain over time.
Every time you run across a call to this function now, you have to go
back and look up what this specific argument means, which is a pain.
Make two functions:
debugfs_apply_options()
debugfs_apply_options_remount()
and then move this original function to be:
_debugfs_apply_options()
or some such name.
Then when you call the function, you know exactly what is happening.
thanks,
greg k-h
prev parent reply other threads:[~2022-09-09 8:49 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-09 0:13 [PATCH v2] debugfs: Only clobber mode/uid/gid on remount if asked Brian Norris
2022-09-09 8:49 ` Greg Kroah-Hartman [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Yxr+GFUnLVaoiSCn@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=briannorris@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).