On Wed, 7 May 2025 16:10:11 -0400, Paul Moore [off-list ref] wrote:

On Wed, May 7, 2025 at 7:12 AM [off-list ref] wrote:

quoted

From: Jinliang Zheng <redacted>

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.

Thank you for your reply. :)

To clarify, the reproducer code never invokes security_add_hooks(), thus
this clearly does not constitute loading a new LSM. 

However, if you believe the current approach might be misinterpreted,
my v3 patch is available for consideration:
- https://lore.kernel.org/all/20250508140438.648533-2-alexjlzheng@tencent.com/ (local)

While I personally find the v2 reproducer more readable and straightforward,
I fully defer to your judgment on this matter.

thanks,
Jinliang Zheng. :)

quoted

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?

quoted

Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
Signed-off-by: Jinliang Zheng <redacted>
---
changelog:
v2: Modify the commit message to make it clearer
v1: https://lore.kernel.org/all/20250426150931.2840-1-alexjlzheng@tencent.com/ (local)
---
 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