linux-kernel - Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210816123619.GB17355@lst.de>
Date:   Mon, 16 Aug 2021 14:36:19 +0200
From:   Christoph Hellwig <hch@....de>
To:     Kari Argillander <kari.argillander@...il.com>
Cc:     Konstantin Komarov <almaz.alexandrovich@...agon-software.com>,
        Christoph Hellwig <hch@....de>, ntfs3@...ts.linux.dev,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Pali Rohár <pali@...nel.org>,
        Matthew Wilcox <willy@...radead.org>
Subject: Re: [RFC PATCH 1/4] fs/ntfs3: Use new api for mounting

> +/*
> + * ntfs_load_nls
> + *

No need to state the function name here.

> + * 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)
> +		return ERR_PTR(-EINVAL);
> +	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;
> +}

This looks like something quite generic and not file system specific.
But I haven't found time to look at the series from Pali how this all
fits together.

> +// clang-format off

Please don't use C++ comments.  And we also should not put weird
formatter annotations into the kernel source anyway.

> +static void ntfs_default_options(struct ntfs_mount_options *opts)
>  {
>  	opts->fs_uid = current_uid();
>  	opts->fs_gid = current_gid();
> +	opts->fs_fmask_inv = ~current_umask();
> +	opts->fs_dmask_inv = ~current_umask();
> +	opts->nls = ntfs_load_nls(CONFIG_NLS_DEFAULT);
> +}

This function seems pretty pointless with a single trivial caller.

> +static int ntfs_fs_parse_param(struct fs_context *fc, struct fs_parameter *param)

Please avoid the overly long line.

> +		break;
> +	case Opt_showmeta:
> +		opts->showmeta = result.negated ? 0 : 1;
> +		break;
> +	case Opt_nls:
> +		unload_nls(opts->nls);
> +
> +		opts->nls = ntfs_load_nls(param->string);
> +		if (IS_ERR(opts->nls)) {
> +			return invalf(fc, "ntfs3: Cannot load nls %s",
> +				      param->string);
>  		}

So instead of unloading here, why not set keep a copy of the string
in the mount options structure and only load the actual table after
option parsing has finished?

> +     struct ntfs_mount_options *new_opts = fc->s_fs_info;

Does this rely on the mount_options being the first member in struct
ntfs_sb_info?  If so that is a landmine for future changes.

> +/*
> + * Set up the filesystem mount context.
> + */
> +static int ntfs_init_fs_context(struct fs_context *fc)
> +{
> +	struct ntfs_sb_info *sbi;
> +
> +	sbi = ntfs_zalloc(sizeof(struct ntfs_sb_info));

Not related to your patch, but why does ntfs3 have kmalloc wrappers
like this?