* [PATCH v2] debugfs: Only clobber mode/uid/gid on remount if asked
@ 2022-09-09 0:13 Brian Norris
2022-09-09 8:49 ` Greg Kroah-Hartman
0 siblings, 1 reply; 2+ messages in thread
From: Brian Norris @ 2022-09-09 0:13 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J . Wysocki
Cc: linux-kernel, Steven Rostedt, Brian Norris
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)
{
struct debugfs_fs_info *fsi = sb->s_fs_info;
struct inode *inode = d_inode(sb->s_root);
struct debugfs_mount_opts *opts = &fsi->mount_opts;
- inode->i_mode &= ~S_IALLUGO;
- inode->i_mode |= opts->mode;
+ /*
+ * On remount, only reset mode/uid/gid if they were provided as mount
+ * options.
+ */
+
+ if (!remount || opts->opts & BIT(Opt_mode)) {
+ inode->i_mode &= ~S_IALLUGO;
+ inode->i_mode |= opts->mode;
+ }
+
+ if (!remount || opts->opts & BIT(Opt_uid))
+ inode->i_uid = opts->uid;
- inode->i_uid = opts->uid;
- inode->i_gid = opts->gid;
+ if (!remount || opts->opts & BIT(Opt_gid))
+ inode->i_gid = opts->gid;
return 0;
}
@@ -175,7 +190,7 @@ static int debugfs_remount(struct super_block *sb, int *flags, char *data)
if (err)
goto fail;
- debugfs_apply_options(sb);
+ debugfs_apply_options(sb, true);
fail:
return err;
@@ -257,7 +272,7 @@ static int debug_fill_super(struct super_block *sb, void *data, int silent)
sb->s_op = &debugfs_super_operations;
sb->s_d_op = &debugfs_dops;
- debugfs_apply_options(sb);
+ debugfs_apply_options(sb, false);
return 0;
--
2.37.2.789.g6183377224-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] debugfs: Only clobber mode/uid/gid on remount if asked
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
0 siblings, 0 replies; 2+ messages in thread
From: Greg Kroah-Hartman @ 2022-09-09 8:49 UTC (permalink / raw)
To: Brian Norris; +Cc: Rafael J . Wysocki, linux-kernel, Steven Rostedt
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-09-09 8:49 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 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).