[PATCH v2] securityfs: fix missing of d_delete() in securityfs_remove()

[Paul Moore]

On Wed, May 7, 2025 at 7:12 AM <alexjlzheng at gmail.com> wrote:
> From: Jinliang Zheng <alexjlzheng at tencent.com>
>
> Consider the following module code (just an example to make it easier to
> illustrate the problem, in fact the LSM module will not be dynamically
> unloaded):
>
>   static struct dentry *dentry;
>
>   static int __init securityfs_test_init(void)
>   {
>           dentry = securityfs_create_dir("standon", NULL);
>           return PTR_ERR(dentry);
>   }
>
>   static void __exit securityfs_test_exit(void)
>   {
>           securityfs_remove(dentry);
>   }
>
>   module_init(securityfs_test_init);
>   module_exit(securityfs_test_exit);
>
> and then:
>
>   insmod /path/to/thismodule
>   cd /sys/kernel/security/standon     <- we hold 'standon'
>   rmmod thismodule                    <- 'standon' don't go away
>   insmod /path/to/thismodule          <- Failed: File exists!

I mentioned this on your original patch, but I'll mention it again
with a bit more of an explanation behind it.  As you know, we don't
currently support dynamically loaded LSMs, which means the reproducer
above isn't really valid from a supported configuration perspective,
even if it does happen to trigger the behavior you are describing.
This may seem silly to you, but you really should stick with valid
configurations when trying to reproduce things as sometimes when
developers see an invalid/unsupported config they may stop reading and
dismiss your concern with a "don't do that!", which is surely not what
you want.

At the very least, I'm personally not sure we would want an
invalid/unsupported reproducer in the git log for the LSM subsystem.

> Although the LSM module will not be dynamically added or deleted after
> the kernel is started, it may dynamically add or delete pseudo files
> for status export or function configuration in userspace according to
> different status, which we are not prohibited from doing so.
>
> In addition, securityfs_recursive_remove() avoids this problem by calling
> __d_drop() directly. As a non-recursive version, it is somewhat strange
> that securityfs_remove() does not clean up the deleted dentry.
>
> Fix this by adding d_delete() in securityfs_remove().

I wondering why we don't simply replace all instances of
securityfs_remove() with securityfs_recursive_remove(), or more likely
just remove the existing securityfs_remove() and rename the
securityfs_recursive_remove() to securityfs_remove().  Do any existing
LSMs rely on securityfs_remove() *not* acting recursively?

> Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
> Signed-off-by: Jinliang Zheng <alexjlzheng at tencent.com>
> ---
> changelog:
> v2: Modify the commit message to make it clearer
> v1: https://lore.kernel.org/all/20250426150931.2840-1-alexjlzheng@tencent.com/
> ---
>  security/inode.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/security/inode.c b/security/inode.c
> index da3ab44c8e57..d99baf26350a 100644
> --- a/security/inode.c
> +++ b/security/inode.c
> @@ -306,6 +306,7 @@ void securityfs_remove(struct dentry *dentry)
>                         simple_rmdir(dir, dentry);
>                 else
>                         simple_unlink(dir, dentry);
> +               d_delete(dentry);
>                 dput(dentry);
>         }
>         inode_unlock(dir);
> --
> 2.49.0

-- 
paul-moore.com