| Message ID | 20250205213931.74614-2-sandeen@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs: last of the pseudofs mount api conversions | expand |
On February 5, 2025 1:34:29 PM PST, Eric Sandeen <sandeen@redhat.com> wrote: >Convert the pstore filesystem to the new mount API. Thanks for doing this! > [...] >+static const struct fs_parameter_spec pstore_param_spec[] = { >+ fsparam_u32 ("kmsg_bytes", Opt_kmsg_bytes), >+ {} I am reminded to change the global to u32 (ulong is way too big), and fix the pstore_set_kmsg_bytes() prototype to be u32 not int. :) > [...] >+ /* pstore has historically ignored invalid kmsg_bytes param */ >+ if (opt < 0) >+ return 0; Seems like a warning would be at least user-friendly. ;) I can add that later. > [...] >@@ -431,19 +434,33 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent) > return -ENOMEM; > > scoped_guard(mutex, &pstore_sb_lock) >- pstore_sb = sb; >+ pstore_sb = sb; Shouldn't scoped_guard() induce a indent? > > pstore_get_records(0); > > return 0; > } > >-static struct dentry *pstore_mount(struct file_system_type *fs_type, >- int flags, const char *dev_name, void *data) >+static int pstore_get_tree(struct fs_context *fc) >+{ >+ if (fc->root) >+ return pstore_reconfigure(fc); I need to double check that changing kmsg_size out from under an active pstore won't cause problems, but it's probably okay. (Honestly I've been wanting to deprecate it as a mount option -- it really should just be a module param, but that's a separate task.) Reviewed-by: Kees Cook <kees@kernel.org> (Is it easier to take this via fs or via pstore?) -Kees
On 2/5/25 8:04 PM, Kees Cook wrote: >> @@ -431,19 +434,33 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent) >> return -ENOMEM; >> >> scoped_guard(mutex, &pstore_sb_lock) >> - pstore_sb = sb; >> + pstore_sb = sb; > Shouldn't scoped_guard() induce a indent? Whoops, not sure how that happened, sorry. Fix on commit or send V2? >> pstore_get_records(0); >> >> return 0; >> } >> >> -static struct dentry *pstore_mount(struct file_system_type *fs_type, >> - int flags, const char *dev_name, void *data) >> +static int pstore_get_tree(struct fs_context *fc) >> +{ >> + if (fc->root) >> + return pstore_reconfigure(fc); > I need to double check that changing kmsg_size out from under an active pstore won't cause problems, but it's probably okay. (Honestly I've been wanting to deprecate it as a mount option -- it really should just be a module param, but that's a separate task.) Honestly I struggled with this for a while, not quite sure what's right. > Reviewed-by: Kees Cook <kees@kernel.org> Thanks, -Eric > (Is it easier to take this via fs or via pstore?) > > -Kees
On February 5, 2025 6:19:06 PM PST, Eric Sandeen <sandeen@redhat.com> wrote: >On 2/5/25 8:04 PM, Kees Cook wrote: >>> @@ -431,19 +434,33 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent) >>> return -ENOMEM; >>> >>> scoped_guard(mutex, &pstore_sb_lock) >>> - pstore_sb = sb; >>> + pstore_sb = sb; >> Shouldn't scoped_guard() induce a indent? > >Whoops, not sure how that happened, sorry. > >Fix on commit or send V2? If I should carry it in pstore, a v2 is appreciated. If you're taking it via fs, fix on commit is fine by me. :) > >>> pstore_get_records(0); >>> >>> return 0; >>> } >>> >>> -static struct dentry *pstore_mount(struct file_system_type *fs_type, >>> - int flags, const char *dev_name, void *data) >>> +static int pstore_get_tree(struct fs_context *fc) >>> +{ >>> + if (fc->root) >>> + return pstore_reconfigure(fc); > >> I need to double check that changing kmsg_size out from under an active pstore won't cause problems, but it's probably okay. (Honestly I've been wanting to deprecate it as a mount option -- it really should just be a module param, but that's a separate task.) > >Honestly I struggled with this for a while, not quite sure what's right. I found the place I need to fix, but it's an existing problem and won't conflict. I'll get it fixed. -Kees > >> Reviewed-by: Kees Cook <kees@kernel.org> > >Thanks, >-Eric > >> (Is it easier to take this via fs or via pstore?) >> >> -Kees >
On Wed, Feb 05, 2025 at 08:19:06PM -0600, Eric Sandeen wrote: > On 2/5/25 8:04 PM, Kees Cook wrote: > >> @@ -431,19 +434,33 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent) > >> return -ENOMEM; > >> > >> scoped_guard(mutex, &pstore_sb_lock) > >> - pstore_sb = sb; > >> + pstore_sb = sb; > > Shouldn't scoped_guard() induce a indent? > > Whoops, not sure how that happened, sorry. > > Fix on commit or send V2? I fixed it.
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c index 56815799ce79..3a4582b79c75 100644 --- a/fs/pstore/inode.c +++ b/fs/pstore/inode.c @@ -14,10 +14,10 @@ #include <linux/init.h> #include <linux/list.h> #include <linux/string.h> -#include <linux/mount.h> #include <linux/seq_file.h> #include <linux/ramfs.h> -#include <linux/parser.h> +#include <linux/fs_parser.h> +#include <linux/fs_context.h> #include <linux/sched.h> #include <linux/magic.h> #include <linux/pstore.h> @@ -226,37 +226,38 @@ static struct inode *pstore_get_inode(struct super_block *sb) } enum { - Opt_kmsg_bytes, Opt_err + Opt_kmsg_bytes }; -static const match_table_t tokens = { - {Opt_kmsg_bytes, "kmsg_bytes=%u"}, - {Opt_err, NULL} +static const struct fs_parameter_spec pstore_param_spec[] = { + fsparam_u32 ("kmsg_bytes", Opt_kmsg_bytes), + {} }; -static void parse_options(char *options) -{ - char *p; - substring_t args[MAX_OPT_ARGS]; - int option; - - if (!options) - return; +struct pstore_context { + unsigned int kmsg_bytes; +}; - while ((p = strsep(&options, ",")) != NULL) { - int token; +static int pstore_parse_param(struct fs_context *fc, struct fs_parameter *param) +{ + struct pstore_context *ctx = fc->fs_private; + struct fs_parse_result result; + int opt; - if (!*p) - continue; + opt = fs_parse(fc, pstore_param_spec, param, &result); + /* pstore has historically ignored invalid kmsg_bytes param */ + if (opt < 0) + return 0; - token = match_token(p, tokens, args); - switch (token) { - case Opt_kmsg_bytes: - if (!match_int(&args[0], &option)) - pstore_set_kmsg_bytes(option); - break; - } + switch (opt) { + case Opt_kmsg_bytes: + ctx->kmsg_bytes = result.uint_32; + break; + default: + return -EINVAL; } + + return 0; } /* @@ -269,10 +270,12 @@ static int pstore_show_options(struct seq_file *m, struct dentry *root) return 0; } -static int pstore_remount(struct super_block *sb, int *flags, char *data) +static int pstore_reconfigure(struct fs_context *fc) { - sync_filesystem(sb); - parse_options(data); + struct pstore_context *ctx = fc->fs_private; + + sync_filesystem(fc->root->d_sb); + pstore_set_kmsg_bytes(ctx->kmsg_bytes); return 0; } @@ -281,7 +284,6 @@ static const struct super_operations pstore_ops = { .statfs = simple_statfs, .drop_inode = generic_delete_inode, .evict_inode = pstore_evict_inode, - .remount_fs = pstore_remount, .show_options = pstore_show_options, }; @@ -406,8 +408,9 @@ void pstore_get_records(int quiet) inode_unlock(d_inode(root)); } -static int pstore_fill_super(struct super_block *sb, void *data, int silent) +static int pstore_fill_super(struct super_block *sb, struct fs_context *fc) { + struct pstore_context *ctx = fc->fs_private; struct inode *inode; sb->s_maxbytes = MAX_LFS_FILESIZE; @@ -417,7 +420,7 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent) sb->s_op = &pstore_ops; sb->s_time_gran = 1; - parse_options(data); + pstore_set_kmsg_bytes(ctx->kmsg_bytes); inode = pstore_get_inode(sb); if (inode) { @@ -431,19 +434,33 @@ static int pstore_fill_super(struct super_block *sb, void *data, int silent) return -ENOMEM; scoped_guard(mutex, &pstore_sb_lock) - pstore_sb = sb; + pstore_sb = sb; pstore_get_records(0); return 0; } -static struct dentry *pstore_mount(struct file_system_type *fs_type, - int flags, const char *dev_name, void *data) +static int pstore_get_tree(struct fs_context *fc) +{ + if (fc->root) + return pstore_reconfigure(fc); + + return get_tree_single(fc, pstore_fill_super); +} + +static void pstore_free_fc(struct fs_context *fc) { - return mount_single(fs_type, flags, data, pstore_fill_super); + kfree(fc->fs_private); } +static const struct fs_context_operations pstore_context_ops = { + .parse_param = pstore_parse_param, + .get_tree = pstore_get_tree, + .reconfigure = pstore_reconfigure, + .free = pstore_free_fc, +}; + static void pstore_kill_sb(struct super_block *sb) { guard(mutex)(&pstore_sb_lock); @@ -456,11 +473,33 @@ static void pstore_kill_sb(struct super_block *sb) INIT_LIST_HEAD(&records_list); } +static int pstore_init_fs_context(struct fs_context *fc) +{ + struct pstore_context *ctx; + + ctx = kzalloc(sizeof(struct pstore_context), GFP_KERNEL); + if (!ctx) + return -ENOMEM; + + /* + * Global kmsg_bytes is initialized to default, and updated + * every time we (re)mount the single-sb filesystem with the + * option specified. + */ + ctx->kmsg_bytes = kmsg_bytes; + + fc->fs_private = ctx; + fc->ops = &pstore_context_ops; + + return 0; +} + static struct file_system_type pstore_fs_type = { .owner = THIS_MODULE, .name = "pstore", - .mount = pstore_mount, .kill_sb = pstore_kill_sb, + .init_fs_context = pstore_init_fs_context, + .parameters = pstore_param_spec, }; int __init pstore_init_fs(void)
Convert the pstore filesystem to the new mount API. Cc: Kees Cook <kees@kernel.org> Cc: Tony Luck <tony.luck@intel.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- fs/pstore/inode.c | 111 +++++++++++++++++++++++++++++++--------------- 1 file changed, 75 insertions(+), 36 deletions(-)