On Tue, May 10, 2022 at 05:51:07PM +0200, Christian Brauner wrote:

On Tue, May 10, 2022 at 09:10:25AM -0500, Serge Hallyn wrote:

quoted

On Tue, May 10, 2022 at 12:25:25PM +0200, Christian Brauner wrote:

quoted

On Mon, May 09, 2022 at 02:54:14PM -0500, Serge Hallyn wrote:

quoted

On Wed, Apr 20, 2022 at 10:06:08AM -0400, Stefan Berger wrote:

quoted

From: Christian Brauner <brauner@kernel.org>

When securityfs creates a new file or directory via
securityfs_create_dentry() it will take an additional reference on the
newly created dentry after it has attached the new inode to the new
dentry and added it to the hashqueues.
If we contrast this with debugfs which has the same underlying logic as
securityfs. It uses a similar pairing as securityfs. Where securityfs
has the securityfs_create_dentry() and securityfs_remove() pairing,
debugfs has the __debugfs_create_file() and debugfs_remove() pairing.

In contrast to securityfs, debugfs doesn't take an additional reference
on the newly created dentry in __debugfs_create_file() which would need
to be put in debugfs_remove().

The additional dget() isn't a problem per se. In the current
implementation of securityfs each created dentry pins the filesystem via

Is 'via' an extra word here or is there a missing word?

I'll delay the rest of my response as the missing word may answer my
remaining question :)

It can be both. It should either be removed or it should be followed by
"securityfs_create_dentry()". securityfs_create_dentry() takes two
references one in lookup_one_len() and another one explicitly via
dget(). The latter one isn't needed. Some of that has been covered in an
earlier thread:
https://lore.kernel.org/lkml/20220105101815.ldsm4s5yx7pmuiil@wittgenstein (local)

Yes, I saw that two references were being taken.  And near as I can tell,
the second one was never being dropped.  So if you tell me that before this
patch the dentries are never freed, then I'm happy.  Otherwise, I'm
bothered the fact that no matching dput is being deleted in the code (to
match the extra dget being removed).  So where is the code where the final
dput was happening, and is it the d_delete() you're adding which is making
it so that that dput won't be called now?

* So consider mounting securityfs _without this patch applied_:

  mount -t securityfs /sfs
  
  and assume we only have a single user that creates a file "foo" via
  
  securityfs_create_file()
  {
  	lookup_one_len();	// first dget()
  	dget();			// second dget()
  }
  
  now assume that user at some point calls
  
  void securityfs_remove()
  {
  	if (d_is_dir(dentry))
  		simple_rmdir(dir, dentry);	// first dput()
  	else
  		simple_unlink(dir, dentry);	// first dput()
  	dput(dentry);				// second dput()
  }

* Now consider mounting securityfs _with this patch applied_:

  securityfs_create_file()
  {
  	lookup_one_len();	// first dget()
  }
  
  void securityfs_remove()
  {
  	dget();					// second dget() 
  	if (d_is_dir(dentry))
  		simple_rmdir(dir, dentry);	// first dput()
  	else
  		simple_unlink(dir, dentry);	// first dput()
  	dput(dentry);				// second dput()
  }

Oh - I was thinking about the new d_delete, but I guess that doesn't matter.

thanks,
-serge