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

[Jinliang Zheng <alexjlzheng@gmail.com>]

On Fri, 25 Apr 2025 18:06:32 -0400, Paul Moore wrote:
> On Fri, Apr 25, 2025 at 5:25 AM <alexjlzheng@gmail.com> wrote:
> >
> > From: Jinliang Zheng <alexjlzheng@tencent.com>
> >
> > Consider the following module code:
> >
> >   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!

Thank you for your reply. :)

> 
> A quick procedural note, and you may have gotten an email about this
> from the stable kernel folks already, you generally shouldn't add the
> stable alias to your emails directly.  You may want to look at the
> kernel docs on the stable kernel if you haven't already:
> 
> * https://docs.kernel.org/process/stable-kernel-rules.html

Sorry for that, I will read it. And thank you for your pointing it out.

> 
> Beyond that, we don't currently support dynamically loading or
> unloading LSMs so the immediate response to the reproducer above is
> "don't do that, we don't support it" :)  However, if you see a similar
> problem with a LSM properly registered with the running kernel please
> let us know.

I don't think that not supporting dynamic loading/unloading of LSMs means
that directories/files under securityfs cannot be dynamically added/deleted.

The example code in the commit message is just to quickly show the problem,
not the actual usage scenario.

I'm not sure whether existing LSMs have dynamic addition/deletion of files,
but I don't think we should prohibit these operations.

Moreover, since securityfs provides the securityfs_remove() interface, it
is necessary to handle the deletion of dentry whenever it is used. What's
more, we have EXPORT_SYMBOL_GPL(securityfs_remove).

(By the way, the reason why I noticed this problem is because I needed to
dynamically create/delete configuration directories/files when implementing
an LSM. Of course, I am not dynamically loading/unloading LSM, but
dynamically adding/deleting directories/files under securityfs according to
the status during LSM operation.)

Therefore, I think we need this patch and strongly recommend it. At least,
it has no harm. Hahahaha

thanks,
Jinliang Zheng :)

> 
> > Fix this by adding d_delete() in securityfs_remove().
> >
> > Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
> > Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> >  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