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

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

[alexjlzheng]

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!

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

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

[Paul Moore]

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!

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

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.

> 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

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

[Jinliang Zheng]

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

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

[Fan Wu]

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.

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

-Fan

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

[Jinliang Zheng]

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