From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 878503FC0 for ; Tue, 24 Aug 2021 11:32:23 +0000 (UTC) Received: by mail.kernel.org (Postfix) with ESMTPSA id 970B6610F8; Tue, 24 Aug 2021 11:32:20 +0000 (UTC) Date: Tue, 24 Aug 2021 13:32:17 +0200 From: Christian Brauner To: Kari Argillander Cc: Konstantin Komarov , Christoph Hellwig , ntfs3@lists.linux.dev, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, Pali =?utf-8?B?Um9ow6Fy?= , Matthew Wilcox Subject: Re: [PATCH v2 3/6] fs/ntfs3: Use new api for mounting Message-ID: <20210824113217.ncuwc3zs452mdgec@wittgenstein> References: <20210819002633.689831-1-kari.argillander@gmail.com> <20210819002633.689831-4-kari.argillander@gmail.com> Precedence: bulk X-Mailing-List: ntfs3@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210819002633.689831-4-kari.argillander@gmail.com> On Thu, Aug 19, 2021 at 03:26:30AM +0300, Kari Argillander wrote: > We have now new mount api as described in Documentation/filesystems. We > should use it as it gives us some benefits which are desribed here > lore.kernel.org/linux-fsdevel/159646178122.1784947.11705396571718464082.stgit@warthog.procyon.org.uk/ > > Nls loading is changed a to load with string. This did make code also > little cleaner. > > Also try to use fsparam_flag_no as much as possible. This is just nice > little touch and is not mandatory but it should not make any harm. It > is just convenient that we can use example acl/noacl mount options. > > Signed-off-by: Kari Argillander > --- Looks mostly sane to me. Thanks! Acked-by: Christian Brauner > fs/ntfs3/ntfs_fs.h | 1 + > fs/ntfs3/super.c | 392 +++++++++++++++++++++++---------------------- > 2 files changed, 199 insertions(+), 194 deletions(-) > > diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h > index 0c3ac89c3115..1f07dd17c6c7 100644 > --- a/fs/ntfs3/ntfs_fs.h > +++ b/fs/ntfs3/ntfs_fs.h > @@ -50,6 +50,7 @@ > > struct ntfs_mount_options { > struct nls_table *nls; > + char *nls_name; > > kuid_t fs_uid; > kgid_t fs_gid; > diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c > index 702da1437cfd..39936a4ce831 100644 > --- a/fs/ntfs3/super.c > +++ b/fs/ntfs3/super.c > @@ -28,10 +28,11 @@ > #include > #include > #include > +#include > +#include > #include > #include > #include > -#include > #include > #include > > @@ -197,8 +198,32 @@ void *ntfs_put_shared(void *ptr) > return ret; > } > > +/* > + * Load nls table or if @nls is utf8 then return NULL because > + * nls=utf8 is totally broken. > + */ > +static struct nls_table *ntfs_load_nls(char *nls) > +{ > + struct nls_table *ret; > + > + if (!nls) > + nls = CONFIG_NLS_DEFAULT; > + > + if (strcmp(nls, "utf8")) > + return NULL; > + if (strcmp(nls, CONFIG_NLS_DEFAULT)) > + return load_nls_default(); > + > + ret = load_nls(nls); > + if (!ret) > + return ERR_PTR(-EINVAL); > + > + return ret; > +} > + > static inline void clear_mount_options(struct ntfs_mount_options *options) > { > + kfree(options->nls_name); > unload_nls(options->nls); > } > > @@ -221,202 +246,150 @@ enum Opt { > Opt_err, > }; > > -static const match_table_t ntfs_tokens = { > - { Opt_uid, "uid=%u" }, > - { Opt_gid, "gid=%u" }, > - { Opt_umask, "umask=%o" }, > - { Opt_dmask, "dmask=%o" }, > - { Opt_fmask, "fmask=%o" }, > - { Opt_immutable, "sys_immutable" }, > - { Opt_discard, "discard" }, > - { Opt_force, "force" }, > - { Opt_sparse, "sparse" }, > - { Opt_nohidden, "nohidden" }, > - { Opt_acl, "acl" }, > - { Opt_showmeta, "showmeta" }, > - { Opt_nls, "nls=%s" }, > - { Opt_prealloc, "prealloc" }, > - { Opt_no_acs_rules, "no_acs_rules" }, > - { Opt_err, NULL }, > +static const struct fs_parameter_spec ntfs_fs_parameters[] = { > + fsparam_u32("uid", Opt_uid), > + fsparam_u32("gid", Opt_gid), > + fsparam_u32oct("umask", Opt_umask), > + fsparam_u32oct("dmask", Opt_dmask), > + fsparam_u32oct("fmask", Opt_fmask), > + fsparam_flag_no("sys_immutable", Opt_immutable), > + fsparam_flag_no("discard", Opt_discard), > + fsparam_flag_no("force", Opt_force), > + fsparam_flag_no("sparse", Opt_sparse), > + fsparam_flag("nohidden", Opt_nohidden), > + fsparam_flag_no("acl", Opt_acl), > + fsparam_flag_no("showmeta", Opt_showmeta), > + fsparam_string("nls", Opt_nls), > + fsparam_flag_no("prealloc", Opt_prealloc), > + fsparam_flag("no_acs_rules", Opt_no_acs_rules), > + {} > }; > > -static noinline int ntfs_parse_options(struct super_block *sb, char *options, > - int silent, > - struct ntfs_mount_options *opts) > +static int ntfs_fs_parse_param(struct fs_context *fc, > + struct fs_parameter *param) > { > - char *p; > - substring_t args[MAX_OPT_ARGS]; > - int option; > - char nls_name[30]; > - struct nls_table *nls; > - > - opts->fs_uid = current_uid(); > - opts->fs_gid = current_gid(); > - opts->fs_fmask_inv = opts->fs_dmask_inv = ~current_umask(); > - nls_name[0] = 0; > - > - if (!options) > - goto out; > - > - while ((p = strsep(&options, ","))) { > - int token; > + struct ntfs_sb_info *sbi = fc->s_fs_info; > + struct ntfs_mount_options *opts = &sbi->options; > + struct fs_parse_result result; > + int opt; > > - if (!*p) > - continue; > + opt = fs_parse(fc, ntfs_fs_parameters, param, &result); > + if (opt < 0) > + return opt; > > - token = match_token(p, ntfs_tokens, args); > - switch (token) { > - case Opt_immutable: > - opts->sys_immutable = 1; > - break; > - case Opt_uid: > - if (match_int(&args[0], &option)) > - return -EINVAL; > - opts->fs_uid = make_kuid(current_user_ns(), option); > - if (!uid_valid(opts->fs_uid)) > - return -EINVAL; > - opts->uid = 1; > - break; > - case Opt_gid: > - if (match_int(&args[0], &option)) > - return -EINVAL; > - opts->fs_gid = make_kgid(current_user_ns(), option); > - if (!gid_valid(opts->fs_gid)) > - return -EINVAL; > - opts->gid = 1; > - break; > - case Opt_umask: > - if (match_octal(&args[0], &option)) > - return -EINVAL; > - opts->fs_fmask_inv = opts->fs_dmask_inv = ~option; > - opts->fmask = opts->dmask = 1; > - break; > - case Opt_dmask: > - if (match_octal(&args[0], &option)) > - return -EINVAL; > - opts->fs_dmask_inv = ~option; > - opts->dmask = 1; > - break; > - case Opt_fmask: > - if (match_octal(&args[0], &option)) > - return -EINVAL; > - opts->fs_fmask_inv = ~option; > - opts->fmask = 1; > - break; > - case Opt_discard: > - opts->discard = 1; > - break; > - case Opt_force: > - opts->force = 1; > - break; > - case Opt_sparse: > - opts->sparse = 1; > - break; > - case Opt_nohidden: > - opts->nohidden = 1; > - break; > - case Opt_acl: > + switch (opt) { > + case Opt_uid: > + opts->fs_uid = make_kuid(current_user_ns(), result.uint_32); > + if (!uid_valid(opts->fs_uid)) > + return -EINVAL; > + opts->uid = 1; If you're only using opts->uid to check whether ->fs_*id is set then you can probably simplify this. You're expressing the expectation that if ->fs_*id is set that it must be a valid *id. So you could initialize ->fs_*id to INVALID_*ID and then instead of having to check "if (opts->uid)" in later code you can directly check "if (valid_uid(opts->fs_uid))". Just a thought. > + break; > + case Opt_gid: > + opts->fs_gid = make_kgid(current_user_ns(), result.uint_32); > + if (!gid_valid(opts->fs_gid)) > + return -EINVAL; > + opts->gid = 1; > + break; > + case Opt_umask: > + opts->fs_fmask_inv = ~result.uint_32; > + opts->fs_dmask_inv = ~result.uint_32; > + opts->fmask = 1; > + opts->dmask = 1; > + break; > + case Opt_dmask: > + opts->fs_dmask_inv = ~result.uint_32; > + opts->dmask = 1; > + break; > + case Opt_fmask: > + opts->fs_fmask_inv = ~result.uint_32; > + opts->fmask = 1; > + break; > + case Opt_immutable: > + opts->sys_immutable = result.negated ? 0 : 1; > + break; > + case Opt_discard: > + opts->discard = result.negated ? 0 : 1; > + break; > + case Opt_force: > + opts->force = result.negated ? 0 : 1; > + break; > + case Opt_sparse: > + opts->sparse = result.negated ? 0 : 1; > + break; > + case Opt_nohidden: > + opts->nohidden = 1; > + break; > + case Opt_acl: > + if (!result.negated) > #ifdef CONFIG_NTFS3_FS_POSIX_ACL > - sb->s_flags |= SB_POSIXACL; > - break; > + fc->sb_flags |= SB_POSIXACL; > #else > - ntfs_err(sb, "support for ACL not compiled in!"); > - return -EINVAL; > + return invalf(fc, "ntfs3: Support for ACL not compiled in!"); > #endif > - case Opt_showmeta: > - opts->showmeta = 1; > - break; > - case Opt_nls: > - match_strlcpy(nls_name, &args[0], sizeof(nls_name)); > - break; > - case Opt_prealloc: > - opts->prealloc = 1; > - break; > - case Opt_no_acs_rules: > - opts->no_acs_rules = 1; > - break; > - default: > - if (!silent) > - ntfs_err( > - sb, > - "Unrecognized mount option \"%s\" or missing value", > - p); > - //return -EINVAL; > - } > + else > + fc->sb_flags &= ~SB_POSIXACL; > + break; > + case Opt_showmeta: > + opts->showmeta = result.negated ? 0 : 1; > + break; > + case Opt_nls: > + opts->nls_name = param->string; > + param->string = NULL; > + break; > + case Opt_prealloc: > + opts->prealloc = result.negated ? 0 : 1; > + break; > + case Opt_no_acs_rules: > + opts->no_acs_rules = 1; > + break; > + default: > + /* Should not be here unless we forget add case. */ > + return -EINVAL; > } > - > -out: > - if (!strcmp(nls_name[0] ? nls_name : CONFIG_NLS_DEFAULT, "utf8")) { > - /* For UTF-8 use utf16s_to_utf8s/utf8s_to_utf16s instead of nls */ > - nls = NULL; > - } else if (nls_name[0]) { > - nls = load_nls(nls_name); > - if (!nls) { > - ntfs_err(sb, "failed to load \"%s\"", nls_name); > - return -EINVAL; > - } > - } else { > - nls = load_nls_default(); > - if (!nls) { > - ntfs_err(sb, "failed to load default nls"); > - return -EINVAL; > - } > - } > - opts->nls = nls; > - > return 0; > } > > -static int ntfs_remount(struct super_block *sb, int *flags, char *data) > +static int ntfs_fs_reconfigure(struct fs_context *fc) > { > - int err, ro_rw; > + struct super_block *sb = fc->root->d_sb; > struct ntfs_sb_info *sbi = sb->s_fs_info; > - struct ntfs_mount_options old_opts; > - char *orig_data = kstrdup(data, GFP_KERNEL); > + struct ntfs_mount_options *new_opts = fc->s_fs_info; > + int ro_rw; > > - if (data && !orig_data) > - return -ENOMEM; > - > - /* Store original options */ > - memcpy(&old_opts, &sbi->options, sizeof(old_opts)); > - clear_mount_options(&sbi->options); > - memset(&sbi->options, 0, sizeof(sbi->options)); > - > - err = ntfs_parse_options(sb, data, 0, &sbi->options); > - if (err) > - goto restore_opts; > - > - ro_rw = sb_rdonly(sb) && !(*flags & SB_RDONLY); > + ro_rw = sb_rdonly(sb) && !(fc->sb_flags & SB_RDONLY); > if (ro_rw && (sbi->flags & NTFS_FLAGS_NEED_REPLAY)) { > - ntfs_warn( > - sb, > - "Couldn't remount rw because journal is not replayed. Please umount/remount instead\n"); > - err = -EINVAL; > - goto restore_opts; > + errorf(fc, "ntfs3: Couldn't remount rw because journal is not replayed. Please umount/remount instead\n"); > + goto clear_new_fc; > } > > sync_filesystem(sb); > > if (ro_rw && (sbi->volume.flags & VOLUME_FLAG_DIRTY) && > - !sbi->options.force) { > - ntfs_warn(sb, "volume is dirty and \"force\" flag is not set!"); > - err = -EINVAL; > - goto restore_opts; > + !new_opts->force) { > + errorf(fc, "ntfs3: Volume is dirty and \"force\" flag is not set!"); > + goto clear_new_fc; > } > > - clear_mount_options(&old_opts); > + /* > + * TODO: We should probably check some mount options does > + * they all work after remount. Example can we really change > + * nls. Remove this comment when all testing is done or > + * even better xfstest is made for it. > + */ > > - ntfs_info(sb, "re-mounted. Opts: %s", orig_data); > - err = 0; > - goto out; > + new_opts->nls = ntfs_load_nls(new_opts->nls_name); > + if (IS_ERR(new_opts->nls)) { > + errorf(fc, "ntfs3: Cannot load nls %s", new_opts->nls_name); > + goto clear_new_fc; > + } > > -restore_opts: > clear_mount_options(&sbi->options); > - memcpy(&sbi->options, &old_opts, sizeof(old_opts)); > + sbi->options = *new_opts; > + return 0; > > -out: > - kfree(orig_data); > - return err; > +clear_new_fc: > + clear_mount_options(new_opts); > + return -EINVAL; > } > > static struct kmem_cache *ntfs_inode_cachep; > @@ -545,10 +518,8 @@ static int ntfs_show_options(struct seq_file *m, struct dentry *root) > seq_printf(m, ",fmask=%04o", ~opts->fs_fmask_inv); > if (opts->dmask) > seq_printf(m, ",dmask=%04o", ~opts->fs_dmask_inv); > - if (opts->nls) > - seq_printf(m, ",nls=%s", opts->nls->charset); > - else > - seq_puts(m, ",nls=utf8"); > + if (opts->nls_name) > + seq_printf(m, ",nls=%s", opts->nls_name); > if (opts->sys_immutable) > seq_puts(m, ",sys_immutable"); > if (opts->discard) > @@ -619,7 +590,6 @@ static const struct super_operations ntfs_sops = { > .statfs = ntfs_statfs, > .show_options = ntfs_show_options, > .sync_fs = ntfs_sync_fs, > - .remount_fs = ntfs_remount, > .write_inode = ntfs3_write_inode, > }; > > @@ -883,10 +853,10 @@ static int ntfs_init_from_boot(struct super_block *sb, u32 sector_size, > } > > /* try to mount*/ > -static int ntfs_fill_super(struct super_block *sb, void *data, int silent) > +static int ntfs_fill_super(struct super_block *sb, struct fs_context *fc) > { > int err; > - struct ntfs_sb_info *sbi; > + struct ntfs_sb_info *sbi = sb->s_fs_info; > struct block_device *bdev = sb->s_bdev; > struct inode *bd_inode = bdev->bd_inode; > struct request_queue *rq = bdev_get_queue(bdev); > @@ -905,11 +875,6 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent) > > ref.high = 0; > > - sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info)); > - if (!sbi) > - return -ENOMEM; > - > - sb->s_fs_info = sbi; > sbi->sb = sb; > sb->s_flags |= SB_NODIRATIME; > sb->s_magic = 0x7366746e; // "ntfs" > @@ -921,10 +886,6 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent) > ratelimit_state_init(&sbi->msg_ratelimit, DEFAULT_RATELIMIT_INTERVAL, > DEFAULT_RATELIMIT_BURST); > > - err = ntfs_parse_options(sb, data, silent, &sbi->options); > - if (err) > - goto out; > - > if (!rq || !blk_queue_discard(rq) || !rq->limits.discard_granularity) { > ; Sidenote, why is this lonely semicolon here? Maybe you can rewrite this so you don't need this branch at all. > } else { > @@ -933,6 +894,14 @@ static int ntfs_fill_super(struct super_block *sb, void *data, int silent) > ~(u64)(sbi->discard_granularity - 1); > } > > + sbi->options.nls = ntfs_load_nls(sbi->options.nls_name); > + if (IS_ERR(sbi->options.nls)) { > + ntfs_err(sb, "ntfs3: Cannot load nls %s", > + sbi->options.nls_name); > + err = PTR_ERR(sbi->options.nls); > + goto out; > + } > + > sb_set_blocksize(sb, PAGE_SIZE); > > /* parse boot */ > @@ -1400,19 +1369,54 @@ int ntfs_discard(struct ntfs_sb_info *sbi, CLST lcn, CLST len) > return err; > } > > -static struct dentry *ntfs_mount(struct file_system_type *fs_type, int flags, > - const char *dev_name, void *data) > +static int ntfs_fs_get_tree(struct fs_context *fc) > +{ > + return get_tree_bdev(fc, ntfs_fill_super); > +} > + > +static void ntfs_fs_free(struct fs_context *fc) > +{ > + struct ntfs_sb_info *sbi = fc->s_fs_info; > + > + if (sbi) > + put_ntfs(sbi); > +} > + > +static const struct fs_context_operations ntfs_context_ops = { > + .parse_param = ntfs_fs_parse_param, > + .get_tree = ntfs_fs_get_tree, > + .reconfigure = ntfs_fs_reconfigure, > + .free = ntfs_fs_free, > +}; > + > +static int ntfs_init_fs_context(struct fs_context *fc) > { > - return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super); > + struct ntfs_sb_info *sbi; > + > + sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info)); > + if (!sbi) > + return -ENOMEM; > + > + /* Default options */ > + sbi->options.fs_uid = current_uid(); > + sbi->options.fs_gid = current_gid(); > + sbi->options.fs_fmask_inv = ~current_umask(); > + sbi->options.fs_dmask_inv = ~current_umask(); > + > + fc->s_fs_info = sbi; > + fc->ops = &ntfs_context_ops; > + > + return 0; > } > > // clang-format off > static struct file_system_type ntfs_fs_type = { > - .owner = THIS_MODULE, > - .name = "ntfs3", > - .mount = ntfs_mount, > - .kill_sb = kill_block_super, > - .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, > + .owner = THIS_MODULE, > + .name = "ntfs3", > + .init_fs_context = ntfs_init_fs_context, > + .parameters = ntfs_fs_parameters, > + .kill_sb = kill_block_super, > + .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, > }; > // clang-format on > > -- > 2.25.1 >