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

[Jinliang Zheng <alexjlzheng@gmail.com>]

On Fri, 25 Apr 2025 22:57:08 -0700, Fan Wu <wufan@kernel.org> wrote:
> On Fri, Apr 25, 2025 at 9:15 PM Jinliang Zheng <alexjlzheng@gmail.com> wrote:
> >
> > 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 :)
> >
> 
> We have added securityfs_recursive_remove() for this purpose.

Thank you for your reply. :)

Yes, but I think securityfs_recursive_remove() is not equal to
securityfs_remove() + d_delete(), it has its own extra work. Therefore, I
think it is better to add the work of d_delete() directly in
securityfs_remove().

There is no reason to do __d_drop() only when deleting files recursively
and not do __d_drop() when deleting files non-recursively, which seems a
bit strange.

thanks,
Jinliang Zheng. :)

> 
> To the best of my knowledge, IPE is the only LSM that will delete
> dentry during normal operation.
> 
> -Fan