Re: [PATCH 31/39] convert selinuxfs

Paul Moore
2025-09-21T20:44:40Z

On Sat, Sep 20, 2025 at 3:48 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Tree has invariant part + two subtrees that get replaced upon each
> policy load. Invariant parts stay for the lifetime of filesystem,
> these two subdirs - from policy load to policy load (serialized
> on lock_rename(root, ...)).
>
> All object creations are via d_alloc_name()+d_add() inside selinuxfs,
> all removals are via simple_recursive_removal().
>
> Turn those d_add() into d_make_persistent()+dput() and that's mostly it.
> Don't bother to store the dentry of /policy_capabilities - it belongs
> to invariant part of tree and we only use it to populate that directory,
> so there's no reason to keep it around afterwards.

Minor comment on that below, as well as a comment style nitpick, but
overall no major concerns from me.

Acked-by: Paul Moore <paul@paul-moore.com>

> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> security/selinux/selinuxfs.c | 52 +++++++++++++++++++++---------------
> 1 file changed, 30 insertions(+), 22 deletions(-)

...

> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 9aa1d03ab612..dc1bb49664f2 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1966,10 +1973,11 @@ static struct dentry *sel_make_swapover_dir(struct super_block *sb,
> /* directory inodes start off with i_nlink == 2 (for "." entry) */
> inc_nlink(inode);
> inode_lock(sb->s_root->d_inode);
> - d_add(dentry, inode);
> + d_make_persistent(dentry, inode);
> inc_nlink(sb->s_root->d_inode);
> inode_unlock(sb->s_root->d_inode);
> - return dentry;
> + dput(dentry);
> + return dentry; // borrowed
> }

Prefer C style comments on their own line:

dput(dentry);
/* borrowed dentry */
return dentry;

> @@ -2079,15 +2088,14 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
> goto err;
> }
>
> - fsi->policycap_dir = sel_make_dir(sb->s_root, POLICYCAP_DIR_NAME,
> + dentry = sel_make_dir(sb->s_root, POLICYCAP_DIR_NAME,
> &fsi->last_ino);

I'd probably keep fsi->policycap_dir in this patch simply to limit the
scope of this patch to just the DCACHE_PERSISTENT related changes, but
I'm not going to make a big fuss about that.

> - if (IS_ERR(fsi->policycap_dir)) {
> - ret = PTR_ERR(fsi->policycap_dir);
> - fsi->policycap_dir = NULL;
> + if (IS_ERR(dentry)) {
> + ret = PTR_ERR(dentry);
> goto err;
> }
>
> - ret = sel_make_policycap(fsi);
> + ret = sel_make_policycap(fsi, dentry);
> if (ret) {
> pr_err("SELinux: failed to load policy capabilities\n");
> goto err;

--
paul-moore.com