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

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

[alexjlzheng]

From: Jinliang Zheng <alexjlzheng@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!

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().

Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
Signed-off-by: Jinliang Zheng <alexjlzheng@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

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

[Paul Moore]

On Wed, May 7, 2025 at 7:12 AM <alexjlzheng@gmail.com> wrote:
> From: Jinliang Zheng <alexjlzheng@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@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

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

[Al Viro]

On Wed, May 07, 2025 at 04:10:11PM -0400, Paul Moore wrote:
> > 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?

It's a bit trickier than that (there are interesting issues around
efi_secret_unlink() nonsense, as well as insane refcounting grabbing
two references where only one is needed to pin the damn thing)...

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

[Paul Moore]

On Wed, May 7, 2025 at 5:23 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, May 07, 2025 at 04:10:11PM -0400, Paul Moore wrote:
> > > 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?
>
> It's a bit trickier than that (there are interesting issues around
> efi_secret_unlink() nonsense, as well as insane refcounting grabbing
> two references where only one is needed to pin the damn thing)...

Fun :/

In that case, what do you think of the change suggested here by
Jinliang Zheng where we add a d_delete() to the existing
securityfs_remove() implementation?

-- 
paul-moore.com

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

[alexjlzheng]

From: Jinliang Zheng <alexjlzheng@tencent.com>

Consider the following execution flow:

  Thread 0: securityfs_create_dir("A")
  Thread 1: cd /sys/kernel/security/A           <- we hold 'A'
  Thread 0: securityfs_remove(dentry)           <- 'A' don't go away
  Thread 0: securityfs_create_dir("A")          <- Failed: File exists!

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().

Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
---
changelog:
v3: Modify the commit message to avoid readers mistakenly thinking that the LSM is being dynamically loaded
v2: https://lore.kernel.org/all/20250507111204.2585739-1-alexjlzheng@tencent.com/
v1: https://lore.kernel.org/all/20250425092548.6828-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

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

[Jinliang Zheng]

On Wed, 7 May 2025 16:10:11 -0400, Paul Moore <paul@paul-moore.com> wrote:
> On Wed, May 7, 2025 at 7:12 AM <alexjlzheng@gmail.com> wrote:
> > From: Jinliang Zheng <alexjlzheng@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.

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/

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

thanks,
Jinliang Zheng. :)

> 
> > 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@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

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

[Fan Wu]

On Thu, May 8, 2025 at 7:11 AM <alexjlzheng@gmail.com> wrote:
>
> From: Jinliang Zheng <alexjlzheng@tencent.com>
>
> Consider the following execution flow:
>
>   Thread 0: securityfs_create_dir("A")
>   Thread 1: cd /sys/kernel/security/A           <- we hold 'A'
>   Thread 0: securityfs_remove(dentry)           <- 'A' don't go away
>   Thread 0: securityfs_create_dir("A")          <- Failed: File exists!
>
> 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().
>
> Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
> Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> ---
> changelog:
> v3: Modify the commit message to avoid readers mistakenly thinking that the LSM is being dynamically loaded
> v2: https://lore.kernel.org/all/20250507111204.2585739-1-alexjlzheng@tencent.com/
> v1: https://lore.kernel.org/all/20250425092548.6828-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
>
>

Since this could impact efi_secret_unlink(), I would suggest adding linux-efi.

-Fan

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

[Jinliang Zheng]

On Thu, 8 May 2025 18:55:30 -0700, Fan Wu <wufan@kernel.org> wrote:
> On Thu, May 8, 2025 at 7:11 AM <alexjlzheng@gmail.com> wrote:
> >
> > From: Jinliang Zheng <alexjlzheng@tencent.com>
> >
> > Consider the following execution flow:
> >
> >   Thread 0: securityfs_create_dir("A")
> >   Thread 1: cd /sys/kernel/security/A           <- we hold 'A'
> >   Thread 0: securityfs_remove(dentry)           <- 'A' don't go away
> >   Thread 0: securityfs_create_dir("A")          <- Failed: File exists!
> >
> > 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().
> >
> > Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
> > Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> > ---
> > changelog:
> > v3: Modify the commit message to avoid readers mistakenly thinking that the LSM is being dynamically loaded
> > v2: https://lore.kernel.org/all/20250507111204.2585739-1-alexjlzheng@tencent.com/
> > v1: https://lore.kernel.org/all/20250425092548.6828-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
> >
> >
> 
> Since this could impact efi_secret_unlink(), I would suggest adding linux-efi.

Thank you for your reply. :)

Did you mean cc to linux-efi?

thanks,
Jinliang Zheng.

> 
> -Fan

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

[Jinliang Zheng]

On Thu, 8 May 2025 18:55:30 -0700, Fan Wu <wufan@kernel.org> wrote:
> On Thu, May 8, 2025 at 7:11 AM <alexjlzheng@gmail.com> wrote:
> >
> > From: Jinliang Zheng <alexjlzheng@tencent.com>
> >
> > Consider the following execution flow:
> >
> >   Thread 0: securityfs_create_dir("A")
> >   Thread 1: cd /sys/kernel/security/A           <- we hold 'A'
> >   Thread 0: securityfs_remove(dentry)           <- 'A' don't go away
> >   Thread 0: securityfs_create_dir("A")          <- Failed: File exists!
> >
> > 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().
> >
> > Fixes: b67dbf9d4c198 ("[PATCH] add securityfs for all LSMs to use")
> > Signed-off-by: Jinliang Zheng <alexjlzheng@tencent.com>
> > ---
> > changelog:
> > v3: Modify the commit message to avoid readers mistakenly thinking that the LSM is being dynamically loaded
> > v2: https://lore.kernel.org/all/20250507111204.2585739-1-alexjlzheng@tencent.com/
> > v1: https://lore.kernel.org/all/20250425092548.6828-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
> >
> >
> 
> Since this could impact efi_secret_unlink(), I would suggest adding linux-efi.

Thank you for your reply. :)

Did you mean cc to linux-efi?

thanks,
Jinliang Zheng.

> 
> -Fan

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

[Al Viro]

On Thu, May 08, 2025 at 10:04:39PM +0800, alexjlzheng@gmail.com wrote:

> 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().

This is not a fix.  First and foremost, securityfs_recursive_remove()
does *not* just call __d_drop() - it calls simple_recursive_removal(),
which takes care to evict anything possibly mounted on those suckers.

Your variant trivially turns into a mount leak - just bind anything
on that thing and trigger removal.

<a bit of a rant follows; if it offends somebody, feel free to report
to CoC committee>

What's more, securityfs object creation is... special.  It does, for
some odd reason, leave you dentry with refcount *two*.  For no reason
whatsoever, as far as I can tell.

securityfs_remove() matches that; securityfs_recursive_remove(),
as far as I can tell, should simply leak them.  That's from RTFS
alone, but I don't see how it could possibly *not* happen -
securityfs_create_file() is a call of securityfs_create_dentry(),
which
	* calls lookup_one_len(), getting a negative dentry with
refcount 1.
	* verifies it's negative
	* gets a new inode
	* does d_instantiate(), attaching it to dentry.
	* does dget(), for some unspeakable reason.  Refcount is 2 now.
	* returns that dentry to caller.

policyfs stuff calls securityfs_create_dir() (which is a wrapper for
securityfs_create_file(), with nothing extra done to refcounts),
then populates it with a bunch of files, all with the same refcount
weirdness.

Result: directory dentry with refcount 2 + number of children and
a bunch of children, each with refcount 2.

Now, securityfs_recursive_remove() calls simple_recursive_removal(),
which will strip _one_ off the refcount of each dentry in that tree.
Yes, they are all unhashed and any stuff mounted on them is kicked
out, but you have a massive dentry leak now - all of those dentries
have refcount at least 1.

I'm not blaming securityfs_recursive_remove() authors - it *should*
have worked; their only fault is that they hadn't guessed that
object creation on securityfs happens to be that strange.

Another special snowflake is efi_secret_unlink() - it calls
securityfs_remove(), which is needed instead of simple_unlink()
since
	* that double refcount needs to be dropped
	* having internal mount pinned is something that needs
to be undone, innit?

Of course, it runs afoul of the parent being locked, but nevermind that -
it just unlocks and relocks it, 'cuz what can go wrong?  That - instead
of discussing that with VFS and filesystem folks.

As for "what can go wrong"...  Consider what happens if another process
calls unlink() on the same file, just before the first one drops the
lock on parent.  Parent found, process 2 blocked on the lock.  Process 1
unlocks that lock and loses CPU.  Process 2 runs and tries to lock the
victim; blocks since process 1 is still holding it locked.  Process 1,
in securityfs_remove(): blocks trying to lock the parent.  AB-BA deadlock.

Oh, well...

Anyway, the reasons for securityfs_remove() use there are real deficiencies
of securityfs.  Weird shit with refcounts is one thing; internal mount
pinning is a bit more subtle, but it's also solvable.

The thing is, objects on securityfs never change parents.  So you only
need to pin for subdirectories of root - everything deeper will be
automatically fine.  And that kills the second reason for those games.
With that dealt with, efi_secret_unlink() can simply call simple_unlink()
instead of those games.

After that securityfs_remove() can become an alias for
securityfs_recursive_remove() (or the other way round, preferably).

BTW,
        d_inode(dent)->i_op = &efi_secret_dir_inode_operations;
in the same drivers/virt/coco is also nasty - you don't change the method
table on an object that is already exposed in shared data structures.
Basic multithreaded programming safety rules...  Yes, _that_ probably runs
too early in the boot for anything to hit it, so it's not a security hole,
but the same "what if somebody copies that code and gets screwed" applies
there...  If anything, that points to the need of securityfs_create_dir()
variant that would override ->i_op, which should've been discussed back
when the thing had been merged.

</rant>

I have fixes for some of that crap done on top of tree-in-dcache series;
give me an hour or two and I'll separate those and rebase to mainline...

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

[Al Viro]

On Fri, May 09, 2025 at 04:23:26AM +0100, Al Viro wrote:

> I have fixes for some of that crap done on top of tree-in-dcache series;
> give me an hour or two and I'll separate those and rebase to mainline...

Completely untested:
git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #untested.securityfs

on top of v6.15-rc5.  And I'm serious about the "untested" part - it builds
with allmodconfig, but that's all I've checked.  So treat that as an outline
of what could be done, but don't use as-is without serious testing.

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

[Al Viro]

On Fri, May 09, 2025 at 05:37:12AM +0100, Al Viro wrote:
> On Fri, May 09, 2025 at 04:23:26AM +0100, Al Viro wrote:
> 
> > I have fixes for some of that crap done on top of tree-in-dcache series;
> > give me an hour or two and I'll separate those and rebase to mainline...
> 
> Completely untested:
> git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #untested.securityfs
> 
> on top of v6.15-rc5.  And I'm serious about the "untested" part - it builds
> with allmodconfig, but that's all I've checked.  So treat that as an outline
> of what could be done, but don't use as-is without serious testing.

PS: I'm really, really serious - do not use without a serious review; this
is a rebase of a branch last touched back in March and it was a part of
long tail, with pretty much zero testing even back then.

Patches are simple enough to have a chance to be somewhere in the vicinity
of being correct, but that's all I can promise.

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

[Paul Moore]

On Fri, May 9, 2025 at 12:46 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, May 09, 2025 at 05:37:12AM +0100, Al Viro wrote:
> > On Fri, May 09, 2025 at 04:23:26AM +0100, Al Viro wrote:
> >
> > > I have fixes for some of that crap done on top of tree-in-dcache series;
> > > give me an hour or two and I'll separate those and rebase to mainline...
> >
> > Completely untested:
> > git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #untested.securityfs
> >
> > on top of v6.15-rc5.  And I'm serious about the "untested" part - it builds
> > with allmodconfig, but that's all I've checked.  So treat that as an outline
> > of what could be done, but don't use as-is without serious testing.
>
> PS: I'm really, really serious - do not use without a serious review; this
> is a rebase of a branch last touched back in March and it was a part of
> long tail, with pretty much zero testing even back then.
>
> Patches are simple enough to have a chance to be somewhere in the vicinity
> of being correct, but that's all I can promise.

Fair enough, although unfortunately I don't think anyone has anything
close to a securityfs test suite so I suspect this may languish on the
lists for a bit unless someone has the cycles to pick it up and
properly test it.

I haven't compared the patches you posted on-list with the stuff in
the tree above, but based on the timestamps I'm guessing the on-list
patches are simply the ones from the tree above?

-- 
paul-moore.com

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

[Al Viro]

On Mon, May 12, 2025 at 05:19:39PM -0400, Paul Moore wrote:
> On Fri, May 9, 2025 at 12:46 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Fri, May 09, 2025 at 05:37:12AM +0100, Al Viro wrote:
> > > On Fri, May 09, 2025 at 04:23:26AM +0100, Al Viro wrote:
> > >
> > > > I have fixes for some of that crap done on top of tree-in-dcache series;
> > > > give me an hour or two and I'll separate those and rebase to mainline...
> > >
> > > Completely untested:
> > > git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #untested.securityfs
> > >
> > > on top of v6.15-rc5.  And I'm serious about the "untested" part - it builds
> > > with allmodconfig, but that's all I've checked.  So treat that as an outline
> > > of what could be done, but don't use as-is without serious testing.
> >
> > PS: I'm really, really serious - do not use without a serious review; this
> > is a rebase of a branch last touched back in March and it was a part of
> > long tail, with pretty much zero testing even back then.
> >
> > Patches are simple enough to have a chance to be somewhere in the vicinity
> > of being correct, but that's all I can promise.
> 
> Fair enough, although unfortunately I don't think anyone has anything
> close to a securityfs test suite so I suspect this may languish on the
> lists for a bit unless someone has the cycles to pick it up and
> properly test it.
> 
> I haven't compared the patches you posted on-list with the stuff in
> the tree above, but based on the timestamps I'm guessing the on-list
> patches are simply the ones from the tree above?

git format-patch output for that branch...

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

[Fan Wu]

On Mon, May 12, 2025 at 2:19 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, May 9, 2025 at 12:46 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Fri, May 09, 2025 at 05:37:12AM +0100, Al Viro wrote:
> > > On Fri, May 09, 2025 at 04:23:26AM +0100, Al Viro wrote:
> > >
> > > > I have fixes for some of that crap done on top of tree-in-dcache series;
> > > > give me an hour or two and I'll separate those and rebase to mainline...
> > >
> > > Completely untested:
> > > git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git #untested.securityfs
> > >
> > > on top of v6.15-rc5.  And I'm serious about the "untested" part - it builds
> > > with allmodconfig, but that's all I've checked.  So treat that as an outline
> > > of what could be done, but don't use as-is without serious testing.
> >
> > PS: I'm really, really serious - do not use without a serious review; this
> > is a rebase of a branch last touched back in March and it was a part of
> > long tail, with pretty much zero testing even back then.
> >
> > Patches are simple enough to have a chance to be somewhere in the vicinity
> > of being correct, but that's all I can promise.
>
> Fair enough, although unfortunately I don't think anyone has anything
> close to a securityfs test suite so I suspect this may languish on the
> lists for a bit unless someone has the cycles to pick it up and
> properly test it.
>

Since it's me who added the recursive remove, I'm interested in
helping get the fix tested and verified. However, I might not have
enough cycles in the near future. Happy to let someone else test it if
they have bandwidth.

-Fan