Re: [PATCH] binderfs: port to new mount api

From: Kees Cook
Date: Thu Mar 12 2020 - 19:56:17 EST

Next message: Kees Cook: "Re: [PATCH v9 02/13] ELF: Add ELF program property parsing support"
Previous message: Kees Cook: "Re: [PATCH 3/3] binderfs_test: switch from /dev to /tmp as mountpoint"
In reply to: Christian Brauner: "[PATCH] binderfs: port to new mount api"
Next in thread: Christian Brauner: "Re: [PATCH] binderfs: port to new mount api"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Mar 12, 2020 at 10:24:20PM +0100, Christian Brauner wrote:
> It's time we port binderfs to the new mount api. We can make use of the
> new option parser, get nicer infrastructure and it will be easiert if we
> ever add any new mount options.
>
> This survives testing with the binderfs selftests:
>
> for i in `seq 1 1000`; do ./binderfs_test; done
>
> including the new stress tests I sent out for review today:
>
> [==========] Running 3 tests from 1 test cases.
> [ RUN ] global.binderfs_stress
> [ OK ] global.binderfs_stress
> [ RUN ] global.binderfs_test_privileged
> # Tests are not run as root. Skipping privileged tests
> [ OK ] global.binderfs_test_privileged

I would use the XFAIL harness infrastructure for these kinds of skips.

-Kees

> [ RUN ] global.binderfs_test_unprivileged
> # Allocated new binder device with major 243, minor 4, and name my-binder
> # Detected binder version: 8
> [ OK ] global.binderfs_test_unprivileged
> [==========] 3 / 3 tests passed.
> [ PASSED ]
>
> Cc: Todd Kjos <tkjos@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> ---
> drivers/android/binderfs.c | 200 +++++++++++++++++++------------------
> 1 file changed, 105 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index f303106b3362..2c89e0b5a82d 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -18,7 +18,7 @@
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/mount.h>
> -#include <linux/parser.h>
> +#include <linux/fs_parser.h>
> #include <linux/radix-tree.h>
> #include <linux/sched.h>
> #include <linux/seq_file.h>
> @@ -48,26 +48,30 @@ static dev_t binderfs_dev;
> static DEFINE_MUTEX(binderfs_minors_mutex);
> static DEFINE_IDA(binderfs_minors);
>
> -enum {
> +enum binderfs_param {
> Opt_max,
> Opt_stats_mode,
> - Opt_err
> };
>
> enum binderfs_stats_mode {
> - STATS_NONE,
> - STATS_GLOBAL,
> + binderfs_stats_mode_unset,
> + binderfs_stats_mode_global,
> };
>
> -static const match_table_t tokens = {
> - { Opt_max, "max=%d" },
> - { Opt_stats_mode, "stats=%s" },
> - { Opt_err, NULL }
> +static const struct constant_table binderfs_param_stats[] = {
> + { "global", binderfs_stats_mode_global },
> + {}
> };
>
> -static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
> +const struct fs_parameter_spec binderfs_fs_parameters[] = {
> + fsparam_u32("max", Opt_max),
> + fsparam_enum("stats", Opt_stats_mode, binderfs_param_stats),
> + {}
> +};
> +
> +static inline struct binderfs_info *BINDERFS_SB(const struct super_block *sb)
> {
> - return inode->i_sb->s_fs_info;
> + return sb->s_fs_info;
> }
>
> bool is_binderfs_device(const struct inode *inode)
> @@ -246,7 +250,7 @@ static long binder_ctl_ioctl(struct file *file, unsigned int cmd,
> static void binderfs_evict_inode(struct inode *inode)
> {
> struct binder_device *device = inode->i_private;
> - struct binderfs_info *info = BINDERFS_I(inode);
> + struct binderfs_info *info = BINDERFS_SB(inode->i_sb);
>
> clear_inode(inode);
>
> @@ -264,97 +268,85 @@ static void binderfs_evict_inode(struct inode *inode)
> }
> }
>
> -/**
> - * binderfs_parse_mount_opts - parse binderfs mount options
> - * @data: options to set (can be NULL in which case defaults are used)
> - */
> -static int binderfs_parse_mount_opts(char *data,
> - struct binderfs_mount_opts *opts)
> +static int binderfs_fs_context_parse_param(struct fs_context *fc,
> + struct fs_parameter *param)
> {
> - char *p, *stats;
> - opts->max = BINDERFS_MAX_MINOR;
> - opts->stats_mode = STATS_NONE;
> -
> - while ((p = strsep(&data, ",")) != NULL) {
> - substring_t args[MAX_OPT_ARGS];
> - int token;
> - int max_devices;
> -
> - if (!*p)
> - continue;
> -
> - token = match_token(p, tokens, args);
> - switch (token) {
> - case Opt_max:
> - if (match_int(&args[0], &max_devices) ||
> - (max_devices < 0 ||
> - (max_devices > BINDERFS_MAX_MINOR)))
> - return -EINVAL;
> -
> - opts->max = max_devices;
> - break;
> - case Opt_stats_mode:
> - if (!capable(CAP_SYS_ADMIN))
> - return -EINVAL;
> + int opt;
> + struct binderfs_mount_opts *ctx = fc->fs_private;
> + struct fs_parse_result result;
>
> - stats = match_strdup(&args[0]);
> - if (!stats)
> - return -ENOMEM;
> + opt = fs_parse(fc, binderfs_fs_parameters, param, &result);
> + if (opt < 0)
> + return opt;
>
> - if (strcmp(stats, "global") != 0) {
> - kfree(stats);
> - return -EINVAL;
> - }
> + switch (opt) {
> + case Opt_max:
> + if (result.uint_32 > BINDERFS_MAX_MINOR)
> + return invalfc(fc, "Bad value for '%s'", param->key);
>
> - opts->stats_mode = STATS_GLOBAL;
> - kfree(stats);
> - break;
> - default:
> - pr_err("Invalid mount options\n");
> - return -EINVAL;
> - }
> + ctx->max = result.uint_32;
> + break;
> + case Opt_stats_mode:
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + ctx->stats_mode = result.uint_32;
> + break;
> + default:
> + return invalfc(fc, "Unsupported parameter '%s'", param->key);
> }
>
> return 0;
> }
>
> -static int binderfs_remount(struct super_block *sb, int *flags, char *data)
> +static int binderfs_fs_context_reconfigure(struct fs_context *fc)
> {
> - int prev_stats_mode, ret;
> - struct binderfs_info *info = sb->s_fs_info;
> + struct binderfs_mount_opts *ctx = fc->fs_private;
> + struct binderfs_info *info = BINDERFS_SB(fc->root->d_sb);
>
> - prev_stats_mode = info->mount_opts.stats_mode;
> - ret = binderfs_parse_mount_opts(data, &info->mount_opts);
> - if (ret)
> - return ret;
> + if (info->mount_opts.stats_mode != ctx->stats_mode)
> + return invalfc(fc, "Binderfs stats mode cannot be changed during a remount");
>
> - if (prev_stats_mode != info->mount_opts.stats_mode) {
> - pr_err("Binderfs stats mode cannot be changed during a remount\n");
> - info->mount_opts.stats_mode = prev_stats_mode;
> - return -EINVAL;
> - }
> + info->mount_opts.stats_mode = ctx->stats_mode;
> + info->mount_opts.max = ctx->max;
>
> return 0;
> }
>
> -static int binderfs_show_mount_opts(struct seq_file *seq, struct dentry *root)
> +static int binderfs_show_options(struct seq_file *seq, struct dentry *root)
> {
> - struct binderfs_info *info;
> + struct binderfs_info *info = BINDERFS_SB(root->d_sb);
>
> - info = root->d_sb->s_fs_info;
> if (info->mount_opts.max <= BINDERFS_MAX_MINOR)
> seq_printf(seq, ",max=%d", info->mount_opts.max);
> - if (info->mount_opts.stats_mode == STATS_GLOBAL)
> +
> + switch (info->mount_opts.stats_mode) {
> + case binderfs_stats_mode_unset:
> + break;
> + case binderfs_stats_mode_global:
> seq_printf(seq, ",stats=global");
> + break;
> + }
>
> return 0;
> }
>
> +static void binderfs_put_super(struct super_block *sb)
> +{
> + struct binderfs_info *info = sb->s_fs_info;
> +
> + if (info && info->ipc_ns)
> + put_ipc_ns(info->ipc_ns);
> +
> + kfree(info);
> + sb->s_fs_info = NULL;
> +}
> +
> static const struct super_operations binderfs_super_ops = {
> .evict_inode = binderfs_evict_inode,
> - .remount_fs = binderfs_remount,
> - .show_options = binderfs_show_mount_opts,
> + .show_options = binderfs_show_options,
> .statfs = simple_statfs,
> + .put_super = binderfs_put_super,
> };
>
> static inline bool is_binderfs_control_device(const struct dentry *dentry)
> @@ -653,10 +645,11 @@ static int init_binder_logs(struct super_block *sb)
> return ret;
> }
>
> -static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> +static int binderfs_fill_super(struct super_block *sb, struct fs_context *fc)
> {
> int ret;
> struct binderfs_info *info;
> + struct binderfs_mount_opts *ctx = fc->fs_private;
> struct inode *inode = NULL;
> struct binderfs_device device_info = { 0 };
> const char *name;
> @@ -689,16 +682,14 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>
> info->ipc_ns = get_ipc_ns(current->nsproxy->ipc_ns);
>
> - ret = binderfs_parse_mount_opts(data, &info->mount_opts);
> - if (ret)
> - return ret;
> -
> info->root_gid = make_kgid(sb->s_user_ns, 0);
> if (!gid_valid(info->root_gid))
> info->root_gid = GLOBAL_ROOT_GID;
> info->root_uid = make_kuid(sb->s_user_ns, 0);
> if (!uid_valid(info->root_uid))
> info->root_uid = GLOBAL_ROOT_UID;
> + info->mount_opts.max = ctx->max;
> + info->mount_opts.stats_mode = ctx->stats_mode;
>
> inode = new_inode(sb);
> if (!inode)
> @@ -730,36 +721,55 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> name++;
> }
>
> - if (info->mount_opts.stats_mode == STATS_GLOBAL)
> + if (info->mount_opts.stats_mode == binderfs_stats_mode_global)
> return init_binder_logs(sb);
>
> return 0;
> }
>
> -static struct dentry *binderfs_mount(struct file_system_type *fs_type,
> - int flags, const char *dev_name,
> - void *data)
> +static int binderfs_fs_context_get_tree(struct fs_context *fc)
> {
> - return mount_nodev(fs_type, flags, data, binderfs_fill_super);
> + return get_tree_nodev(fc, binderfs_fill_super);
> }
>
> -static void binderfs_kill_super(struct super_block *sb)
> +static void binderfs_fs_context_free(struct fs_context *fc)
> {
> - struct binderfs_info *info = sb->s_fs_info;
> + struct binderfs_mount_opts *ctx = fc->fs_private;
> +
> + fc->fs_private = NULL;
> + kfree(ctx);
> +}
>
> - kill_litter_super(sb);
> +static const struct fs_context_operations binderfs_fs_context_ops = {
> + .free = binderfs_fs_context_free,
> + .get_tree = binderfs_fs_context_get_tree,
> + .parse_param = binderfs_fs_context_parse_param,
> + .reconfigure = binderfs_fs_context_reconfigure,
> +};
>
> - if (info && info->ipc_ns)
> - put_ipc_ns(info->ipc_ns);
> +static int binderfs_init_fs_context(struct fs_context *fc)
> +{
> + struct binderfs_mount_opts *ctx;
>
> - kfree(info);
> + ctx = kzalloc(sizeof(struct binderfs_mount_opts), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->max = BINDERFS_MAX_MINOR;
> + ctx->stats_mode = binderfs_stats_mode_unset;
> +
> + fc->fs_private = ctx;
> + fc->ops = &binderfs_fs_context_ops;
> +
> + return 0;
> }
>
> static struct file_system_type binder_fs_type = {
> - .name = "binder",
> - .mount = binderfs_mount,
> - .kill_sb = binderfs_kill_super,
> - .fs_flags = FS_USERNS_MOUNT,
> + .name = "binder",
> + .init_fs_context = binderfs_init_fs_context,
> + .parameters = binderfs_fs_parameters,
> + .kill_sb = kill_litter_super,
> + .fs_flags = FS_USERNS_MOUNT,
> };
>
> int __init init_binderfs(void)
>
> base-commit: f17f06a0c7794d3a7c2425663738823354447472
> --
> 2.25.1
>

--
Kees Cook

Next message: Kees Cook: "Re: [PATCH v9 02/13] ELF: Add ELF program property parsing support"
Previous message: Kees Cook: "Re: [PATCH 3/3] binderfs_test: switch from /dev to /tmp as mountpoint"
In reply to: Christian Brauner: "[PATCH] binderfs: port to new mount api"
Next in thread: Christian Brauner: "Re: [PATCH] binderfs: port to new mount api"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]