On Mon, Dec 06, 2021 at 04:14:15PM -0500, James Bottomley wrote:
> On Mon, 2021-12-06 at 12:25 -0500, Stefan Berger wrote:
> [...]
> > v3:
> > - Further modifications to virtualized SecurityFS following James's
> > posted patch
> > - Dropping of early teardown for user_namespaces since not needed
> > anymore
>
> This is my incremental to this series that moves the namespaced
> securityfs away from using a vfsmount and on to a root dentry instead,
> meaning we can call the blocking notifier from fill_super as Christian
> requested (and thus can remove the securityfs_notifier_sent indicator
> since it's only called once).
Somehow b4 retrieves your patch out-of-band which makes it weird to
reply to so I'm copy-pasting it here and reply inline:
On Mon, Dec 06, 2021 at 08:27:00PM +0000, James Bottomley wrote:
> ---
> include/linux/user_namespace.h | 3 +-
> security/inode.c | 55 +++++++++++++---------------------
> 2 files changed, 22 insertions(+), 36 deletions(-)
>
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index 6b8bd060d8c4..03a0879376a0 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -104,8 +104,7 @@ struct user_namespace {
> struct ima_namespace *ima_ns;
> #endif
> #ifdef CONFIG_SECURITYFS
> - struct vfsmount *securityfs_mount;
> - bool securityfs_notifier_sent;
> + struct dentry *securityfs_root;
> #endif
> } __randomize_layout;
>
> diff --git a/security/inode.c b/security/inode.c
> index 45211845fc31..f8b6cb3dfb87 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -24,6 +24,7 @@
> #include <linux/magic.h>
> #include <linux/user_namespace.h>
>
> +static struct vfsmount *securityfs_mount;
> static int securityfs_mount_count;
>
> static BLOCKING_NOTIFIER_HEAD(securityfs_ns_notifier);
> @@ -40,43 +41,24 @@ static const struct super_operations securityfs_super_operations = {
> .free_inode = securityfs_free_inode,
> };
>
> -static struct file_system_type fs_type;
> -
> -static void securityfs_free_context(struct fs_context *fc)
> -{
> - struct user_namespace *ns = fc->user_ns;
> -
> - if (ns == &init_user_ns ||
> - ns->securityfs_notifier_sent)
> - return;
> -
> - ns->securityfs_notifier_sent = true;
> -
> - ns->securityfs_mount = vfs_kern_mount(&fs_type, SB_KERNMOUNT,
> - fs_type.name, NULL);
> - if (IS_ERR(ns->securityfs_mount)) {
> - printk(KERN_ERR "kern mount on securityfs ERROR: %ld\n",
> - PTR_ERR(ns->securityfs_mount));
> - ns->securityfs_mount = NULL;
> - return;
> - }
> -
> - blocking_notifier_call_chain(&securityfs_ns_notifier,
> - SECURITYFS_NS_ADD, fc->user_ns);
> - mntput(ns->securityfs_mount);
> -}
> -
> static int securityfs_fill_super(struct super_block *sb, struct fs_context *fc)
> {
> static const struct tree_descr files[] = {{""}};
> int error;
> + struct user_namespace *ns = fc->user_ns;
>
> error = simple_fill_super(sb, SECURITYFS_MAGIC, files);
> if (error)
> return error;
>
> + ns->securityfs_root = dget(sb->s_root);
> +
> sb->s_op = &securityfs_super_operations;
>
> + if (ns != &init_user_ns)
> + blocking_notifier_call_chain(&securityfs_ns_notifier,
> + SECURITYFS_NS_ADD, ns);
I would propose not to use the notifier logic. While it might be nifty
it's over-engineered in my opinion. The dentry stashing in struct
user_namespace currently serves the purpose to make it retrievable in
ima_fs_ns_init(). That doesn't justify its existence imho.
There is one central place were all users of namespaced securityfs can
create the files that they need to and that is in
securityfs_fill_super(). (If you want to make that more obvious then give
it a subdirectory securityfs and move inode.c in there.)
We simply will expect users to add:
ima_init_securityfs()
mylsm_init_securityfs()
that are to be placed in fill_super
and
ima_kill_securityfs()
mylsm_kill_securityfs()
that get called in kill_super and the root dentry and other relevant
information should be passed explicitly into those functions. Then we
can remove the dentry stashing from struct user_namespace altogether and
the patch gets smaller too.
