O
Sat, Aug 21, 2021 at 11:59:39AM +0300, Kari Argillander wrote:
> Bcc:
> Subject: Re: [PATCH] proc: prevent mount proc on same mountpoint in one pid
> namespace
> Reply-To:
> In-Reply-To: <20210821083105.30336-1-yang.yang29@xxxxxxxxxx>
>
> On Sat, Aug 21, 2021 at 01:31:05AM -0700, cgel.zte@xxxxxxxxx wrote:
> > From: Yang Yang <yang.yang29@xxxxxxxxxx>
> >
> > Patch "proc: allow to mount many instances of proc in one pid namespace"
> > aims to mount many instances of proc on different mountpoint, see
> > tools/testing/selftests/proc/proc-multiple-procfs.c.
> >
> > But there is a side-effects, user can mount many instances of proc on
> > the same mountpoint in one pid namespace, which is not allowed before.
> > This duplicate mount makes no sense but wastes memory and CPU, and user
> > may be confused why kernel allows it.
> >
> > The logic of this patch is: when try to mount proc on /mnt, check if
> > there is a proc instance mount on /mnt in the same pid namespace. If
> > answer is yes, return -EBUSY.
> >
> > Since this check can't be done in proc_get_tree(), which call
> > get_tree_nodev() and will create new super_block unconditionally.
> > And other nodev fs may faces the same case, so add a new hook in
> > fs_context_operations.
> >
> > Reported-by: Zeal Robot <zealci@xxxxxxxxxx>
> > Signed-off-by: Yang Yang <yang.yang29@xxxxxxxxxx>
> > ---
> > fs/namespace.c | 9 +++++++++
> > fs/proc/root.c | 15 +++++++++++++++
> > include/linux/fs_context.h | 1 +
> > 3 files changed, 25 insertions(+)
> >
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index f79d9471cb76..84da649a70c5 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -2878,6 +2878,7 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
> > static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
> > int mnt_flags, const char *name, void *data)
> > {
> > + int (*check_mntpoint)(struct fs_context *fc, struct path *path);
> > struct file_system_type *type;
> > struct fs_context *fc;
> > const char *subtype = NULL;
> > @@ -2906,6 +2907,13 @@ static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
> > if (IS_ERR(fc))
> > return PTR_ERR(fc);
> >
> > + /* check if there is a same super_block mount on path*/
> > + check_mntpoint = fc->ops->check_mntpoint;
> > + if (check_mntpoint)
> > + err = check_mntpoint(fc, path);
> > + if (err < 0)
> > + goto err_fc;
> > +
> > if (subtype)
> > err = vfs_parse_fs_string(fc, "subtype",
> > subtype, strlen(subtype));
> > @@ -2920,6 +2928,7 @@ static int do_new_mount(struct path *path, const char *fstype, int sb_flags,
> > if (!err)
> > err = do_new_mount_fc(fc, path, mnt_flags);
> >
> > +err_fc:
> > put_fs_context(fc);
> > return err;
> > }
> > diff --git a/fs/proc/root.c b/fs/proc/root.c
> > index c7e3b1350ef8..0971d6b0bec2 100644
> > --- a/fs/proc/root.c
> > +++ b/fs/proc/root.c
> > @@ -237,11 +237,26 @@ static void proc_fs_context_free(struct fs_context *fc)
> > kfree(ctx);
> > }
> >
> > +static int proc_check_mntpoint(struct fs_context *fc, struct path *path)
> > +{
> > + struct super_block *mnt_sb = path->mnt->mnt_sb;
> > + struct proc_fs_info *fs_info;
> > +
> > + if (strcmp(mnt_sb->s_type->name, "proc") == 0) {
> > + fs_info = mnt_sb->s_fs_info;
> > + if (fs_info->pid_ns == task_active_pid_ns(current) &&
> > + path->mnt->mnt_root == path->dentry)
> > + return -EBUSY;
> > + }
> > + return 0;
> > +}
> > +
> > static const struct fs_context_operations proc_fs_context_ops = {
> > .free = proc_fs_context_free,
> > .parse_param = proc_parse_param,
> > .get_tree = proc_get_tree,
> > .reconfigure = proc_reconfigure,
> > + .check_mntpoint = proc_check_mntpoint,
> > };
> >
> > static int proc_init_fs_context(struct fs_context *fc)
> > diff --git a/include/linux/fs_context.h b/include/linux/fs_context.h
> > index 6b54982fc5f3..090a05fb2d7d 100644
> > --- a/include/linux/fs_context.h
> > +++ b/include/linux/fs_context.h
> > @@ -119,6 +119,7 @@ struct fs_context_operations {
> > int (*parse_monolithic)(struct fs_context *fc, void *data);
> > int (*get_tree)(struct fs_context *fc);
> > int (*reconfigure)(struct fs_context *fc);
> > + int (*check_mntpoint)(struct fs_context *fc, struct path *path);
>
> Don't you think this should be it's own patch. It is after all internal
> api change. This also needs documentation. It would be confusing if
> someone convert to new mount api and there is one line which just
> address some proc stuff but even commit message does not address does
> every fs needs to add this.
>
> Documentation is very good shape right now and we are in face that
> everyone is migrating to use new mount api so everyting should be well
> documented.
> i
Thanks for your reply!
I will take commit message more carefully next time.
Sinece I am not quit sure about this patch, so I didn't write
Documentation for patch v1. AIViro had made it clear, so this
patch is abondoned.
> > };
> >
> > /*
> > --
> > 2.25.1
> >
