[RFC PATCH 00/14] configfs cleanups and fixes

[RFC PATCH 00/14] configfs cleanups and fixes

[Al Viro]

A bunch of configfs patches; fix for UAF caught while doing dentry memory
safety audit + stuff cherry-picked from old branches, up to the minimal
switch to d_make_persistent().

There's a *lot* more still rotting in the old configfs branches (the
earliest stuff there is from back in 2018), but I would prefer to deal
with that gradually - the last thing I want is yet another variant of
the same pile sitting around to be sorted out someday ;-/

The stuff in this pile:
	* minimal UAF fix for configfs_lookup() (ancient, but you'd
need the allocation of in-core inode to fail at the right time, and if
the things are controllable for attacker to that degree, the system is
already FUBAR)
	* obvious cleanup in configfs_mkdir() - no need to do a manual
analogue of take_dentry_name_snapshot() when we are not going to modify
the copy we make.
	* sorting out the argument types for a bunch of "iterate over
configfs_dirent subtree" functions; being able to switch back and forth
between dentry and configfs_dirent (via ->d_fsdata and ->s_dentry resp.)
doesn't mean we need to without a good reason.	Passing dentry only to
use it for ->d_fsdata isn't one, especially when some callers get it as
some_configfs_dirent->s_dentry in the first place.
	* partially untangle creation and removal of directory trees;
ideally I'd like to use simple_recursive_removal() for the latter
and create-then-move-in-place for the former, but that takes a _lot_
of preliminary massage.  For now just get it to the point where we can
regularize the refcount mess - some objects are pinned once, some twice,
for no good reason.
	* switch to d_make_persistent() on creation side with
simple_rmdir() and simple_unlink() on removals.
	* sanitize attribute removal - we don't want full
__simple_unlink() there *and* we ought to lock the inode for i_nlink
updates.
	* do *not* update the timestamps of directory when looking an
attribute up; stat foo/bar shouldn't update the modification time of foo.


	Fixes aside, the main result is that configfs is finally switched
to tree-in-dcache machinery.  It's *not* making use of recursive removal
helpers yet, and it still does the bloody awful "build subtree in full
sight of userland, with possibility of failure halfway through and need
to unroll" that forces the locking model from hell; dealing with that
is a separate patch series, once this one is out of the way.  However,
it is using DCACHE_PERSISTENT properly now.  And apparmorfs is the sole
remaining user of __simple_{unlink,rmdir}() at that point.

	This branch (7.1-rc4-based) lives in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.configfs
Individual patches in followups.  It has been lightly tested, but it
obviously needs more beating.

	Review and testing would be very welcome.

Al Viro (14):
  configfs_lookup(): don't leave ->s_dentry dangling on failure
  configfs_mkdir(): use take_dentry_name_snapshot()
  configfs_detach_prep(): pass configfs_dirent instead of dentry
  configfs_depend_prep(): pass configfs_dirent instead of dentry
  configfs_do_depend_item(): pass configfs_dirent instead of dentry
  configfs_detach_rollback(): pass configfs_dirent instead of dentry
  populate_group(): move cleanup on failure to the sole caller
  populate_attrs(): move cleanup to the sole caller
  configfs_remove_dir(), detach_attrs(): switch to passing dentry
  switch configfs_detach_{group,item}() to passing dentry
  configfs: dentry refcount needs to be pinned only once
  configfs: mark pinned dentries persistent
  kill configfs_drop_dentry()
  configfs_create(): lift parent timestamp updates into callers

 fs/configfs/configfs_internal.h |   1 -
 fs/configfs/dir.c               | 225 ++++++++++++++------------------
 fs/configfs/inode.c             |  25 ----
 fs/configfs/symlink.c           |   3 +-
 4 files changed, 97 insertions(+), 157 deletions(-)

-- 
2.47.3

[RFC PATCH 01/14] configfs_lookup(): don't leave ->s_dentry dangling on failure

[Al Viro]

Normally ->s_dentry is cleared when dentry it's pointing to becomes
negative (on eviction, realistically).  However, that only happens
if dentry gets to be positive in the first place; in case of inode
allocation failure dentry never becomes positive, so ->d_iput()
is not called at all.

We do part of what normally would've been done by configfs_d_iput()
(dropping the reference to configfs_dirent) manually, but we do
not clear ->s_dentry there.  Sloppy as it is, it does not matter in
case of configfs_create_{dir,link}() - there configfs_dirent does
not survive dropping the sole reference to it.

However, for configfs_lookup() it *does* survive, with a dangling
pointer to soon to be freed dentry sitting it its ->s_dentry.

Subsequent getdents(2) in that directory will end up dereferencing
that pointer in order to pick the inode number.  Use after free...

This is the minimal fix; the right approach is to set the linkage
between dentry and configfs_dirent only after we know that we have
an inode, but that takes more surgery and the bug had been there
since 2006, so...

Fixes: 3d0f89bb1694 ("configfs: Add permission and ownership to configfs objects") # 2.6.16-rc3
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 362b6ff9b908..e84483a0836d 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -486,6 +486,9 @@ static struct dentry * configfs_lookup(struct inode *dir,
 
 			inode = configfs_create(dentry, mode);
 			if (IS_ERR(inode)) {
+				spin_lock(&configfs_dirent_lock);
+				sd->s_dentry = NULL;
+				spin_unlock(&configfs_dirent_lock);
 				configfs_put(sd);
 				return ERR_CAST(inode);
 			}
-- 
2.47.3

Re: [RFC PATCH 01/14] configfs_lookup(): don't leave ->s_dentry dangling on failure

[Jan Kara]

On Tue 19-05-26 08:06:20, Al Viro wrote:
> Normally ->s_dentry is cleared when dentry it's pointing to becomes
> negative (on eviction, realistically).  However, that only happens
> if dentry gets to be positive in the first place; in case of inode
> allocation failure dentry never becomes positive, so ->d_iput()
> is not called at all.
> 
> We do part of what normally would've been done by configfs_d_iput()
> (dropping the reference to configfs_dirent) manually, but we do
> not clear ->s_dentry there.  Sloppy as it is, it does not matter in
> case of configfs_create_{dir,link}() - there configfs_dirent does
> not survive dropping the sole reference to it.
> 
> However, for configfs_lookup() it *does* survive, with a dangling
> pointer to soon to be freed dentry sitting it its ->s_dentry.
> 
> Subsequent getdents(2) in that directory will end up dereferencing
> that pointer in order to pick the inode number.  Use after free...
> 
> This is the minimal fix; the right approach is to set the linkage
> between dentry and configfs_dirent only after we know that we have
> an inode, but that takes more surgery and the bug had been there
> since 2006, so...
> 
> Fixes: 3d0f89bb1694 ("configfs: Add permission and ownership to configfs objects") # 2.6.16-rc3
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 362b6ff9b908..e84483a0836d 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -486,6 +486,9 @@ static struct dentry * configfs_lookup(struct inode *dir,
>  
>  			inode = configfs_create(dentry, mode);
>  			if (IS_ERR(inode)) {
> +				spin_lock(&configfs_dirent_lock);
> +				sd->s_dentry = NULL;
> +				spin_unlock(&configfs_dirent_lock);
>  				configfs_put(sd);
>  				return ERR_CAST(inode);
>  			}
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH 01/14] configfs_lookup(): don't leave ->s_dentry dangling on failure

[Breno Leitao]

On Tue, May 19, 2026 at 08:06:20AM +0100, Al Viro wrote:
> Normally ->s_dentry is cleared when dentry it's pointing to becomes
> negative (on eviction, realistically).  However, that only happens
> if dentry gets to be positive in the first place; in case of inode
> allocation failure dentry never becomes positive, so ->d_iput()
> is not called at all.
> 
> We do part of what normally would've been done by configfs_d_iput()
> (dropping the reference to configfs_dirent) manually, but we do
> not clear ->s_dentry there.  Sloppy as it is, it does not matter in
> case of configfs_create_{dir,link}() - there configfs_dirent does
> not survive dropping the sole reference to it.
> 
> However, for configfs_lookup() it *does* survive, with a dangling
> pointer to soon to be freed dentry sitting it its ->s_dentry.
> 
> Subsequent getdents(2) in that directory will end up dereferencing
> that pointer in order to pick the inode number.  Use after free...
> 
> This is the minimal fix; the right approach is to set the linkage
> between dentry and configfs_dirent only after we know that we have
> an inode, but that takes more surgery and the bug had been there
> since 2006, so...
> 
> Fixes: 3d0f89bb1694 ("configfs: Add permission and ownership to configfs objects") # 2.6.16-rc3
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Reviewed-by: Breno Leitao <leitao@debian.org>

[RFC PATCH 02/14] configfs_mkdir(): use take_dentry_name_snapshot()

[Al Viro]

Note that neither ->make_group() nor ->make_item() are allowed to modify
the string passed to them - the argument is const char *.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index e84483a0836d..662dcd7d1bf0 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1301,7 +1301,11 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	const struct config_item_type *type;
 	struct module *subsys_owner = NULL, *new_item_owner = NULL;
 	struct configfs_fragment *frag;
-	char *name;
+	struct name_snapshot n;
+	const char *name;
+
+	take_dentry_name_snapshot(&n, dentry);
+	name = n.name.name;
 
 	sd = dentry->d_parent->d_fsdata;
 
@@ -1353,14 +1357,6 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 		goto out_put;
 	}
 
-	name = kmalloc(dentry->d_name.len + 1, GFP_KERNEL);
-	if (!name) {
-		ret = -ENOMEM;
-		goto out_subsys_put;
-	}
-
-	snprintf(name, dentry->d_name.len + 1, "%s", dentry->d_name.name);
-
 	mutex_lock(&subsys->su_mutex);
 	if (type->ct_group_ops->make_group) {
 		group = type->ct_group_ops->make_group(to_config_group(parent_item), name);
@@ -1382,7 +1378,6 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	}
 	mutex_unlock(&subsys->su_mutex);
 
-	kfree(name);
 	if (ret) {
 		/*
 		 * If ret != 0, then link_obj() was never called.
@@ -1469,6 +1464,7 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	put_fragment(frag);
 
 out:
+	release_dentry_name_snapshot(&n);
 	return ERR_PTR(ret);
 }
 
-- 
2.47.3

Re: [RFC PATCH 02/14] configfs_mkdir(): use take_dentry_name_snapshot()

[Jan Kara]

On Tue 19-05-26 08:06:21, Al Viro wrote:
> Note that neither ->make_group() nor ->make_item() are allowed to modify
> the string passed to them - the argument is const char *.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index e84483a0836d..662dcd7d1bf0 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -1301,7 +1301,11 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	const struct config_item_type *type;
>  	struct module *subsys_owner = NULL, *new_item_owner = NULL;
>  	struct configfs_fragment *frag;
> -	char *name;
> +	struct name_snapshot n;
> +	const char *name;
> +
> +	take_dentry_name_snapshot(&n, dentry);
> +	name = n.name.name;
>  
>  	sd = dentry->d_parent->d_fsdata;
>  
> @@ -1353,14 +1357,6 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  		goto out_put;
>  	}
>  
> -	name = kmalloc(dentry->d_name.len + 1, GFP_KERNEL);
> -	if (!name) {
> -		ret = -ENOMEM;
> -		goto out_subsys_put;
> -	}
> -
> -	snprintf(name, dentry->d_name.len + 1, "%s", dentry->d_name.name);
> -
>  	mutex_lock(&subsys->su_mutex);
>  	if (type->ct_group_ops->make_group) {
>  		group = type->ct_group_ops->make_group(to_config_group(parent_item), name);
> @@ -1382,7 +1378,6 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	}
>  	mutex_unlock(&subsys->su_mutex);
>  
> -	kfree(name);
>  	if (ret) {
>  		/*
>  		 * If ret != 0, then link_obj() was never called.
> @@ -1469,6 +1464,7 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>  	put_fragment(frag);
>  
>  out:
> +	release_dentry_name_snapshot(&n);
>  	return ERR_PTR(ret);
>  }
>  
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH 02/14] configfs_mkdir(): use take_dentry_name_snapshot()

[Breno Leitao]

On Tue, May 19, 2026 at 08:06:21AM +0100, Al Viro wrote:
> Note that neither ->make_group() nor ->make_item() are allowed to modify
> the string passed to them - the argument is const char *.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Reviewed-by: Breno Leitao <leitao@debian.org>

[RFC PATCH 03/14] configfs_detach_prep(): pass configfs_dirent instead of dentry

[Al Viro]

The only thing it uses the argument for is its ->d_fsdata and
all callers have that already available.

Note that in the recursive call we are dealing with a (sub)directory
configfs_dirent, and for those ->s_dentry->d_fsdata points back
to configfs_dirent itself.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 662dcd7d1bf0..0c071252c9d4 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -516,9 +516,8 @@ static struct dentry * configfs_lookup(struct inode *dir,
  * If there is an error, the caller will reset the flags via
  * configfs_detach_rollback().
  */
-static int configfs_detach_prep(struct dentry *dentry, struct dentry **wait)
+static int configfs_detach_prep(struct configfs_dirent *parent_sd, struct dentry **wait)
 {
-	struct configfs_dirent *parent_sd = dentry->d_fsdata;
 	struct configfs_dirent *sd;
 	int ret;
 
@@ -546,7 +545,7 @@ static int configfs_detach_prep(struct dentry *dentry, struct dentry **wait)
 			 * Yup, recursive.  If there's a problem, blame
 			 * deep nesting of default_groups
 			 */
-			ret = configfs_detach_prep(sd->s_dentry, wait);
+			ret = configfs_detach_prep(sd, wait);
 			if (!ret)
 				continue;
 		} else
@@ -1512,7 +1511,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 		 */
 		ret = sd->s_dependent_count ? -EBUSY : 0;
 		if (!ret) {
-			ret = configfs_detach_prep(dentry, &wait);
+			ret = configfs_detach_prep(sd, &wait);
 			if (ret)
 				configfs_detach_rollback(dentry);
 		}
@@ -1809,7 +1808,7 @@ void configfs_unregister_group(struct config_group *group)
 
 	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
 	spin_lock(&configfs_dirent_lock);
-	configfs_detach_prep(dentry, NULL);
+	configfs_detach_prep(sd, NULL);
 	spin_unlock(&configfs_dirent_lock);
 
 	configfs_detach_group(&group->cg_item);
@@ -1956,7 +1955,7 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
 	inode_lock_nested(d_inode(dentry), I_MUTEX_CHILD);
 	mutex_lock(&configfs_symlink_mutex);
 	spin_lock(&configfs_dirent_lock);
-	if (configfs_detach_prep(dentry, NULL)) {
+	if (configfs_detach_prep(sd, NULL)) {
 		pr_err("Tried to unregister non-empty subsystem!\n");
 	}
 	spin_unlock(&configfs_dirent_lock);
-- 
2.47.3

Re: [RFC PATCH 03/14] configfs_detach_prep(): pass configfs_dirent instead of dentry

[Jan Kara]

On Tue 19-05-26 08:06:22, Al Viro wrote:
> The only thing it uses the argument for is its ->d_fsdata and
> all callers have that already available.
> 
> Note that in the recursive call we are dealing with a (sub)directory
> configfs_dirent, and for those ->s_dentry->d_fsdata points back
> to configfs_dirent itself.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 662dcd7d1bf0..0c071252c9d4 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -516,9 +516,8 @@ static struct dentry * configfs_lookup(struct inode *dir,
>   * If there is an error, the caller will reset the flags via
>   * configfs_detach_rollback().
>   */
> -static int configfs_detach_prep(struct dentry *dentry, struct dentry **wait)
> +static int configfs_detach_prep(struct configfs_dirent *parent_sd, struct dentry **wait)
>  {
> -	struct configfs_dirent *parent_sd = dentry->d_fsdata;
>  	struct configfs_dirent *sd;
>  	int ret;
>  
> @@ -546,7 +545,7 @@ static int configfs_detach_prep(struct dentry *dentry, struct dentry **wait)
>  			 * Yup, recursive.  If there's a problem, blame
>  			 * deep nesting of default_groups
>  			 */
> -			ret = configfs_detach_prep(sd->s_dentry, wait);
> +			ret = configfs_detach_prep(sd, wait);
>  			if (!ret)
>  				continue;
>  		} else
> @@ -1512,7 +1511,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
>  		 */
>  		ret = sd->s_dependent_count ? -EBUSY : 0;
>  		if (!ret) {
> -			ret = configfs_detach_prep(dentry, &wait);
> +			ret = configfs_detach_prep(sd, &wait);
>  			if (ret)
>  				configfs_detach_rollback(dentry);
>  		}
> @@ -1809,7 +1808,7 @@ void configfs_unregister_group(struct config_group *group)
>  
>  	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
>  	spin_lock(&configfs_dirent_lock);
> -	configfs_detach_prep(dentry, NULL);
> +	configfs_detach_prep(sd, NULL);
>  	spin_unlock(&configfs_dirent_lock);
>  
>  	configfs_detach_group(&group->cg_item);
> @@ -1956,7 +1955,7 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
>  	inode_lock_nested(d_inode(dentry), I_MUTEX_CHILD);
>  	mutex_lock(&configfs_symlink_mutex);
>  	spin_lock(&configfs_dirent_lock);
> -	if (configfs_detach_prep(dentry, NULL)) {
> +	if (configfs_detach_prep(sd, NULL)) {
>  		pr_err("Tried to unregister non-empty subsystem!\n");
>  	}
>  	spin_unlock(&configfs_dirent_lock);
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH 03/14] configfs_detach_prep(): pass configfs_dirent instead of dentry

[Breno Leitao]

On Tue, May 19, 2026 at 08:06:22AM +0100, Al Viro wrote:
> The only thing it uses the argument for is its ->d_fsdata and
> all callers have that already available.
> 
> Note that in the recursive call we are dealing with a (sub)directory
> configfs_dirent, and for those ->s_dentry->d_fsdata points back
> to configfs_dirent itself.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Reviewed-by: Breno Leitao <leitao@debian.org>

[RFC PATCH 04/14] configfs_depend_prep(): pass configfs_dirent instead of dentry

[Al Viro]

Again, the only thing it uses dentry for is dentry->d_fsdata; for the
recursive call the situation is the same as with configfs_detach_prep()
and the same observation about ->s_dentry->d_fsdata applies.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 0c071252c9d4..5219c4fd527e 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1067,15 +1067,12 @@ static int configfs_dump(struct configfs_dirent *sd, int level)
  * much on the stack, though, so folks that need this function - be careful
  * about your stack!  Patches will be accepted to make it iterative.
  */
-static int configfs_depend_prep(struct dentry *origin,
+static int configfs_depend_prep(struct configfs_dirent *sd,
 				struct config_item *target)
 {
-	struct configfs_dirent *child_sd, *sd;
+	struct configfs_dirent *child_sd;
 	int ret = 0;
 
-	BUG_ON(!origin || !origin->d_fsdata);
-	sd = origin->d_fsdata;
-
 	if (sd->s_element == target)  /* Boo-yah */
 		goto out;
 
@@ -1083,8 +1080,7 @@ static int configfs_depend_prep(struct dentry *origin,
 		if ((child_sd->s_type & CONFIGFS_DIR) &&
 		    !(child_sd->s_type & CONFIGFS_USET_DROPPING) &&
 		    !(child_sd->s_type & CONFIGFS_USET_CREATING)) {
-			ret = configfs_depend_prep(child_sd->s_dentry,
-						   target);
+			ret = configfs_depend_prep(child_sd, target);
 			if (!ret)
 				goto out;  /* Child path boo-yah */
 		}
@@ -1105,7 +1101,7 @@ static int configfs_do_depend_item(struct dentry *subsys_dentry,
 
 	spin_lock(&configfs_dirent_lock);
 	/* Scan the tree, return 0 if found */
-	ret = configfs_depend_prep(subsys_dentry, target);
+	ret = configfs_depend_prep(subsys_dentry->d_fsdata, target);
 	if (ret)
 		goto out_unlock_dirent_lock;
 
-- 
2.47.3

Re: [RFC PATCH 04/14] configfs_depend_prep(): pass configfs_dirent instead of dentry

[Jan Kara]

On Tue 19-05-26 08:06:23, Al Viro wrote:
> Again, the only thing it uses dentry for is dentry->d_fsdata; for the
> recursive call the situation is the same as with configfs_detach_prep()
> and the same observation about ->s_dentry->d_fsdata applies.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 0c071252c9d4..5219c4fd527e 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -1067,15 +1067,12 @@ static int configfs_dump(struct configfs_dirent *sd, int level)
>   * much on the stack, though, so folks that need this function - be careful
>   * about your stack!  Patches will be accepted to make it iterative.
>   */
> -static int configfs_depend_prep(struct dentry *origin,
> +static int configfs_depend_prep(struct configfs_dirent *sd,
>  				struct config_item *target)
>  {
> -	struct configfs_dirent *child_sd, *sd;
> +	struct configfs_dirent *child_sd;
>  	int ret = 0;
>  
> -	BUG_ON(!origin || !origin->d_fsdata);
> -	sd = origin->d_fsdata;
> -
>  	if (sd->s_element == target)  /* Boo-yah */
>  		goto out;
>  
> @@ -1083,8 +1080,7 @@ static int configfs_depend_prep(struct dentry *origin,
>  		if ((child_sd->s_type & CONFIGFS_DIR) &&
>  		    !(child_sd->s_type & CONFIGFS_USET_DROPPING) &&
>  		    !(child_sd->s_type & CONFIGFS_USET_CREATING)) {
> -			ret = configfs_depend_prep(child_sd->s_dentry,
> -						   target);
> +			ret = configfs_depend_prep(child_sd, target);
>  			if (!ret)
>  				goto out;  /* Child path boo-yah */
>  		}
> @@ -1105,7 +1101,7 @@ static int configfs_do_depend_item(struct dentry *subsys_dentry,
>  
>  	spin_lock(&configfs_dirent_lock);
>  	/* Scan the tree, return 0 if found */
> -	ret = configfs_depend_prep(subsys_dentry, target);
> +	ret = configfs_depend_prep(subsys_dentry->d_fsdata, target);
>  	if (ret)
>  		goto out_unlock_dirent_lock;
>  
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 05/14] configfs_do_depend_item(): pass configfs_dirent instead of dentry

[Al Viro]

Again, the only thing it uses the argument for is its ->d_fsdata
and callers already have that - as the matter of fact, they are
passing ->s_dentry of that configfs_dirent, so that the function
could get it back as ->d_fsdata of that.  With nothing else in
dentry even looked at...

configfs_dirent in question is a directory one - in this case those
are subdirectories of root (aka roots of "subsystem" trees).

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 5219c4fd527e..e4b7eaa58d65 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1093,7 +1093,7 @@ static int configfs_depend_prep(struct configfs_dirent *sd,
 	return ret;
 }
 
-static int configfs_do_depend_item(struct dentry *subsys_dentry,
+static int configfs_do_depend_item(struct configfs_dirent *subsys_sd,
 				   struct config_item *target)
 {
 	struct configfs_dirent *p;
@@ -1101,7 +1101,7 @@ static int configfs_do_depend_item(struct dentry *subsys_dentry,
 
 	spin_lock(&configfs_dirent_lock);
 	/* Scan the tree, return 0 if found */
-	ret = configfs_depend_prep(subsys_dentry->d_fsdata, target);
+	ret = configfs_depend_prep(subsys_sd, target);
 	if (ret)
 		goto out_unlock_dirent_lock;
 
@@ -1167,7 +1167,7 @@ int configfs_depend_item(struct configfs_subsystem *subsys,
 	}
 
 	/* Ok, now we can trust subsys/s_item */
-	ret = configfs_do_depend_item(subsys_sd->s_dentry, target);
+	ret = configfs_do_depend_item(subsys_sd, target);
 
 out_unlock_fs:
 	inode_unlock(d_inode(root));
@@ -1269,7 +1269,7 @@ int configfs_depend_item_unlocked(struct configfs_subsystem *caller_subsys,
 	}
 
 	/* Now we can execute core of depend item */
-	ret = configfs_do_depend_item(subsys_sd->s_dentry, target);
+	ret = configfs_do_depend_item(subsys_sd, target);
 
 	if (target_subsys != caller_subsys)
 out_root_unlock:
-- 
2.47.3

Re: [RFC PATCH 05/14] configfs_do_depend_item(): pass configfs_dirent instead of dentry

[Jan Kara]

On Tue 19-05-26 08:06:24, Al Viro wrote:
> Again, the only thing it uses the argument for is its ->d_fsdata
> and callers already have that - as the matter of fact, they are
> passing ->s_dentry of that configfs_dirent, so that the function
> could get it back as ->d_fsdata of that.  With nothing else in
> dentry even looked at...
> 
> configfs_dirent in question is a directory one - in this case those
> are subdirectories of root (aka roots of "subsystem" trees).
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 5219c4fd527e..e4b7eaa58d65 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -1093,7 +1093,7 @@ static int configfs_depend_prep(struct configfs_dirent *sd,
>  	return ret;
>  }
>  
> -static int configfs_do_depend_item(struct dentry *subsys_dentry,
> +static int configfs_do_depend_item(struct configfs_dirent *subsys_sd,
>  				   struct config_item *target)
>  {
>  	struct configfs_dirent *p;
> @@ -1101,7 +1101,7 @@ static int configfs_do_depend_item(struct dentry *subsys_dentry,
>  
>  	spin_lock(&configfs_dirent_lock);
>  	/* Scan the tree, return 0 if found */
> -	ret = configfs_depend_prep(subsys_dentry->d_fsdata, target);
> +	ret = configfs_depend_prep(subsys_sd, target);
>  	if (ret)
>  		goto out_unlock_dirent_lock;
>  
> @@ -1167,7 +1167,7 @@ int configfs_depend_item(struct configfs_subsystem *subsys,
>  	}
>  
>  	/* Ok, now we can trust subsys/s_item */
> -	ret = configfs_do_depend_item(subsys_sd->s_dentry, target);
> +	ret = configfs_do_depend_item(subsys_sd, target);
>  
>  out_unlock_fs:
>  	inode_unlock(d_inode(root));
> @@ -1269,7 +1269,7 @@ int configfs_depend_item_unlocked(struct configfs_subsystem *caller_subsys,
>  	}
>  
>  	/* Now we can execute core of depend item */
> -	ret = configfs_do_depend_item(subsys_sd->s_dentry, target);
> +	ret = configfs_do_depend_item(subsys_sd, target);
>  
>  	if (target_subsys != caller_subsys)
>  out_root_unlock:
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 06/14] configfs_detach_rollback(): pass configfs_dirent instead of dentry

[Al Viro]

same story as with configfs_detach_prep() this function is undoing.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index e4b7eaa58d65..726c56d1772d 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -562,16 +562,15 @@ static int configfs_detach_prep(struct configfs_dirent *parent_sd, struct dentry
  * Walk the tree, resetting CONFIGFS_USET_DROPPING wherever it was
  * set.
  */
-static void configfs_detach_rollback(struct dentry *dentry)
+static void configfs_detach_rollback(struct configfs_dirent *parent_sd)
 {
-	struct configfs_dirent *parent_sd = dentry->d_fsdata;
 	struct configfs_dirent *sd;
 
 	parent_sd->s_type &= ~CONFIGFS_USET_DROPPING;
 
 	list_for_each_entry(sd, &parent_sd->s_children, s_sibling)
 		if (sd->s_type & CONFIGFS_USET_DEFAULT)
-			configfs_detach_rollback(sd->s_dentry);
+			configfs_detach_rollback(sd);
 }
 
 static void detach_attrs(struct config_item * item)
@@ -1509,7 +1508,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 		if (!ret) {
 			ret = configfs_detach_prep(sd, &wait);
 			if (ret)
-				configfs_detach_rollback(dentry);
+				configfs_detach_rollback(sd);
 		}
 		spin_unlock(&configfs_dirent_lock);
 		mutex_unlock(&configfs_symlink_mutex);
@@ -1530,7 +1529,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 	frag = sd->s_frag;
 	if (down_write_killable(&frag->frag_sem)) {
 		spin_lock(&configfs_dirent_lock);
-		configfs_detach_rollback(dentry);
+		configfs_detach_rollback(sd);
 		spin_unlock(&configfs_dirent_lock);
 		config_item_put(parent_item);
 		return -EINTR;
-- 
2.47.3

Re: [RFC PATCH 06/14] configfs_detach_rollback(): pass configfs_dirent instead of dentry

[Jan Kara]

On Tue 19-05-26 08:06:25, Al Viro wrote:
> same story as with configfs_detach_prep() this function is undoing.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index e4b7eaa58d65..726c56d1772d 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -562,16 +562,15 @@ static int configfs_detach_prep(struct configfs_dirent *parent_sd, struct dentry
>   * Walk the tree, resetting CONFIGFS_USET_DROPPING wherever it was
>   * set.
>   */
> -static void configfs_detach_rollback(struct dentry *dentry)
> +static void configfs_detach_rollback(struct configfs_dirent *parent_sd)
>  {
> -	struct configfs_dirent *parent_sd = dentry->d_fsdata;
>  	struct configfs_dirent *sd;
>  
>  	parent_sd->s_type &= ~CONFIGFS_USET_DROPPING;
>  
>  	list_for_each_entry(sd, &parent_sd->s_children, s_sibling)
>  		if (sd->s_type & CONFIGFS_USET_DEFAULT)
> -			configfs_detach_rollback(sd->s_dentry);
> +			configfs_detach_rollback(sd);
>  }
>  
>  static void detach_attrs(struct config_item * item)
> @@ -1509,7 +1508,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
>  		if (!ret) {
>  			ret = configfs_detach_prep(sd, &wait);
>  			if (ret)
> -				configfs_detach_rollback(dentry);
> +				configfs_detach_rollback(sd);
>  		}
>  		spin_unlock(&configfs_dirent_lock);
>  		mutex_unlock(&configfs_symlink_mutex);
> @@ -1530,7 +1529,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
>  	frag = sd->s_frag;
>  	if (down_write_killable(&frag->frag_sem)) {
>  		spin_lock(&configfs_dirent_lock);
> -		configfs_detach_rollback(dentry);
> +		configfs_detach_rollback(sd);
>  		spin_unlock(&configfs_dirent_lock);
>  		config_item_put(parent_item);
>  		return -EINTR;
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 07/14] populate_group(): move cleanup on failure to the sole caller

[Al Viro]

... where it folds with configfs_detach_item() into a call of
configfs_detach_group().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 726c56d1772d..d2a60429a545 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -728,17 +728,13 @@ static int populate_groups(struct config_group *group,
 			   struct configfs_fragment *frag)
 {
 	struct config_group *new_group;
-	int ret = 0;
 
 	list_for_each_entry(new_group, &group->default_groups, group_entry) {
-		ret = create_default_group(group, new_group, frag);
-		if (ret) {
-			detach_groups(group);
-			break;
-		}
+		int ret = create_default_group(group, new_group, frag);
+		if (ret)
+			return ret;
 	}
-
-	return ret;
+	return 0;
 }
 
 void configfs_remove_default_groups(struct config_group *group)
@@ -878,6 +874,13 @@ static void configfs_detach_item(struct config_item *item)
 	configfs_remove_dir(item);
 }
 
+/* Caller holds the mutex of the group's inode */
+static void configfs_detach_group(struct config_item *item)
+{
+	detach_groups(to_config_group(item));
+	configfs_detach_item(item);
+}
+
 static int configfs_attach_group(struct config_item *parent_item,
 				 struct config_item *item,
 				 struct dentry *dentry,
@@ -904,7 +907,7 @@ static int configfs_attach_group(struct config_item *parent_item,
 		configfs_adjust_dir_dirent_depth_before_populate(sd);
 		ret = populate_groups(to_config_group(item), frag);
 		if (ret) {
-			configfs_detach_item(item);
+			configfs_detach_group(item);
 			d_inode(dentry)->i_flags |= S_DEAD;
 			dont_mount(dentry);
 		}
@@ -917,13 +920,6 @@ static int configfs_attach_group(struct config_item *parent_item,
 	return ret;
 }
 
-/* Caller holds the mutex of the group's inode */
-static void configfs_detach_group(struct config_item *item)
-{
-	detach_groups(to_config_group(item));
-	configfs_detach_item(item);
-}
-
 /*
  * After the item has been detached from the filesystem view, we are
  * ready to tear it out of the hierarchy.  Notify the client before
-- 
2.47.3

Re: [RFC PATCH 07/14] populate_group(): move cleanup on failure to the sole caller

[Jan Kara]

On Tue 19-05-26 08:06:26, Al Viro wrote:
> ... where it folds with configfs_detach_item() into a call of
> configfs_detach_group().
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 726c56d1772d..d2a60429a545 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -728,17 +728,13 @@ static int populate_groups(struct config_group *group,
>  			   struct configfs_fragment *frag)
>  {
>  	struct config_group *new_group;
> -	int ret = 0;
>  
>  	list_for_each_entry(new_group, &group->default_groups, group_entry) {
> -		ret = create_default_group(group, new_group, frag);
> -		if (ret) {
> -			detach_groups(group);
> -			break;
> -		}
> +		int ret = create_default_group(group, new_group, frag);
> +		if (ret)
> +			return ret;
>  	}
> -
> -	return ret;
> +	return 0;
>  }
>  
>  void configfs_remove_default_groups(struct config_group *group)
> @@ -878,6 +874,13 @@ static void configfs_detach_item(struct config_item *item)
>  	configfs_remove_dir(item);
>  }
>  
> +/* Caller holds the mutex of the group's inode */
> +static void configfs_detach_group(struct config_item *item)
> +{
> +	detach_groups(to_config_group(item));
> +	configfs_detach_item(item);
> +}
> +
>  static int configfs_attach_group(struct config_item *parent_item,
>  				 struct config_item *item,
>  				 struct dentry *dentry,
> @@ -904,7 +907,7 @@ static int configfs_attach_group(struct config_item *parent_item,
>  		configfs_adjust_dir_dirent_depth_before_populate(sd);
>  		ret = populate_groups(to_config_group(item), frag);
>  		if (ret) {
> -			configfs_detach_item(item);
> +			configfs_detach_group(item);
>  			d_inode(dentry)->i_flags |= S_DEAD;
>  			dont_mount(dentry);
>  		}
> @@ -917,13 +920,6 @@ static int configfs_attach_group(struct config_item *parent_item,
>  	return ret;
>  }
>  
> -/* Caller holds the mutex of the group's inode */
> -static void configfs_detach_group(struct config_item *item)
> -{
> -	detach_groups(to_config_group(item));
> -	configfs_detach_item(item);
> -}
> -
>  /*
>   * After the item has been detached from the filesystem view, we are
>   * ready to tear it out of the hierarchy.  Notify the client before
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 08/14] populate_attrs(): move cleanup to the sole caller

[Al Viro]

... where it folds with configfs_remove_dir() into a call of
configfs_detach_item().  Note that at the early failure exit
(before we'd added any children) we were not calling detach_attrs()
only because there it would've been a no-op - nothing added,
nothing there to be removed.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index d2a60429a545..395270cc3710 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -621,25 +621,23 @@ static int populate_attrs(struct config_item *item)
 			if (ops && ops->is_visible && !ops->is_visible(item, attr, i))
 				continue;
 
-			if ((error = configfs_create_file(item, attr)))
-				break;
+			error = configfs_create_file(item, attr);
+			if (error)
+				return error;
 		}
 	}
-	if (!error && t->ct_bin_attrs) {
+	if (t->ct_bin_attrs) {
 		for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
 			if (ops && ops->is_bin_visible && !ops->is_bin_visible(item, bin_attr, i))
 				continue;
 
 			error = configfs_create_bin_file(item, bin_attr);
 			if (error)
-				break;
+				return error;
 		}
 	}
 
-	if (error)
-		detach_attrs(item);
-
-	return error;
+	return 0;
 }
 
 static int configfs_attach_group(struct config_item *parent_item,
@@ -824,6 +822,13 @@ static void link_group(struct config_group *parent_group, struct config_group *g
 		link_group(group, new_group);
 }
 
+/* Caller holds the mutex of the item's inode */
+static void configfs_detach_item(struct config_item *item)
+{
+	detach_attrs(item);
+	configfs_remove_dir(item);
+}
+
 /*
  * The goal is that configfs_attach_item() (and
  * configfs_attach_group()) can be called from either the VFS or this
@@ -856,7 +861,7 @@ static int configfs_attach_item(struct config_item *parent_item,
 			 * we must lock them as rmdir() would.
 			 */
 			inode_lock(d_inode(dentry));
-			configfs_remove_dir(item);
+			configfs_detach_item(item);
 			d_inode(dentry)->i_flags |= S_DEAD;
 			dont_mount(dentry);
 			inode_unlock(d_inode(dentry));
@@ -867,13 +872,6 @@ static int configfs_attach_item(struct config_item *parent_item,
 	return ret;
 }
 
-/* Caller holds the mutex of the item's inode */
-static void configfs_detach_item(struct config_item *item)
-{
-	detach_attrs(item);
-	configfs_remove_dir(item);
-}
-
 /* Caller holds the mutex of the group's inode */
 static void configfs_detach_group(struct config_item *item)
 {
-- 
2.47.3

Re: [RFC PATCH 08/14] populate_attrs(): move cleanup to the sole caller

[Jan Kara]

On Tue 19-05-26 08:06:27, Al Viro wrote:
> ... where it folds with configfs_remove_dir() into a call of
> configfs_detach_item().  Note that at the early failure exit
> (before we'd added any children) we were not calling detach_attrs()
> only because there it would've been a no-op - nothing added,
> nothing there to be removed.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index d2a60429a545..395270cc3710 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -621,25 +621,23 @@ static int populate_attrs(struct config_item *item)
>  			if (ops && ops->is_visible && !ops->is_visible(item, attr, i))
>  				continue;
>  
> -			if ((error = configfs_create_file(item, attr)))
> -				break;
> +			error = configfs_create_file(item, attr);
> +			if (error)
> +				return error;
>  		}
>  	}
> -	if (!error && t->ct_bin_attrs) {
> +	if (t->ct_bin_attrs) {
>  		for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
>  			if (ops && ops->is_bin_visible && !ops->is_bin_visible(item, bin_attr, i))
>  				continue;
>  
>  			error = configfs_create_bin_file(item, bin_attr);
>  			if (error)
> -				break;
> +				return error;
>  		}
>  	}
>  
> -	if (error)
> -		detach_attrs(item);
> -
> -	return error;
> +	return 0;
>  }
>  
>  static int configfs_attach_group(struct config_item *parent_item,
> @@ -824,6 +822,13 @@ static void link_group(struct config_group *parent_group, struct config_group *g
>  		link_group(group, new_group);
>  }
>  
> +/* Caller holds the mutex of the item's inode */
> +static void configfs_detach_item(struct config_item *item)
> +{
> +	detach_attrs(item);
> +	configfs_remove_dir(item);
> +}
> +
>  /*
>   * The goal is that configfs_attach_item() (and
>   * configfs_attach_group()) can be called from either the VFS or this
> @@ -856,7 +861,7 @@ static int configfs_attach_item(struct config_item *parent_item,
>  			 * we must lock them as rmdir() would.
>  			 */
>  			inode_lock(d_inode(dentry));
> -			configfs_remove_dir(item);
> +			configfs_detach_item(item);
>  			d_inode(dentry)->i_flags |= S_DEAD;
>  			dont_mount(dentry);
>  			inode_unlock(d_inode(dentry));
> @@ -867,13 +872,6 @@ static int configfs_attach_item(struct config_item *parent_item,
>  	return ret;
>  }
>  
> -/* Caller holds the mutex of the item's inode */
> -static void configfs_detach_item(struct config_item *item)
> -{
> -	detach_attrs(item);
> -	configfs_remove_dir(item);
> -}
> -
>  /* Caller holds the mutex of the group's inode */
>  static void configfs_detach_group(struct config_item *item)
>  {
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 09/14] configfs_remove_dir(), detach_attrs(): switch to passing dentry

[Al Viro]

... and deal with grabbing/dropping it in the sole caller.
After that configfs_remove_dir() becomes an unconditional call of remove_dir(),
so we can fold them together.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 57 ++++++++++++++++-------------------------------
 1 file changed, 19 insertions(+), 38 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 395270cc3710..deb18689aa0a 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -394,7 +394,18 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
 	return PTR_ERR(inode);
 }
 
-static void remove_dir(struct dentry * d)
+/**
+ * configfs_remove_dir - remove an config_item's directory.
+ * @d:	dentry we're removing.
+ *
+ * The only thing special about this is that we remove any files in
+ * the directory before we remove the directory, and we've inlined
+ * what used to be configfs_rmdir() below, instead of calling separately.
+ *
+ * Caller holds the mutex of the item's inode
+ */
+
+static void configfs_remove_dir(struct dentry *d)
 {
 	struct dentry * parent = dget(d->d_parent);
 
@@ -414,31 +425,6 @@ static void remove_dir(struct dentry * d)
 	dput(parent);
 }
 
-/**
- * configfs_remove_dir - remove an config_item's directory.
- * @item:	config_item we're removing.
- *
- * The only thing special about this is that we remove any files in
- * the directory before we remove the directory, and we've inlined
- * what used to be configfs_rmdir() below, instead of calling separately.
- *
- * Caller holds the mutex of the item's inode
- */
-
-static void configfs_remove_dir(struct config_item * item)
-{
-	struct dentry * dentry = dget(item->ci_dentry);
-
-	if (!dentry)
-		return;
-
-	remove_dir(dentry);
-	/**
-	 * Drop reference from dget() on entrance.
-	 */
-	dput(dentry);
-}
-
 static struct dentry * configfs_lookup(struct inode *dir,
 				       struct dentry *dentry,
 				       unsigned int flags)
@@ -573,15 +559,11 @@ static void configfs_detach_rollback(struct configfs_dirent *parent_sd)
 			configfs_detach_rollback(sd);
 }
 
-static void detach_attrs(struct config_item * item)
+static void detach_attrs(struct dentry *dentry)
 {
-	struct dentry * dentry = dget(item->ci_dentry);
 	struct configfs_dirent * parent_sd;
 	struct configfs_dirent * sd, * tmp;
 
-	if (!dentry)
-		return;
-
 	pr_debug("configfs %s: dropping attrs for  dir\n",
 		 dentry->d_name.name);
 
@@ -595,11 +577,6 @@ static void detach_attrs(struct config_item * item)
 		configfs_drop_dentry(sd, dentry);
 		configfs_put(sd);
 	}
-
-	/**
-	 * Drop reference from dget() on entrance.
-	 */
-	dput(dentry);
 }
 
 static int populate_attrs(struct config_item *item)
@@ -825,8 +802,12 @@ static void link_group(struct config_group *parent_group, struct config_group *g
 /* Caller holds the mutex of the item's inode */
 static void configfs_detach_item(struct config_item *item)
 {
-	detach_attrs(item);
-	configfs_remove_dir(item);
+	struct dentry *dentry = dget(item->ci_dentry);
+	if (dentry) {
+		detach_attrs(dentry);
+		configfs_remove_dir(dentry);
+		dput(dentry);
+	}
 }
 
 /*
-- 
2.47.3

Re: [RFC PATCH 09/14] configfs_remove_dir(), detach_attrs(): switch to passing dentry

[Jan Kara]

On Tue 19-05-26 08:06:28, Al Viro wrote:
> ... and deal with grabbing/dropping it in the sole caller.
> After that configfs_remove_dir() becomes an unconditional call of remove_dir(),
> so we can fold them together.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 57 ++++++++++++++++-------------------------------
>  1 file changed, 19 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 395270cc3710..deb18689aa0a 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -394,7 +394,18 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
>  	return PTR_ERR(inode);
>  }
>  
> -static void remove_dir(struct dentry * d)
> +/**
> + * configfs_remove_dir - remove an config_item's directory.
> + * @d:	dentry we're removing.
> + *
> + * The only thing special about this is that we remove any files in
> + * the directory before we remove the directory, and we've inlined
> + * what used to be configfs_rmdir() below, instead of calling separately.
> + *
> + * Caller holds the mutex of the item's inode
> + */
> +
> +static void configfs_remove_dir(struct dentry *d)
>  {
>  	struct dentry * parent = dget(d->d_parent);
>  
> @@ -414,31 +425,6 @@ static void remove_dir(struct dentry * d)
>  	dput(parent);
>  }
>  
> -/**
> - * configfs_remove_dir - remove an config_item's directory.
> - * @item:	config_item we're removing.
> - *
> - * The only thing special about this is that we remove any files in
> - * the directory before we remove the directory, and we've inlined
> - * what used to be configfs_rmdir() below, instead of calling separately.
> - *
> - * Caller holds the mutex of the item's inode
> - */
> -
> -static void configfs_remove_dir(struct config_item * item)
> -{
> -	struct dentry * dentry = dget(item->ci_dentry);
> -
> -	if (!dentry)
> -		return;
> -
> -	remove_dir(dentry);
> -	/**
> -	 * Drop reference from dget() on entrance.
> -	 */
> -	dput(dentry);
> -}
> -
>  static struct dentry * configfs_lookup(struct inode *dir,
>  				       struct dentry *dentry,
>  				       unsigned int flags)
> @@ -573,15 +559,11 @@ static void configfs_detach_rollback(struct configfs_dirent *parent_sd)
>  			configfs_detach_rollback(sd);
>  }
>  
> -static void detach_attrs(struct config_item * item)
> +static void detach_attrs(struct dentry *dentry)
>  {
> -	struct dentry * dentry = dget(item->ci_dentry);
>  	struct configfs_dirent * parent_sd;
>  	struct configfs_dirent * sd, * tmp;
>  
> -	if (!dentry)
> -		return;
> -
>  	pr_debug("configfs %s: dropping attrs for  dir\n",
>  		 dentry->d_name.name);
>  
> @@ -595,11 +577,6 @@ static void detach_attrs(struct config_item * item)
>  		configfs_drop_dentry(sd, dentry);
>  		configfs_put(sd);
>  	}
> -
> -	/**
> -	 * Drop reference from dget() on entrance.
> -	 */
> -	dput(dentry);
>  }
>  
>  static int populate_attrs(struct config_item *item)
> @@ -825,8 +802,12 @@ static void link_group(struct config_group *parent_group, struct config_group *g
>  /* Caller holds the mutex of the item's inode */
>  static void configfs_detach_item(struct config_item *item)
>  {
> -	detach_attrs(item);
> -	configfs_remove_dir(item);
> +	struct dentry *dentry = dget(item->ci_dentry);
> +	if (dentry) {
> +		detach_attrs(dentry);
> +		configfs_remove_dir(dentry);
> +		dput(dentry);
> +	}
>  }
>  
>  /*
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 10/14] switch configfs_detach_{group,item}() to passing dentry

[Al Viro]

... and there's no need to grab/drop it, or check for NULL - none
of the callers would even get there with NULL dentry and all of
them have the sucker pinned

Note that if sd is a directory configfs_dirent, we have sd->s_element
pointing to config_item with item->ci_dentry equal to sd->s_dentry.
Which is the only reason why detach_groups() gets away with using
the latter for locking the inode and the former for removal.

Aren't redundant data structures wonderful, for obfuscation if nothing
else?

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 43 +++++++++++++++----------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index deb18689aa0a..39a496749dc2 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -621,18 +621,14 @@ static int configfs_attach_group(struct config_item *parent_item,
 				 struct config_item *item,
 				 struct dentry *dentry,
 				 struct configfs_fragment *frag);
-static void configfs_detach_group(struct config_item *item);
+static void configfs_detach_group(struct dentry *dentry);
 
-static void detach_groups(struct config_group *group)
+static void detach_groups(struct dentry *dentry)
 {
-	struct dentry * dentry = dget(group->cg_item.ci_dentry);
 	struct dentry *child;
 	struct configfs_dirent *parent_sd;
 	struct configfs_dirent *sd, *tmp;
 
-	if (!dentry)
-		return;
-
 	parent_sd = dentry->d_fsdata;
 	list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
 		if (!sd->s_element ||
@@ -643,7 +639,7 @@ static void detach_groups(struct config_group *group)
 
 		inode_lock(d_inode(child));
 
-		configfs_detach_group(sd->s_element);
+		configfs_detach_group(child);
 		d_inode(child)->i_flags |= S_DEAD;
 		dont_mount(child);
 
@@ -652,11 +648,6 @@ static void detach_groups(struct config_group *group)
 		d_delete(child);
 		dput(child);
 	}
-
-	/**
-	 * Drop reference from dget() on entrance.
-	 */
-	dput(dentry);
 }
 
 /*
@@ -800,14 +791,10 @@ static void link_group(struct config_group *parent_group, struct config_group *g
 }
 
 /* Caller holds the mutex of the item's inode */
-static void configfs_detach_item(struct config_item *item)
+static void configfs_detach_item(struct dentry *dentry)
 {
-	struct dentry *dentry = dget(item->ci_dentry);
-	if (dentry) {
-		detach_attrs(dentry);
-		configfs_remove_dir(dentry);
-		dput(dentry);
-	}
+	detach_attrs(dentry);
+	configfs_remove_dir(dentry);
 }
 
 /*
@@ -842,7 +829,7 @@ static int configfs_attach_item(struct config_item *parent_item,
 			 * we must lock them as rmdir() would.
 			 */
 			inode_lock(d_inode(dentry));
-			configfs_detach_item(item);
+			configfs_detach_item(dentry);
 			d_inode(dentry)->i_flags |= S_DEAD;
 			dont_mount(dentry);
 			inode_unlock(d_inode(dentry));
@@ -854,10 +841,10 @@ static int configfs_attach_item(struct config_item *parent_item,
 }
 
 /* Caller holds the mutex of the group's inode */
-static void configfs_detach_group(struct config_item *item)
+static void configfs_detach_group(struct dentry *dentry)
 {
-	detach_groups(to_config_group(item));
-	configfs_detach_item(item);
+	detach_groups(dentry);
+	configfs_detach_item(dentry);
 }
 
 static int configfs_attach_group(struct config_item *parent_item,
@@ -886,7 +873,7 @@ static int configfs_attach_group(struct config_item *parent_item,
 		configfs_adjust_dir_dirent_depth_before_populate(sd);
 		ret = populate_groups(to_config_group(item), frag);
 		if (ret) {
-			configfs_detach_group(item);
+			configfs_detach_group(dentry);
 			d_inode(dentry)->i_flags |= S_DEAD;
 			dont_mount(dentry);
 		}
@@ -1522,13 +1509,13 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 		dead_item_owner = item->ci_type->ct_owner;
 
 	if (sd->s_type & CONFIGFS_USET_DIR) {
-		configfs_detach_group(item);
+		configfs_detach_group(dentry);
 
 		mutex_lock(&subsys->su_mutex);
 		client_disconnect_notify(parent_item, item);
 		unlink_group(to_config_group(item));
 	} else {
-		configfs_detach_item(item);
+		configfs_detach_item(dentry);
 
 		mutex_lock(&subsys->su_mutex);
 		client_disconnect_notify(parent_item, item);
@@ -1781,7 +1768,7 @@ void configfs_unregister_group(struct config_group *group)
 	configfs_detach_prep(sd, NULL);
 	spin_unlock(&configfs_dirent_lock);
 
-	configfs_detach_group(&group->cg_item);
+	configfs_detach_group(dentry);
 	d_inode(dentry)->i_flags |= S_DEAD;
 	dont_mount(dentry);
 	d_drop(dentry);
@@ -1930,7 +1917,7 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
 	}
 	spin_unlock(&configfs_dirent_lock);
 	mutex_unlock(&configfs_symlink_mutex);
-	configfs_detach_group(&group->cg_item);
+	configfs_detach_group(dentry);
 	d_inode(dentry)->i_flags |= S_DEAD;
 	dont_mount(dentry);
 	inode_unlock(d_inode(dentry));
-- 
2.47.3

Re: [RFC PATCH 10/14] switch configfs_detach_{group,item}() to passing dentry

[Jan Kara]

On Tue 19-05-26 08:06:29, Al Viro wrote:
> ... and there's no need to grab/drop it, or check for NULL - none
> of the callers would even get there with NULL dentry and all of
> them have the sucker pinned
> 
> Note that if sd is a directory configfs_dirent, we have sd->s_element
> pointing to config_item with item->ci_dentry equal to sd->s_dentry.
> Which is the only reason why detach_groups() gets away with using
> the latter for locking the inode and the former for removal.
> 
> Aren't redundant data structures wonderful, for obfuscation if nothing
> else?
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 43 +++++++++++++++----------------------------
>  1 file changed, 15 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index deb18689aa0a..39a496749dc2 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -621,18 +621,14 @@ static int configfs_attach_group(struct config_item *parent_item,
>  				 struct config_item *item,
>  				 struct dentry *dentry,
>  				 struct configfs_fragment *frag);
> -static void configfs_detach_group(struct config_item *item);
> +static void configfs_detach_group(struct dentry *dentry);
>  
> -static void detach_groups(struct config_group *group)
> +static void detach_groups(struct dentry *dentry)
>  {
> -	struct dentry * dentry = dget(group->cg_item.ci_dentry);
>  	struct dentry *child;
>  	struct configfs_dirent *parent_sd;
>  	struct configfs_dirent *sd, *tmp;
>  
> -	if (!dentry)
> -		return;
> -
>  	parent_sd = dentry->d_fsdata;
>  	list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
>  		if (!sd->s_element ||
> @@ -643,7 +639,7 @@ static void detach_groups(struct config_group *group)
>  
>  		inode_lock(d_inode(child));
>  
> -		configfs_detach_group(sd->s_element);
> +		configfs_detach_group(child);
>  		d_inode(child)->i_flags |= S_DEAD;
>  		dont_mount(child);
>  
> @@ -652,11 +648,6 @@ static void detach_groups(struct config_group *group)
>  		d_delete(child);
>  		dput(child);
>  	}
> -
> -	/**
> -	 * Drop reference from dget() on entrance.
> -	 */
> -	dput(dentry);
>  }
>  
>  /*
> @@ -800,14 +791,10 @@ static void link_group(struct config_group *parent_group, struct config_group *g
>  }
>  
>  /* Caller holds the mutex of the item's inode */
> -static void configfs_detach_item(struct config_item *item)
> +static void configfs_detach_item(struct dentry *dentry)
>  {
> -	struct dentry *dentry = dget(item->ci_dentry);
> -	if (dentry) {
> -		detach_attrs(dentry);
> -		configfs_remove_dir(dentry);
> -		dput(dentry);
> -	}
> +	detach_attrs(dentry);
> +	configfs_remove_dir(dentry);
>  }
>  
>  /*
> @@ -842,7 +829,7 @@ static int configfs_attach_item(struct config_item *parent_item,
>  			 * we must lock them as rmdir() would.
>  			 */
>  			inode_lock(d_inode(dentry));
> -			configfs_detach_item(item);
> +			configfs_detach_item(dentry);
>  			d_inode(dentry)->i_flags |= S_DEAD;
>  			dont_mount(dentry);
>  			inode_unlock(d_inode(dentry));
> @@ -854,10 +841,10 @@ static int configfs_attach_item(struct config_item *parent_item,
>  }
>  
>  /* Caller holds the mutex of the group's inode */
> -static void configfs_detach_group(struct config_item *item)
> +static void configfs_detach_group(struct dentry *dentry)
>  {
> -	detach_groups(to_config_group(item));
> -	configfs_detach_item(item);
> +	detach_groups(dentry);
> +	configfs_detach_item(dentry);
>  }
>  
>  static int configfs_attach_group(struct config_item *parent_item,
> @@ -886,7 +873,7 @@ static int configfs_attach_group(struct config_item *parent_item,
>  		configfs_adjust_dir_dirent_depth_before_populate(sd);
>  		ret = populate_groups(to_config_group(item), frag);
>  		if (ret) {
> -			configfs_detach_group(item);
> +			configfs_detach_group(dentry);
>  			d_inode(dentry)->i_flags |= S_DEAD;
>  			dont_mount(dentry);
>  		}
> @@ -1522,13 +1509,13 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
>  		dead_item_owner = item->ci_type->ct_owner;
>  
>  	if (sd->s_type & CONFIGFS_USET_DIR) {
> -		configfs_detach_group(item);
> +		configfs_detach_group(dentry);
>  
>  		mutex_lock(&subsys->su_mutex);
>  		client_disconnect_notify(parent_item, item);
>  		unlink_group(to_config_group(item));
>  	} else {
> -		configfs_detach_item(item);
> +		configfs_detach_item(dentry);
>  
>  		mutex_lock(&subsys->su_mutex);
>  		client_disconnect_notify(parent_item, item);
> @@ -1781,7 +1768,7 @@ void configfs_unregister_group(struct config_group *group)
>  	configfs_detach_prep(sd, NULL);
>  	spin_unlock(&configfs_dirent_lock);
>  
> -	configfs_detach_group(&group->cg_item);
> +	configfs_detach_group(dentry);
>  	d_inode(dentry)->i_flags |= S_DEAD;
>  	dont_mount(dentry);
>  	d_drop(dentry);
> @@ -1930,7 +1917,7 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
>  	}
>  	spin_unlock(&configfs_dirent_lock);
>  	mutex_unlock(&configfs_symlink_mutex);
> -	configfs_detach_group(&group->cg_item);
> +	configfs_detach_group(dentry);
>  	d_inode(dentry)->i_flags |= S_DEAD;
>  	dont_mount(dentry);
>  	inode_unlock(d_inode(dentry));
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 11/14] configfs: dentry refcount needs to be pinned only once

[Al Viro]

currently we have a weird situation where
	* symlinks and roots of subtrees created by mkdir are pinned once
	* subdirectories of subtrees created by mkdir are pinned twice
	* roots of subtrees created by register_{group,subsystem} are pinned
twice.

It makes things harder to follow for no good reason.  The goal is to
encapsulate the unbalanced dget/dput into d_{make,discard}_persisitent()
and, preferably, allow a use of simple_recursive_removal() or analogue
thereof.  So let's regularize that and pin things only once.

create_default_group() and configfs_register_subsystem() don't need to
keep their reference around on success - configfs_create_dir() has pinned
the sucker already.  So we can drop the reference passed to
configfs_create_dir() (via configfs_attach_group(), etc.) both on success
and on failure.  On the removal side we no longer have the double references,
so we need an explicit dget() to compensate.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 39a496749dc2..6d9c6a5b17ff 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -635,7 +635,7 @@ static void detach_groups(struct dentry *dentry)
 		    !(sd->s_type & CONFIGFS_USET_DEFAULT))
 			continue;
 
-		child = sd->s_dentry;
+		child = dget(sd->s_dentry);
 
 		inode_lock(d_inode(child));
 
@@ -683,8 +683,8 @@ static int create_default_group(struct config_group *parent_group,
 		} else {
 			BUG_ON(d_inode(child));
 			d_drop(child);
-			dput(child);
 		}
+		dput(child);
 	}
 
 	return ret;
@@ -1754,7 +1754,7 @@ EXPORT_SYMBOL(configfs_register_group);
 void configfs_unregister_group(struct config_group *group)
 {
 	struct configfs_subsystem *subsys = group->cg_subsys;
-	struct dentry *dentry = group->cg_item.ci_dentry;
+	struct dentry *dentry = dget(group->cg_item.ci_dentry);
 	struct dentry *parent = group->cg_item.ci_parent->ci_dentry;
 	struct configfs_dirent *sd = dentry->d_fsdata;
 	struct configfs_fragment *frag = sd->s_frag;
@@ -1869,12 +1869,12 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
 		if (err) {
 			BUG_ON(d_inode(dentry));
 			d_drop(dentry);
-			dput(dentry);
 		} else {
 			spin_lock(&configfs_dirent_lock);
 			configfs_dir_set_ready(dentry->d_fsdata);
 			spin_unlock(&configfs_dirent_lock);
 		}
+		dput(dentry);
 	}
 
 	inode_unlock(d_inode(root));
@@ -1893,7 +1893,7 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
 void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
 {
 	struct config_group *group = &subsys->su_group;
-	struct dentry *dentry = group->cg_item.ci_dentry;
+	struct dentry *dentry = dget(group->cg_item.ci_dentry);
 	struct dentry *root = dentry->d_sb->s_root;
 	struct configfs_dirent *sd = dentry->d_fsdata;
 	struct configfs_fragment *frag = sd->s_frag;
-- 
2.47.3

Re: [RFC PATCH 11/14] configfs: dentry refcount needs to be pinned only once

[Jan Kara]

On Tue 19-05-26 08:06:30, Al Viro wrote:
> currently we have a weird situation where
> 	* symlinks and roots of subtrees created by mkdir are pinned once
> 	* subdirectories of subtrees created by mkdir are pinned twice
> 	* roots of subtrees created by register_{group,subsystem} are pinned
> twice.
> 
> It makes things harder to follow for no good reason.  The goal is to
> encapsulate the unbalanced dget/dput into d_{make,discard}_persisitent()
> and, preferably, allow a use of simple_recursive_removal() or analogue
> thereof.  So let's regularize that and pin things only once.
> 
> create_default_group() and configfs_register_subsystem() don't need to
> keep their reference around on success - configfs_create_dir() has pinned
> the sucker already.  So we can drop the reference passed to
> configfs_create_dir() (via configfs_attach_group(), etc.) both on success
> and on failure.  On the removal side we no longer have the double references,
> so we need an explicit dget() to compensate.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 39a496749dc2..6d9c6a5b17ff 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -635,7 +635,7 @@ static void detach_groups(struct dentry *dentry)
>  		    !(sd->s_type & CONFIGFS_USET_DEFAULT))
>  			continue;
>  
> -		child = sd->s_dentry;
> +		child = dget(sd->s_dentry);
>  
>  		inode_lock(d_inode(child));
>  
> @@ -683,8 +683,8 @@ static int create_default_group(struct config_group *parent_group,
>  		} else {
>  			BUG_ON(d_inode(child));
>  			d_drop(child);
> -			dput(child);
>  		}
> +		dput(child);
>  	}
>  
>  	return ret;
> @@ -1754,7 +1754,7 @@ EXPORT_SYMBOL(configfs_register_group);
>  void configfs_unregister_group(struct config_group *group)
>  {
>  	struct configfs_subsystem *subsys = group->cg_subsys;
> -	struct dentry *dentry = group->cg_item.ci_dentry;
> +	struct dentry *dentry = dget(group->cg_item.ci_dentry);
>  	struct dentry *parent = group->cg_item.ci_parent->ci_dentry;
>  	struct configfs_dirent *sd = dentry->d_fsdata;
>  	struct configfs_fragment *frag = sd->s_frag;
> @@ -1869,12 +1869,12 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
>  		if (err) {
>  			BUG_ON(d_inode(dentry));
>  			d_drop(dentry);
> -			dput(dentry);
>  		} else {
>  			spin_lock(&configfs_dirent_lock);
>  			configfs_dir_set_ready(dentry->d_fsdata);
>  			spin_unlock(&configfs_dirent_lock);
>  		}
> +		dput(dentry);
>  	}
>  
>  	inode_unlock(d_inode(root));
> @@ -1893,7 +1893,7 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
>  void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
>  {
>  	struct config_group *group = &subsys->su_group;
> -	struct dentry *dentry = group->cg_item.ci_dentry;
> +	struct dentry *dentry = dget(group->cg_item.ci_dentry);
>  	struct dentry *root = dentry->d_sb->s_root;
>  	struct configfs_dirent *sd = dentry->d_fsdata;
>  	struct configfs_fragment *frag = sd->s_frag;
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 12/14] configfs: mark pinned dentries persistent

[Al Viro]

on the removal side we can (finally) get rid of __simple_unlink()
and __simple_rmdir() kludges now that dentries in question are
properly marked persistent - simple_unlink() and simple_rmdir()
will do the right thing for those.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c     | 13 +++----------
 fs/configfs/symlink.c |  3 +--
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 6d9c6a5b17ff..740ea2c115bd 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -314,9 +314,7 @@ static int configfs_create_dir(struct config_item *item, struct dentry *dentry,
 	inode->i_fop = &configfs_dir_operations;
 	/* directory inodes start off with i_nlink == 2 (for "." entry) */
 	inc_nlink(inode);
-	d_instantiate(dentry, inode);
-	/* already hashed */
-	dget(dentry);  /* pin directory dentries in core */
+	d_make_persistent(dentry, inode);
 	inc_nlink(d_inode(p));
 	item->ci_dentry = dentry;
 	return 0;
@@ -384,8 +382,7 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
 
 	inode->i_link = body;
 	inode->i_op = &configfs_symlink_inode_operations;
-	d_instantiate(dentry, inode);
-	dget(dentry);  /* pin link dentries in core */
+	d_make_persistent(dentry, inode);
 	return 0;
 
 out_remove:
@@ -412,12 +409,8 @@ static void configfs_remove_dir(struct dentry *d)
 	configfs_remove_dirent(d);
 
 	if (d_really_is_positive(d)) {
-		if (likely(simple_empty(d))) {
-			__simple_rmdir(d_inode(parent),d);
-			dput(d);
-		} else {
+		if (unlikely(simple_rmdir(d_inode(parent), d)))
 			pr_warn("remove_dir (%pd): attributes remain", d);
-		}
 	}
 
 	pr_debug(" o %pd removing done (%d)\n", d, d_count(d));
diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index f3f79c67add5..31eb28b27309 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -229,9 +229,8 @@ int configfs_unlink(struct inode *dir, struct dentry *dentry)
 	spin_lock(&configfs_dirent_lock);
 	list_del_init(&sd->s_sibling);
 	spin_unlock(&configfs_dirent_lock);
-	configfs_drop_dentry(sd, dentry->d_parent);
-	dput(dentry);
 	configfs_put(sd);
+	simple_unlink(dir, dentry);
 
 	/*
 	 * drop_link() must be called before
-- 
2.47.3

Re: [RFC PATCH 12/14] configfs: mark pinned dentries persistent

[Jan Kara]

On Tue 19-05-26 08:06:31, Al Viro wrote:
> on the removal side we can (finally) get rid of __simple_unlink()
> and __simple_rmdir() kludges now that dentries in question are
> properly marked persistent - simple_unlink() and simple_rmdir()
> will do the right thing for those.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c     | 13 +++----------
>  fs/configfs/symlink.c |  3 +--
>  2 files changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 6d9c6a5b17ff..740ea2c115bd 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -314,9 +314,7 @@ static int configfs_create_dir(struct config_item *item, struct dentry *dentry,
>  	inode->i_fop = &configfs_dir_operations;
>  	/* directory inodes start off with i_nlink == 2 (for "." entry) */
>  	inc_nlink(inode);
> -	d_instantiate(dentry, inode);
> -	/* already hashed */
> -	dget(dentry);  /* pin directory dentries in core */
> +	d_make_persistent(dentry, inode);
>  	inc_nlink(d_inode(p));
>  	item->ci_dentry = dentry;
>  	return 0;
> @@ -384,8 +382,7 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
>  
>  	inode->i_link = body;
>  	inode->i_op = &configfs_symlink_inode_operations;
> -	d_instantiate(dentry, inode);
> -	dget(dentry);  /* pin link dentries in core */
> +	d_make_persistent(dentry, inode);
>  	return 0;
>  
>  out_remove:
> @@ -412,12 +409,8 @@ static void configfs_remove_dir(struct dentry *d)
>  	configfs_remove_dirent(d);
>  
>  	if (d_really_is_positive(d)) {
> -		if (likely(simple_empty(d))) {
> -			__simple_rmdir(d_inode(parent),d);
> -			dput(d);
> -		} else {
> +		if (unlikely(simple_rmdir(d_inode(parent), d)))
>  			pr_warn("remove_dir (%pd): attributes remain", d);
> -		}
>  	}
>  
>  	pr_debug(" o %pd removing done (%d)\n", d, d_count(d));
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index f3f79c67add5..31eb28b27309 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
> @@ -229,9 +229,8 @@ int configfs_unlink(struct inode *dir, struct dentry *dentry)
>  	spin_lock(&configfs_dirent_lock);
>  	list_del_init(&sd->s_sibling);
>  	spin_unlock(&configfs_dirent_lock);
> -	configfs_drop_dentry(sd, dentry->d_parent);
> -	dput(dentry);
>  	configfs_put(sd);
> +	simple_unlink(dir, dentry);
>  
>  	/*
>  	 * drop_link() must be called before
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 13/14] kill configfs_drop_dentry()

[Al Viro]

Fold into the only remaining user, don't bother with the timestamps
of parent - we are going to rmdir it shortly anyway, which will
override those.

Fix the locking of inode, while we are at it - updating the link
count and timestamps ought to be done with the inode locked.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/configfs_internal.h |  1 -
 fs/configfs/dir.c               | 17 ++++++++++++++++-
 fs/configfs/inode.c             | 22 ----------------------
 3 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index 0b969d0eb8ff..acdeea8e2d69 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -76,7 +76,6 @@ extern int configfs_make_dirent(struct configfs_dirent *, struct dentry *,
 extern int configfs_dirent_is_ready(struct configfs_dirent *);
 
 extern const unsigned char * configfs_get_name(struct configfs_dirent *sd);
-extern void configfs_drop_dentry(struct configfs_dirent *sd, struct dentry *parent);
 extern int configfs_setattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, struct iattr *iattr);
 
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 740ea2c115bd..a2bb602fb38c 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -562,12 +562,27 @@ static void detach_attrs(struct dentry *dentry)
 
 	parent_sd = dentry->d_fsdata;
 	list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
+		struct dentry *child;
 		if (!sd->s_element || !(sd->s_type & CONFIGFS_NOT_PINNED))
 			continue;
 		spin_lock(&configfs_dirent_lock);
 		list_del_init(&sd->s_sibling);
+		child = sd->s_dentry;
+		if (child && simple_positive(child))
+			dget_dlock(child);
+		else
+			child = NULL;
 		spin_unlock(&configfs_dirent_lock);
-		configfs_drop_dentry(sd, dentry);
+		if (child) {
+			struct inode *inode = child->d_inode;
+			d_drop(child);
+
+			inode_lock_nested(inode, I_MUTEX_NONDIR2);
+			inode_set_ctime_current(inode);
+			drop_nlink(inode);
+			inode_unlock(inode);
+			dput(child);
+		}
 		configfs_put(sd);
 	}
 }
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index bc507b720a01..3c26b6c443be 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -195,25 +195,3 @@ const unsigned char * configfs_get_name(struct configfs_dirent *sd)
 	}
 	return NULL;
 }
-
-
-/*
- * Unhashes the dentry corresponding to given configfs_dirent
- * Called with parent inode's i_mutex held.
- */
-void configfs_drop_dentry(struct configfs_dirent * sd, struct dentry * parent)
-{
-	struct dentry * dentry = sd->s_dentry;
-
-	if (dentry) {
-		spin_lock(&dentry->d_lock);
-		if (simple_positive(dentry)) {
-			dget_dlock(dentry);
-			__d_drop(dentry);
-			spin_unlock(&dentry->d_lock);
-			__simple_unlink(d_inode(parent), dentry);
-			dput(dentry);
-		} else
-			spin_unlock(&dentry->d_lock);
-	}
-}
-- 
2.47.3

Re: [RFC PATCH 13/14] kill configfs_drop_dentry()

[Jan Kara]

On Tue 19-05-26 08:06:32, Al Viro wrote:
> Fold into the only remaining user, don't bother with the timestamps
> of parent - we are going to rmdir it shortly anyway, which will
> override those.
> 
> Fix the locking of inode, while we are at it - updating the link
> count and timestamps ought to be done with the inode locked.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
...
>  	parent_sd = dentry->d_fsdata;
>  	list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
> +		struct dentry *child;
>  		if (!sd->s_element || !(sd->s_type & CONFIGFS_NOT_PINNED))
>  			continue;
>  		spin_lock(&configfs_dirent_lock);
>  		list_del_init(&sd->s_sibling);
> +		child = sd->s_dentry;
> +		if (child && simple_positive(child))
> +			dget_dlock(child);

Why is it safe to call dget_dlock() here? We hold only configfs_dirent_lock
so cannot we race with somebody doing dget() on the 'child' dentry? Even if
it is indeed safe for some reason I'm missing, it would deserve a comment I
guess?

								Honza

> +		else
> +			child = NULL;
>  		spin_unlock(&configfs_dirent_lock);
> -		configfs_drop_dentry(sd, dentry);
> +		if (child) {
> +			struct inode *inode = child->d_inode;
> +			d_drop(child);
> +
> +			inode_lock_nested(inode, I_MUTEX_NONDIR2);
> +			inode_set_ctime_current(inode);
> +			drop_nlink(inode);
> +			inode_unlock(inode);
> +			dput(child);
> +		}
>  		configfs_put(sd);
>  	}
>  }
> diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
> index bc507b720a01..3c26b6c443be 100644
> --- a/fs/configfs/inode.c
> +++ b/fs/configfs/inode.c
> @@ -195,25 +195,3 @@ const unsigned char * configfs_get_name(struct configfs_dirent *sd)
>  	}
>  	return NULL;
>  }
> -
> -
> -/*
> - * Unhashes the dentry corresponding to given configfs_dirent
> - * Called with parent inode's i_mutex held.
> - */
> -void configfs_drop_dentry(struct configfs_dirent * sd, struct dentry * parent)
> -{
> -	struct dentry * dentry = sd->s_dentry;
> -
> -	if (dentry) {
> -		spin_lock(&dentry->d_lock);
> -		if (simple_positive(dentry)) {
> -			dget_dlock(dentry);
> -			__d_drop(dentry);
> -			spin_unlock(&dentry->d_lock);
> -			__simple_unlink(d_inode(parent), dentry);
> -			dput(dentry);
> -		} else
> -			spin_unlock(&dentry->d_lock);
> -	}
> -}
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH 13/14] kill configfs_drop_dentry()

[Linus Torvalds]

On Tue, 19 May 2026 at 06:13, Jan Kara <jack@suse.cz> wrote:
>
> Why is it safe to call dget_dlock() here?

That does indeed look very buggy. Even if there is some reason why the
child dentry hasn't been seen by anybody else - and I don't see such a
reason - it seems a bogus optimization to try to avoid the proper
locking of the refcount.

Was there some lock order reason for not just locking the child dentry?

              Linus

Re: [RFC PATCH 13/14] kill configfs_drop_dentry()

[Al Viro]

On Tue, May 19, 2026 at 03:12:56PM +0200, Jan Kara wrote:

> Why is it safe to call dget_dlock() here? We hold only configfs_dirent_lock
> so cannot we race with somebody doing dget() on the 'child' dentry? Even if
> it is indeed safe for some reason I'm missing, it would deserve a comment I
> guess?

Because of the braindamage on conflict resolution.  Thanks for spotting,
fixed and force-pushed.  Updated version of commit:

commit 85863f5773c15f02ff6518ad034e3b380b78ca53
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Tue May 12 12:53:35 2026 -0400

    kill configfs_drop_dentry()
    
    Fold into the only remaining user, don't bother with the timestamps
    of parent - we are going to rmdir it shortly anyway, which will
    override those.
    
    Fix the locking of inode, while we are at it - updating the link
    count and timestamps ought to be done with the inode locked.
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index 0b969d0eb8ff..acdeea8e2d69 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -76,7 +76,6 @@ extern int configfs_make_dirent(struct configfs_dirent *, struct dentry *,
 extern int configfs_dirent_is_ready(struct configfs_dirent *);
 
 extern const unsigned char * configfs_get_name(struct configfs_dirent *sd);
-extern void configfs_drop_dentry(struct configfs_dirent *sd, struct dentry *parent);
 extern int configfs_setattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, struct iattr *iattr);
 
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 740ea2c115bd..a68b4a9241c0 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -562,12 +562,33 @@ static void detach_attrs(struct dentry *dentry)
 
 	parent_sd = dentry->d_fsdata;
 	list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
+		struct dentry *child;
 		if (!sd->s_element || !(sd->s_type & CONFIGFS_NOT_PINNED))
 			continue;
 		spin_lock(&configfs_dirent_lock);
 		list_del_init(&sd->s_sibling);
+		child = sd->s_dentry;
+		if (child) {
+			spin_lock(&child->d_lock);
+			if (simple_positive(child)) {
+				dget_dlock(child);
+				spin_unlock(&child->d_lock);
+			} else {
+				spin_unlock(&child->d_lock);
+				child = NULL;
+			}
+		}
 		spin_unlock(&configfs_dirent_lock);
-		configfs_drop_dentry(sd, dentry);
+		if (child) {
+			struct inode *inode = child->d_inode;
+			d_drop(child);
+
+			inode_lock_nested(inode, I_MUTEX_NONDIR2);
+			inode_set_ctime_current(inode);
+			drop_nlink(inode);
+			inode_unlock(inode);
+			dput(child);
+		}
 		configfs_put(sd);
 	}
 }
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index bc507b720a01..3c26b6c443be 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -195,25 +195,3 @@ const unsigned char * configfs_get_name(struct configfs_dirent *sd)
 	}
 	return NULL;
 }
-
-
-/*
- * Unhashes the dentry corresponding to given configfs_dirent
- * Called with parent inode's i_mutex held.
- */
-void configfs_drop_dentry(struct configfs_dirent * sd, struct dentry * parent)
-{
-	struct dentry * dentry = sd->s_dentry;
-
-	if (dentry) {
-		spin_lock(&dentry->d_lock);
-		if (simple_positive(dentry)) {
-			dget_dlock(dentry);
-			__d_drop(dentry);
-			spin_unlock(&dentry->d_lock);
-			__simple_unlink(d_inode(parent), dentry);
-			dput(dentry);
-		} else
-			spin_unlock(&dentry->d_lock);
-	}
-}

Re: [RFC PATCH 13/14] kill configfs_drop_dentry()

[Jan Kara]

On Tue 19-05-26 16:37:24, Al Viro wrote:
> On Tue, May 19, 2026 at 03:12:56PM +0200, Jan Kara wrote:
> 
> > Why is it safe to call dget_dlock() here? We hold only configfs_dirent_lock
> > so cannot we race with somebody doing dget() on the 'child' dentry? Even if
> > it is indeed safe for some reason I'm missing, it would deserve a comment I
> > guess?
> 
> Because of the braindamage on conflict resolution.  Thanks for spotting,
> fixed and force-pushed.  Updated version of commit:
> 
> commit 85863f5773c15f02ff6518ad034e3b380b78ca53
> Author: Al Viro <viro@zeniv.linux.org.uk>
> Date:   Tue May 12 12:53:35 2026 -0400
> 
>     kill configfs_drop_dentry()
>     
>     Fold into the only remaining user, don't bother with the timestamps
>     of parent - we are going to rmdir it shortly anyway, which will
>     override those.
>     
>     Fix the locking of inode, while we are at it - updating the link
>     count and timestamps ought to be done with the inode locked.
>     
>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> 
> diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
> index 0b969d0eb8ff..acdeea8e2d69 100644
> --- a/fs/configfs/configfs_internal.h
> +++ b/fs/configfs/configfs_internal.h
> @@ -76,7 +76,6 @@ extern int configfs_make_dirent(struct configfs_dirent *, struct dentry *,
>  extern int configfs_dirent_is_ready(struct configfs_dirent *);
>  
>  extern const unsigned char * configfs_get_name(struct configfs_dirent *sd);
> -extern void configfs_drop_dentry(struct configfs_dirent *sd, struct dentry *parent);
>  extern int configfs_setattr(struct mnt_idmap *idmap,
>  			    struct dentry *dentry, struct iattr *iattr);
>  
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 740ea2c115bd..a68b4a9241c0 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -562,12 +562,33 @@ static void detach_attrs(struct dentry *dentry)
>  
>  	parent_sd = dentry->d_fsdata;
>  	list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
> +		struct dentry *child;
>  		if (!sd->s_element || !(sd->s_type & CONFIGFS_NOT_PINNED))
>  			continue;
>  		spin_lock(&configfs_dirent_lock);
>  		list_del_init(&sd->s_sibling);
> +		child = sd->s_dentry;
> +		if (child) {
> +			spin_lock(&child->d_lock);
> +			if (simple_positive(child)) {
> +				dget_dlock(child);
> +				spin_unlock(&child->d_lock);
> +			} else {
> +				spin_unlock(&child->d_lock);
> +				child = NULL;
> +			}
> +		}
>  		spin_unlock(&configfs_dirent_lock);
> -		configfs_drop_dentry(sd, dentry);
> +		if (child) {
> +			struct inode *inode = child->d_inode;
> +			d_drop(child);
> +
> +			inode_lock_nested(inode, I_MUTEX_NONDIR2);
> +			inode_set_ctime_current(inode);
> +			drop_nlink(inode);
> +			inode_unlock(inode);
> +			dput(child);
> +		}
>  		configfs_put(sd);
>  	}
>  }
> diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
> index bc507b720a01..3c26b6c443be 100644
> --- a/fs/configfs/inode.c
> +++ b/fs/configfs/inode.c
> @@ -195,25 +195,3 @@ const unsigned char * configfs_get_name(struct configfs_dirent *sd)
>  	}
>  	return NULL;
>  }
> -
> -
> -/*
> - * Unhashes the dentry corresponding to given configfs_dirent
> - * Called with parent inode's i_mutex held.
> - */
> -void configfs_drop_dentry(struct configfs_dirent * sd, struct dentry * parent)
> -{
> -	struct dentry * dentry = sd->s_dentry;
> -
> -	if (dentry) {
> -		spin_lock(&dentry->d_lock);
> -		if (simple_positive(dentry)) {
> -			dget_dlock(dentry);
> -			__d_drop(dentry);
> -			spin_unlock(&dentry->d_lock);
> -			__simple_unlink(d_inode(parent), dentry);
> -			dput(dentry);
> -		} else
> -			spin_unlock(&dentry->d_lock);
> -	}
> -}
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 14/14] configfs_create(): lift parent timestamp updates into callers

[Al Viro]

... and do *not* do it in ->lookup() case.  stat foo/bar
should not update mtime of foo, TYVM...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c   | 6 +++++-
 fs/configfs/inode.c | 3 ---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index a2bb602fb38c..4da2fec2b64e 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -295,6 +295,7 @@ static int configfs_create_dir(struct config_item *item, struct dentry *dentry,
 	int error;
 	umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
 	struct dentry *p = dentry->d_parent;
+	struct inode *p_inode = d_inode(p);
 	struct inode *inode;
 
 	BUG_ON(!item);
@@ -315,7 +316,8 @@ static int configfs_create_dir(struct config_item *item, struct dentry *dentry,
 	/* directory inodes start off with i_nlink == 2 (for "." entry) */
 	inc_nlink(inode);
 	d_make_persistent(dentry, inode);
-	inc_nlink(d_inode(p));
+	inc_nlink(p_inode);
+	inode_set_mtime_to_ts(p_inode, inode_set_ctime_current(p_inode));
 	item->ci_dentry = dentry;
 	return 0;
 
@@ -369,6 +371,7 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
 	int err = 0;
 	umode_t mode = S_IFLNK | S_IRWXUGO;
 	struct configfs_dirent *p = parent->d_fsdata;
+	struct inode *p_inode = d_inode(parent);
 	struct inode *inode;
 
 	err = configfs_make_dirent(p, dentry, target, mode, CONFIGFS_ITEM_LINK,
@@ -383,6 +386,7 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
 	inode->i_link = body;
 	inode->i_op = &configfs_symlink_inode_operations;
 	d_make_persistent(dentry, inode);
+	inode_set_mtime_to_ts(p_inode, inode_set_ctime_current(p_inode));
 	return 0;
 
 out_remove:
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index 3c26b6c443be..68290fe0e374 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -157,7 +157,6 @@ struct inode *configfs_create(struct dentry *dentry, umode_t mode)
 {
 	struct inode *inode = NULL;
 	struct configfs_dirent *sd;
-	struct inode *p_inode;
 
 	if (!dentry)
 		return ERR_PTR(-ENOENT);
@@ -170,8 +169,6 @@ struct inode *configfs_create(struct dentry *dentry, umode_t mode)
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 
-	p_inode = d_inode(dentry->d_parent);
-	inode_set_mtime_to_ts(p_inode, inode_set_ctime_current(p_inode));
 	configfs_set_inode_lock_class(sd, inode);
 	return inode;
 }
-- 
2.47.3

Re: [RFC PATCH 14/14] configfs_create(): lift parent timestamp updates into callers

[Jan Kara]

On Tue 19-05-26 08:06:33, Al Viro wrote:
> ... and do *not* do it in ->lookup() case.  stat foo/bar
> should not update mtime of foo, TYVM...
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/configfs/dir.c   | 6 +++++-
>  fs/configfs/inode.c | 3 ---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index a2bb602fb38c..4da2fec2b64e 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -295,6 +295,7 @@ static int configfs_create_dir(struct config_item *item, struct dentry *dentry,
>  	int error;
>  	umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
>  	struct dentry *p = dentry->d_parent;
> +	struct inode *p_inode = d_inode(p);
>  	struct inode *inode;
>  
>  	BUG_ON(!item);
> @@ -315,7 +316,8 @@ static int configfs_create_dir(struct config_item *item, struct dentry *dentry,
>  	/* directory inodes start off with i_nlink == 2 (for "." entry) */
>  	inc_nlink(inode);
>  	d_make_persistent(dentry, inode);
> -	inc_nlink(d_inode(p));
> +	inc_nlink(p_inode);
> +	inode_set_mtime_to_ts(p_inode, inode_set_ctime_current(p_inode));
>  	item->ci_dentry = dentry;
>  	return 0;
>  
> @@ -369,6 +371,7 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
>  	int err = 0;
>  	umode_t mode = S_IFLNK | S_IRWXUGO;
>  	struct configfs_dirent *p = parent->d_fsdata;
> +	struct inode *p_inode = d_inode(parent);
>  	struct inode *inode;
>  
>  	err = configfs_make_dirent(p, dentry, target, mode, CONFIGFS_ITEM_LINK,
> @@ -383,6 +386,7 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
>  	inode->i_link = body;
>  	inode->i_op = &configfs_symlink_inode_operations;
>  	d_make_persistent(dentry, inode);
> +	inode_set_mtime_to_ts(p_inode, inode_set_ctime_current(p_inode));
>  	return 0;
>  
>  out_remove:
> diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
> index 3c26b6c443be..68290fe0e374 100644
> --- a/fs/configfs/inode.c
> +++ b/fs/configfs/inode.c
> @@ -157,7 +157,6 @@ struct inode *configfs_create(struct dentry *dentry, umode_t mode)
>  {
>  	struct inode *inode = NULL;
>  	struct configfs_dirent *sd;
> -	struct inode *p_inode;
>  
>  	if (!dentry)
>  		return ERR_PTR(-ENOENT);
> @@ -170,8 +169,6 @@ struct inode *configfs_create(struct dentry *dentry, umode_t mode)
>  	if (!inode)
>  		return ERR_PTR(-ENOMEM);
>  
> -	p_inode = d_inode(dentry->d_parent);
> -	inode_set_mtime_to_ts(p_inode, inode_set_ctime_current(p_inode));
>  	configfs_set_inode_lock_class(sd, inode);
>  	return inode;
>  }
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v2 00/18] configfs cleanups and fixes

[Al Viro]

	Updated and rebased to -rc6; changes since v1:
* added a fix for races between lseek() and directory removals; had been
there since 2008.  If we use list_for_each_entry_safe(), we need to make
sure that the next object won't get moved around and/or removed while
we are in the loop body; holding the directory locked is enough to make
sure that another thread won't add/remove stuff to that directory, but
it does *not* prevent cursors being moved around.  So when lseek() for
those switched to global spinlock for list protection, all places that
relies upon the directory lock alone for list traversals needed to be
modified...
	Fixes (configfs_lookup()-triggered UAF and this one) are in
the beginning of the queue; that's #fixes-configfs and the rest is on
top of those.
* fixed braindamage in "kill configfs_drop_dentry()" (used to be #13,
now #14).
* added several "correct the argument types" patches - removing
never unused arguments for configfs_attach_{group,item}() and
converting create_default_group() from parent's config_group to
parent's dentry.

	This branch (7.1-rc6-based) lives in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.configfs
Individual patches in followups.  It still needs more testing...

	Review and testing would be very welcome.

I've also pushed tentative followups out into #more.configfs, but those
definitely will miss the coming merge window.  Will post separately...

Al Viro (18):
  configfs_lookup(): don't leave ->s_dentry dangling on failure
  configfs: fix lockless traversals of ->s_children
  configfs_mkdir(): use take_dentry_name_snapshot()
  configfs_detach_prep(): pass configfs_dirent instead of dentry
  configfs_depend_prep(): pass configfs_dirent instead of dentry
  configfs_do_depend_item(): pass configfs_dirent instead of dentry
  configfs_detach_rollback(): pass configfs_dirent instead of dentry
  populate_group(): move cleanup on failure to the sole caller
  populate_attrs(): move cleanup to the sole caller
  configfs_remove_dir(), detach_attrs(): switch to passing dentry
  switch configfs_detach_{group,item}() to passing dentry
  configfs: dentry refcount needs to be pinned only once
  configfs: mark pinned dentries persistent
  kill configfs_drop_dentry()
  configfs_create(): lift parent timestamp updates into callers
  configs_attach_item(): drop unused parent_item argument
  configfs_attach_group(): drop the unused parent_item argument
  create_default_group(): pass parent's dentry instead of config_group

 fs/configfs/configfs_internal.h |   1 -
 fs/configfs/dir.c               | 315 ++++++++++++++++----------------
 fs/configfs/inode.c             |  25 ---
 fs/configfs/symlink.c           |   3 +-
 4 files changed, 157 insertions(+), 187 deletions(-)

-- 
2.47.3

[PATCH v2 01/18] configfs_lookup(): don't leave ->s_dentry dangling on failure

[Al Viro]

Normally ->s_dentry is cleared when dentry it's pointing to becomes
negative (on eviction, realistically).  However, that only happens
if dentry gets to be positive in the first place; in case of inode
allocation failure dentry never becomes positive, so ->d_iput()
is not called at all.

We do part of what normally would've been done by configfs_d_iput()
(dropping the reference to configfs_dirent) manually, but we do
not clear ->s_dentry there.  Sloppy as it is, it does not matter in
case of configfs_create_{dir,link}() - there configfs_dirent does
not survive dropping the sole reference to it.

However, for configfs_lookup() it *does* survive, with a dangling
pointer to soon to be freed dentry sitting it its ->s_dentry.

Subsequent getdents(2) in that directory will end up dereferencing
that pointer in order to pick the inode number.  Use after free...

This is the minimal fix; the right approach is to set the linkage
between dentry and configfs_dirent only after we know that we have
an inode, but that takes more surgery and the bug had been there
since 2006, so...

Fixes: 3d0f89bb1694 ("configfs: Add permission and ownership to configfs objects") # 2.6.16-rc3
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 362b6ff9b908..e84483a0836d 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -486,6 +486,9 @@ static struct dentry * configfs_lookup(struct inode *dir,
 
 			inode = configfs_create(dentry, mode);
 			if (IS_ERR(inode)) {
+				spin_lock(&configfs_dirent_lock);
+				sd->s_dentry = NULL;
+				spin_unlock(&configfs_dirent_lock);
 				configfs_put(sd);
 				return ERR_CAST(inode);
 			}
-- 
2.47.3

[PATCH v2 1/3] get rid of impossible checks in detach_attrs()/configfs_detach_item()

[Al Viro]

configfs_detach_item() is never called with NULL item->ci_dentry.
Call graph stinks to high heaven, unfortunately (which is a major part
of the reasons for the entire massage)...

1) after successful configfs_create_dir(item, dentry), item->ci_dentry
is non-NULL, equal to dentry and dentry->d_fsdata->s_element points
to item.
2) after successful configfs_attach_item(_, item, dentry), we know we'd
passed through successful configfs_create_dir()
3) all directory dentries other than root have passed through successful
configfs_create_dir().  Root has ->d_fsdata set manually so that the
same warranties hold - ->d_fsdata->s_element->ci_dentry points back to
dentry itself.
4) when sd->s_type gets CONFIGFS_USET_DEFAULT set, we are guaranteed that
sd->s_element points to an item with ->ci_dentry non-NULL and
->ci_dentry->d_fsdata pointing back to sd.

configfs_detach_item()
	configfs_attach_item(), after successful configfs_create_dir()
	configfs_attach_group(), after successful configfs_attach_item()
	configfs_rmdir(), with item coming from dentry->d_fsdata->s_element,
			dentry being that of a directory
	configfs_detach_group()
		configfs_rmdir() (same as above)
		detach_groups(), given sd->s_element when sd->s_type has
				CONFIGFS_USET_DEFAULT))
		configfs_unregister_group(), assumes we'd called
				configfs_register_group() first, which
				would've done configfs_default_group(),
				which would've done configfs_attach_group().
				In any case, oopses a line later if
				->ci_dentry is NULL.
		configfs_unregister_subsystem(), assumes we'd called
				configfs_register_subsystem() first, which
				would've done configfs_attach_group().
				In any case, would've already oopsed if
				->ci_dentry had been NULL.

detach_attrs() is called only by configfs_detach_item() and gets item
from it.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

detach_attrs() is called only by configfs_detach_item() and gets item
from it.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 98bf37376165..515f41e7dba2 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -519,9 +519,6 @@ static void detach_attrs(struct config_item * item)
 	struct configfs_dirent * parent_sd;
 	struct configfs_dirent * sd, * tmp;
 
-	if (!dentry)
-		return;
-
 	pr_debug("configfs %s: dropping attrs for  dir\n",
 		 dentry->d_name.name);
 
@@ -773,9 +770,6 @@ static void configfs_detach_item(struct config_item *item)
 	detach_attrs(item);
 
 	dentry = dget(item->ci_dentry);
-	if (!dentry)
-		return;
-
 	parent = dget(dentry->d_parent);
 
 	configfs_remove_dirent(dentry);
-- 
2.47.3

Re: [PATCH v2 1/3] get rid of impossible checks in detach_attrs()/configfs_detach_item()

[Al Viro]

	Argh...  Please disregard the 1--3/3 part of the thread -
I've missed the stale (and really ancient - it is from back in
2019) files sitting around.  Sorry, should've checked what the glob
had expanded to... ;-/

Re: [PATCH v2 1/3] get rid of impossible checks in detach_attrs()/configfs_detach_item()

[Christian Brauner]

On Wed, Jun 03, 2026 at 08:53:40AM +0100, Al Viro wrote:
> 	Argh...  Please disregard the 1--3/3 part of the thread -
> I've missed the stale (and really ancient - it is from back in
> 2019) files sitting around.  Sorry, should've checked what the glob
> had expanded to... ;-/

I'm more worried about what time it is where you currently are... ;)

Re: [PATCH v2 1/3] get rid of impossible checks in detach_attrs()/configfs_detach_item()

[Al Viro]

On Wed, Jun 03, 2026 at 10:09:37AM +0200, Christian Brauner wrote:
> On Wed, Jun 03, 2026 at 08:53:40AM +0100, Al Viro wrote:
> > 	Argh...  Please disregard the 1--3/3 part of the thread -
> > I've missed the stale (and really ancient - it is from back in
> > 2019) files sitting around.  Sorry, should've checked what the glob
> > had expanded to... ;-/
> 
> I'm more worried about what time it is where you currently are... ;)

4am ;-/

FWIW, the followups in #more.configfs switch the sucker to
..._recursive_removal() and massage it to building subtrees
detached from the main dentry tree; as the result, the locking
is _much_ milder there - all lockdep magic goes away and so
does the configfs_dirent_is_ready() crap.

Diffstat of that part:
 Documentation/filesystems/porting.rst |   8 +
 fs/configfs/configfs_internal.h       |  12 +-
 fs/configfs/dir.c                     | 705 +++++++-------------------
 fs/configfs/file.c                    |  43 --
 fs/configfs/inode.c                   |  41 +-
 fs/configfs/symlink.c                 |   9 -
 fs/dcache.c                           |   2 +-
 fs/libfs.c                            |  61 ++-
 include/linux/fs.h                    |   2 +
 include/linux/fsnotify.h              |  13 -
 net/sunrpc/rpc_pipe.c                 |   2 +-
 11 files changed, 243 insertions(+), 655 deletions(-)

and it even seems to survive light testing.  There still are bits
in old branches that need to be ported on top of that, but most
of the tricky massage is already in.  Unlike #work.configfs that's
the next cycle fodder, though.

I suspect that #fixes-configfs (first two commits on #work.configfs)
might be worth merging before -final; the rest of #work.configfs is
-next fodder if nobody has problems with it.  #more.configfs is
going to be rebased on top of 7.2-rc1 at the end of merge window...

Anyway, I'm really too sleepy right now - will post other stuff
intended for -next (starting with updated #work.dcache) after I get
some sleep.

[PATCH v2 2/3] configfs_detach_item(): victim is never negative

[Al Viro]

it's been positive when created and had been pinned all along...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 515f41e7dba2..273e43adc8b7 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -773,9 +773,7 @@ static void configfs_detach_item(struct config_item *item)
 	parent = dget(dentry->d_parent);
 
 	configfs_remove_dirent(dentry);
-
-	if (d_really_is_positive(dentry))
-		simple_rmdir(d_inode(parent),dentry);
+	simple_rmdir(d_inode(parent), dentry);
 
 	dput(parent);
 	/**
-- 
2.47.3

[PATCH v2 02/18] configfs: fix lockless traversals of ->s_children

[Al Viro]

Having the parent directory locked protects entries from removal
by another thread, but it does *not* protect cursors from being
moved around by lseek() - or freed, for that matter.

Fixes: 6f6107640625 ("configfs: Introduce configfs_dirent_lock")
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 54 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 13 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index e84483a0836d..eb991b2a9c34 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -235,15 +235,16 @@ static int configfs_dirent_exists(struct dentry *dentry)
 	const unsigned char *new = dentry->d_name.name;
 	struct configfs_dirent *sd;
 
+	spin_lock(&configfs_dirent_lock);
 	list_for_each_entry(sd, &parent_sd->s_children, s_sibling) {
 		if (sd->s_element) {
-			const unsigned char *existing = configfs_get_name(sd);
-			if (strcmp(existing, new))
-				continue;
-			else
+			if (strcmp(configfs_get_name(sd), new) == 0) {
+				spin_unlock(&configfs_dirent_lock);
 				return -EEXIST;
+			}
 		}
 	}
+	spin_unlock(&configfs_dirent_lock);
 
 	return 0;
 }
@@ -575,11 +576,28 @@ static void configfs_detach_rollback(struct dentry *dentry)
 			configfs_detach_rollback(sd->s_dentry);
 }
 
+/*
+ * Find the next non-cursor.  configfs_dirent_lock held by caller.
+ */
+static struct configfs_dirent *next_dirent(struct configfs_dirent *parent,
+					   struct configfs_dirent *last)
+{
+	struct configfs_dirent *s;
+
+	s = list_prepare_entry(last, &parent->s_children, s_sibling);
+
+	list_for_each_entry_continue(s, &parent->s_children, s_sibling) {
+		if (s->s_element)
+			return s;
+	}
+	return NULL;
+}
+
 static void detach_attrs(struct config_item * item)
 {
 	struct dentry * dentry = dget(item->ci_dentry);
-	struct configfs_dirent * parent_sd;
-	struct configfs_dirent * sd, * tmp;
+	struct configfs_dirent *parent_sd;
+	struct configfs_dirent *sd, *next;
 
 	if (!dentry)
 		return;
@@ -588,15 +606,19 @@ static void detach_attrs(struct config_item * item)
 		 dentry->d_name.name);
 
 	parent_sd = dentry->d_fsdata;
-	list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
-		if (!sd->s_element || !(sd->s_type & CONFIGFS_NOT_PINNED))
+
+	spin_lock(&configfs_dirent_lock);
+	for (sd = next_dirent(parent_sd, NULL); sd; sd = next) {
+		next = next_dirent(parent_sd, sd);
+		if (!(sd->s_type & CONFIGFS_NOT_PINNED))
 			continue;
-		spin_lock(&configfs_dirent_lock);
 		list_del_init(&sd->s_sibling);
 		spin_unlock(&configfs_dirent_lock);
 		configfs_drop_dentry(sd, dentry);
 		configfs_put(sd);
+		spin_lock(&configfs_dirent_lock);
 	}
+	spin_unlock(&configfs_dirent_lock);
 
 	/**
 	 * Drop reference from dget() on entrance.
@@ -655,18 +677,20 @@ static void detach_groups(struct config_group *group)
 	struct dentry * dentry = dget(group->cg_item.ci_dentry);
 	struct dentry *child;
 	struct configfs_dirent *parent_sd;
-	struct configfs_dirent *sd, *tmp;
+	struct configfs_dirent *sd, *next;
 
 	if (!dentry)
 		return;
 
 	parent_sd = dentry->d_fsdata;
-	list_for_each_entry_safe(sd, tmp, &parent_sd->s_children, s_sibling) {
-		if (!sd->s_element ||
-		    !(sd->s_type & CONFIGFS_USET_DEFAULT))
+	spin_lock(&configfs_dirent_lock);
+	for (sd = next_dirent(parent_sd, NULL); sd; sd = next) {
+		next = next_dirent(parent_sd, sd);
+		if (!(sd->s_type & CONFIGFS_USET_DEFAULT))
 			continue;
 
 		child = sd->s_dentry;
+		spin_unlock(&configfs_dirent_lock);
 
 		inode_lock(d_inode(child));
 
@@ -678,7 +702,9 @@ static void detach_groups(struct config_group *group)
 
 		d_delete(child);
 		dput(child);
+		spin_lock(&configfs_dirent_lock);
 	}
+	spin_unlock(&configfs_dirent_lock);
 
 	/**
 	 * Drop reference from dget() on entrance.
@@ -1130,6 +1156,7 @@ configfs_find_subsys_dentry(struct configfs_dirent *root_sd,
 	struct configfs_dirent *p;
 	struct configfs_dirent *ret = NULL;
 
+	spin_lock(&configfs_dirent_lock);
 	list_for_each_entry(p, &root_sd->s_children, s_sibling) {
 		if (p->s_type & CONFIGFS_DIR &&
 		    p->s_element == subsys_item) {
@@ -1137,6 +1164,7 @@ configfs_find_subsys_dentry(struct configfs_dirent *root_sd,
 			break;
 		}
 	}
+	spin_unlock(&configfs_dirent_lock);
 
 	return ret;
 }
-- 
2.47.3

[PATCH v2 3/3] configfs: expand the call of simple_rmdir()

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 273e43adc8b7..775496add849 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -766,15 +766,24 @@ static void configfs_detach_item(struct config_item *item)
 {
 	struct dentry *dentry;
 	struct dentry *parent;
+	struct inode *dir, *inode;
 
 	detach_attrs(item);
 
 	dentry = dget(item->ci_dentry);
 	parent = dget(dentry->d_parent);
+	dir = d_inode(parent);
+	inode = d_inode(dentry);
 
 	configfs_remove_dirent(dentry);
-	simple_rmdir(d_inode(parent), dentry);
-
+	if (simple_empty(dentry)) {
+		drop_nlink(d_inode(dentry));
+		inode_set_mtime_to_ts(dir,
+			      inode_set_ctime_to_ts(dir, inode_set_ctime_current(inode)));
+		drop_nlink(inode);
+		dput(dentry);
+		drop_nlink(dir);
+	}
 	dput(parent);
 	/**
 	 * Drop reference from dget() on entrance.
-- 
2.47.3

[PATCH v2 03/18] configfs_mkdir(): use take_dentry_name_snapshot()

[Al Viro]

Note that neither ->make_group() nor ->make_item() are allowed to modify
the string passed to them - the argument is const char *.

Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index eb991b2a9c34..8181771c8a58 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1329,7 +1329,11 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	const struct config_item_type *type;
 	struct module *subsys_owner = NULL, *new_item_owner = NULL;
 	struct configfs_fragment *frag;
-	char *name;
+	struct name_snapshot n;
+	const char *name;
+
+	take_dentry_name_snapshot(&n, dentry);
+	name = n.name.name;
 
 	sd = dentry->d_parent->d_fsdata;
 
@@ -1381,14 +1385,6 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 		goto out_put;
 	}
 
-	name = kmalloc(dentry->d_name.len + 1, GFP_KERNEL);
-	if (!name) {
-		ret = -ENOMEM;
-		goto out_subsys_put;
-	}
-
-	snprintf(name, dentry->d_name.len + 1, "%s", dentry->d_name.name);
-
 	mutex_lock(&subsys->su_mutex);
 	if (type->ct_group_ops->make_group) {
 		group = type->ct_group_ops->make_group(to_config_group(parent_item), name);
@@ -1410,7 +1406,6 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	}
 	mutex_unlock(&subsys->su_mutex);
 
-	kfree(name);
 	if (ret) {
 		/*
 		 * If ret != 0, then link_obj() was never called.
@@ -1497,6 +1492,7 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	put_fragment(frag);
 
 out:
+	release_dentry_name_snapshot(&n);
 	return ERR_PTR(ret);
 }
 
-- 
2.47.3

[PATCH v2 04/18] configfs_detach_prep(): pass configfs_dirent instead of dentry

[Al Viro]

The only thing it uses the argument for is its ->d_fsdata and
all callers have that already available.

Note that in the recursive call we are dealing with a (sub)directory
configfs_dirent, and for those ->s_dentry->d_fsdata points back
to configfs_dirent itself.

Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 8181771c8a58..b456e4de25ab 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -517,9 +517,8 @@ static struct dentry * configfs_lookup(struct inode *dir,
  * If there is an error, the caller will reset the flags via
  * configfs_detach_rollback().
  */
-static int configfs_detach_prep(struct dentry *dentry, struct dentry **wait)
+static int configfs_detach_prep(struct configfs_dirent *parent_sd, struct dentry **wait)
 {
-	struct configfs_dirent *parent_sd = dentry->d_fsdata;
 	struct configfs_dirent *sd;
 	int ret;
 
@@ -547,7 +546,7 @@ static int configfs_detach_prep(struct dentry *dentry, struct dentry **wait)
 			 * Yup, recursive.  If there's a problem, blame
 			 * deep nesting of default_groups
 			 */
-			ret = configfs_detach_prep(sd->s_dentry, wait);
+			ret = configfs_detach_prep(sd, wait);
 			if (!ret)
 				continue;
 		} else
@@ -1540,7 +1539,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 		 */
 		ret = sd->s_dependent_count ? -EBUSY : 0;
 		if (!ret) {
-			ret = configfs_detach_prep(dentry, &wait);
+			ret = configfs_detach_prep(sd, &wait);
 			if (ret)
 				configfs_detach_rollback(dentry);
 		}
@@ -1837,7 +1836,7 @@ void configfs_unregister_group(struct config_group *group)
 
 	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
 	spin_lock(&configfs_dirent_lock);
-	configfs_detach_prep(dentry, NULL);
+	configfs_detach_prep(sd, NULL);
 	spin_unlock(&configfs_dirent_lock);
 
 	configfs_detach_group(&group->cg_item);
@@ -1984,7 +1983,7 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
 	inode_lock_nested(d_inode(dentry), I_MUTEX_CHILD);
 	mutex_lock(&configfs_symlink_mutex);
 	spin_lock(&configfs_dirent_lock);
-	if (configfs_detach_prep(dentry, NULL)) {
+	if (configfs_detach_prep(sd, NULL)) {
 		pr_err("Tried to unregister non-empty subsystem!\n");
 	}
 	spin_unlock(&configfs_dirent_lock);
-- 
2.47.3

[PATCH v2 05/18] configfs_depend_prep(): pass configfs_dirent instead of dentry

[Al Viro]

Again, the only thing it uses dentry for is dentry->d_fsdata; for the
recursive call the situation is the same as with configfs_detach_prep()
and the same observation about ->s_dentry->d_fsdata applies.

Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Breno Leitao <leitao@debian.org>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index b456e4de25ab..a6b99bcbddbc 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1093,15 +1093,12 @@ static int configfs_dump(struct configfs_dirent *sd, int level)
  * much on the stack, though, so folks that need this function - be careful
  * about your stack!  Patches will be accepted to make it iterative.
  */
-static int configfs_depend_prep(struct dentry *origin,
+static int configfs_depend_prep(struct configfs_dirent *sd,
 				struct config_item *target)
 {
-	struct configfs_dirent *child_sd, *sd;
+	struct configfs_dirent *child_sd;
 	int ret = 0;
 
-	BUG_ON(!origin || !origin->d_fsdata);
-	sd = origin->d_fsdata;
-
 	if (sd->s_element == target)  /* Boo-yah */
 		goto out;
 
@@ -1109,8 +1106,7 @@ static int configfs_depend_prep(struct dentry *origin,
 		if ((child_sd->s_type & CONFIGFS_DIR) &&
 		    !(child_sd->s_type & CONFIGFS_USET_DROPPING) &&
 		    !(child_sd->s_type & CONFIGFS_USET_CREATING)) {
-			ret = configfs_depend_prep(child_sd->s_dentry,
-						   target);
+			ret = configfs_depend_prep(child_sd, target);
 			if (!ret)
 				goto out;  /* Child path boo-yah */
 		}
@@ -1131,7 +1127,7 @@ static int configfs_do_depend_item(struct dentry *subsys_dentry,
 
 	spin_lock(&configfs_dirent_lock);
 	/* Scan the tree, return 0 if found */
-	ret = configfs_depend_prep(subsys_dentry, target);
+	ret = configfs_depend_prep(subsys_dentry->d_fsdata, target);
 	if (ret)
 		goto out_unlock_dirent_lock;
 
-- 
2.47.3

[PATCH v2 06/18] configfs_do_depend_item(): pass configfs_dirent instead of dentry

[Al Viro]

Again, the only thing it uses the argument for is its ->d_fsdata
and callers already have that - as the matter of fact, they are
passing ->s_dentry of that configfs_dirent, so that the function
could get it back as ->d_fsdata of that.  With nothing else in
dentry even looked at...

configfs_dirent in question is a directory one - in this case those
are subdirectories of root (aka roots of "subsystem" trees).

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index a6b99bcbddbc..c6055a626bef 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -1119,7 +1119,7 @@ static int configfs_depend_prep(struct configfs_dirent *sd,
 	return ret;
 }
 
-static int configfs_do_depend_item(struct dentry *subsys_dentry,
+static int configfs_do_depend_item(struct configfs_dirent *subsys_sd,
 				   struct config_item *target)
 {
 	struct configfs_dirent *p;
@@ -1127,7 +1127,7 @@ static int configfs_do_depend_item(struct dentry *subsys_dentry,
 
 	spin_lock(&configfs_dirent_lock);
 	/* Scan the tree, return 0 if found */
-	ret = configfs_depend_prep(subsys_dentry->d_fsdata, target);
+	ret = configfs_depend_prep(subsys_sd, target);
 	if (ret)
 		goto out_unlock_dirent_lock;
 
@@ -1195,7 +1195,7 @@ int configfs_depend_item(struct configfs_subsystem *subsys,
 	}
 
 	/* Ok, now we can trust subsys/s_item */
-	ret = configfs_do_depend_item(subsys_sd->s_dentry, target);
+	ret = configfs_do_depend_item(subsys_sd, target);
 
 out_unlock_fs:
 	inode_unlock(d_inode(root));
@@ -1297,7 +1297,7 @@ int configfs_depend_item_unlocked(struct configfs_subsystem *caller_subsys,
 	}
 
 	/* Now we can execute core of depend item */
-	ret = configfs_do_depend_item(subsys_sd->s_dentry, target);
+	ret = configfs_do_depend_item(subsys_sd, target);
 
 	if (target_subsys != caller_subsys)
 out_root_unlock:
-- 
2.47.3

[PATCH v2 07/18] configfs_detach_rollback(): pass configfs_dirent instead of dentry

[Al Viro]

same story as with configfs_detach_prep() this function is undoing.

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index c6055a626bef..c420191610c3 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -563,16 +563,15 @@ static int configfs_detach_prep(struct configfs_dirent *parent_sd, struct dentry
  * Walk the tree, resetting CONFIGFS_USET_DROPPING wherever it was
  * set.
  */
-static void configfs_detach_rollback(struct dentry *dentry)
+static void configfs_detach_rollback(struct configfs_dirent *parent_sd)
 {
-	struct configfs_dirent *parent_sd = dentry->d_fsdata;
 	struct configfs_dirent *sd;
 
 	parent_sd->s_type &= ~CONFIGFS_USET_DROPPING;
 
 	list_for_each_entry(sd, &parent_sd->s_children, s_sibling)
 		if (sd->s_type & CONFIGFS_USET_DEFAULT)
-			configfs_detach_rollback(sd->s_dentry);
+			configfs_detach_rollback(sd);
 }
 
 /*
@@ -1537,7 +1536,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 		if (!ret) {
 			ret = configfs_detach_prep(sd, &wait);
 			if (ret)
-				configfs_detach_rollback(dentry);
+				configfs_detach_rollback(sd);
 		}
 		spin_unlock(&configfs_dirent_lock);
 		mutex_unlock(&configfs_symlink_mutex);
@@ -1558,7 +1557,7 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 	frag = sd->s_frag;
 	if (down_write_killable(&frag->frag_sem)) {
 		spin_lock(&configfs_dirent_lock);
-		configfs_detach_rollback(dentry);
+		configfs_detach_rollback(sd);
 		spin_unlock(&configfs_dirent_lock);
 		config_item_put(parent_item);
 		return -EINTR;
-- 
2.47.3

[PATCH v2 08/18] populate_group(): move cleanup on failure to the sole caller

[Al Viro]

... where it folds with configfs_detach_item() into a call of
configfs_detach_group().

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index c420191610c3..01de8ef5fbe6 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -754,17 +754,13 @@ static int populate_groups(struct config_group *group,
 			   struct configfs_fragment *frag)
 {
 	struct config_group *new_group;
-	int ret = 0;
 
 	list_for_each_entry(new_group, &group->default_groups, group_entry) {
-		ret = create_default_group(group, new_group, frag);
-		if (ret) {
-			detach_groups(group);
-			break;
-		}
+		int ret = create_default_group(group, new_group, frag);
+		if (ret)
+			return ret;
 	}
-
-	return ret;
+	return 0;
 }
 
 void configfs_remove_default_groups(struct config_group *group)
@@ -904,6 +900,13 @@ static void configfs_detach_item(struct config_item *item)
 	configfs_remove_dir(item);
 }
 
+/* Caller holds the mutex of the group's inode */
+static void configfs_detach_group(struct config_item *item)
+{
+	detach_groups(to_config_group(item));
+	configfs_detach_item(item);
+}
+
 static int configfs_attach_group(struct config_item *parent_item,
 				 struct config_item *item,
 				 struct dentry *dentry,
@@ -930,7 +933,7 @@ static int configfs_attach_group(struct config_item *parent_item,
 		configfs_adjust_dir_dirent_depth_before_populate(sd);
 		ret = populate_groups(to_config_group(item), frag);
 		if (ret) {
-			configfs_detach_item(item);
+			configfs_detach_group(item);
 			d_inode(dentry)->i_flags |= S_DEAD;
 			dont_mount(dentry);
 		}
@@ -943,13 +946,6 @@ static int configfs_attach_group(struct config_item *parent_item,
 	return ret;
 }
 
-/* Caller holds the mutex of the group's inode */
-static void configfs_detach_group(struct config_item *item)
-{
-	detach_groups(to_config_group(item));
-	configfs_detach_item(item);
-}
-
 /*
  * After the item has been detached from the filesystem view, we are
  * ready to tear it out of the hierarchy.  Notify the client before
-- 
2.47.3

[PATCH v2 09/18] populate_attrs(): move cleanup to the sole caller

[Al Viro]

... where it folds with configfs_remove_dir() into a call of
configfs_detach_item().  Note that at the early failure exit
(before we'd added any children) we were not calling detach_attrs()
only because there it would've been a no-op - nothing added,
nothing there to be removed.

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 01de8ef5fbe6..6580c919ee5a 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -643,25 +643,23 @@ static int populate_attrs(struct config_item *item)
 			if (ops && ops->is_visible && !ops->is_visible(item, attr, i))
 				continue;
 
-			if ((error = configfs_create_file(item, attr)))
-				break;
+			error = configfs_create_file(item, attr);
+			if (error)
+				return error;
 		}
 	}
-	if (!error && t->ct_bin_attrs) {
+	if (t->ct_bin_attrs) {
 		for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
 			if (ops && ops->is_bin_visible && !ops->is_bin_visible(item, bin_attr, i))
 				continue;
 
 			error = configfs_create_bin_file(item, bin_attr);
 			if (error)
-				break;
+				return error;
 		}
 	}
 
-	if (error)
-		detach_attrs(item);
-
-	return error;
+	return 0;
 }
 
 static int configfs_attach_group(struct config_item *parent_item,
@@ -850,6 +848,13 @@ static void link_group(struct config_group *parent_group, struct config_group *g
 		link_group(group, new_group);
 }
 
+/* Caller holds the mutex of the item's inode */
+static void configfs_detach_item(struct config_item *item)
+{
+	detach_attrs(item);
+	configfs_remove_dir(item);
+}
+
 /*
  * The goal is that configfs_attach_item() (and
  * configfs_attach_group()) can be called from either the VFS or this
@@ -882,7 +887,7 @@ static int configfs_attach_item(struct config_item *parent_item,
 			 * we must lock them as rmdir() would.
 			 */
 			inode_lock(d_inode(dentry));
-			configfs_remove_dir(item);
+			configfs_detach_item(item);
 			d_inode(dentry)->i_flags |= S_DEAD;
 			dont_mount(dentry);
 			inode_unlock(d_inode(dentry));
@@ -893,13 +898,6 @@ static int configfs_attach_item(struct config_item *parent_item,
 	return ret;
 }
 
-/* Caller holds the mutex of the item's inode */
-static void configfs_detach_item(struct config_item *item)
-{
-	detach_attrs(item);
-	configfs_remove_dir(item);
-}
-
 /* Caller holds the mutex of the group's inode */
 static void configfs_detach_group(struct config_item *item)
 {
-- 
2.47.3

[PATCH v2 10/18] configfs_remove_dir(), detach_attrs(): switch to passing dentry

[Al Viro]

... and deal with grabbing/dropping it in the sole caller.
After that configfs_remove_dir() becomes an unconditional call of remove_dir(),
so we can fold them together.

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 60 ++++++++++++++++-------------------------------
 1 file changed, 20 insertions(+), 40 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 6580c919ee5a..11e2597fb1e2 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -395,7 +395,18 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
 	return PTR_ERR(inode);
 }
 
-static void remove_dir(struct dentry * d)
+/**
+ * configfs_remove_dir - remove an config_item's directory.
+ * @d:	dentry we're removing.
+ *
+ * The only thing special about this is that we remove any files in
+ * the directory before we remove the directory, and we've inlined
+ * what used to be configfs_rmdir() below, instead of calling separately.
+ *
+ * Caller holds the mutex of the item's inode
+ */
+
+static void configfs_remove_dir(struct dentry *d)
 {
 	struct dentry * parent = dget(d->d_parent);
 
@@ -415,31 +426,6 @@ static void remove_dir(struct dentry * d)
 	dput(parent);
 }
 
-/**
- * configfs_remove_dir - remove an config_item's directory.
- * @item:	config_item we're removing.
- *
- * The only thing special about this is that we remove any files in
- * the directory before we remove the directory, and we've inlined
- * what used to be configfs_rmdir() below, instead of calling separately.
- *
- * Caller holds the mutex of the item's inode
- */
-
-static void configfs_remove_dir(struct config_item * item)
-{
-	struct dentry * dentry = dget(item->ci_dentry);
-
-	if (!dentry)
-		return;
-
-	remove_dir(dentry);
-	/**
-	 * Drop reference from dget() on entrance.
-	 */
-	dput(dentry);
-}
-
 static struct dentry * configfs_lookup(struct inode *dir,
 				       struct dentry *dentry,
 				       unsigned int flags)
@@ -591,17 +577,12 @@ static struct configfs_dirent *next_dirent(struct configfs_dirent *parent,
 	return NULL;
 }
 
-static void detach_attrs(struct config_item * item)
+static void detach_attrs(struct dentry *dentry)
 {
-	struct dentry * dentry = dget(item->ci_dentry);
 	struct configfs_dirent *parent_sd;
 	struct configfs_dirent *sd, *next;
 
-	if (!dentry)
-		return;
-
-	pr_debug("configfs %s: dropping attrs for  dir\n",
-		 dentry->d_name.name);
+	pr_debug("configfs %pd: dropping attrs for  dir\n", dentry);
 
 	parent_sd = dentry->d_fsdata;
 
@@ -617,11 +598,6 @@ static void detach_attrs(struct config_item * item)
 		spin_lock(&configfs_dirent_lock);
 	}
 	spin_unlock(&configfs_dirent_lock);
-
-	/**
-	 * Drop reference from dget() on entrance.
-	 */
-	dput(dentry);
 }
 
 static int populate_attrs(struct config_item *item)
@@ -851,8 +827,12 @@ static void link_group(struct config_group *parent_group, struct config_group *g
 /* Caller holds the mutex of the item's inode */
 static void configfs_detach_item(struct config_item *item)
 {
-	detach_attrs(item);
-	configfs_remove_dir(item);
+	struct dentry *dentry = dget(item->ci_dentry);
+	if (dentry) {
+		detach_attrs(dentry);
+		configfs_remove_dir(dentry);
+		dput(dentry);
+	}
 }
 
 /*
-- 
2.47.3

[PATCH v2 11/18] switch configfs_detach_{group,item}() to passing dentry

[Al Viro]

... and there's no need to grab/drop it, or check for NULL - none
of the callers would even get there with NULL dentry and all of
them have the sucker pinned

Note that if sd is a directory configfs_dirent, we have sd->s_element
pointing to config_item with item->ci_dentry equal to sd->s_dentry.
Which is the only reason why detach_groups() gets away with using
the latter for locking the inode and the former for removal.

Aren't redundant data structures wonderful, for obfuscation if nothing
else?

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 43 +++++++++++++++----------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 11e2597fb1e2..87a0847d88b9 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -642,18 +642,14 @@ static int configfs_attach_group(struct config_item *parent_item,
 				 struct config_item *item,
 				 struct dentry *dentry,
 				 struct configfs_fragment *frag);
-static void configfs_detach_group(struct config_item *item);
+static void configfs_detach_group(struct dentry *dentry);
 
-static void detach_groups(struct config_group *group)
+static void detach_groups(struct dentry *dentry)
 {
-	struct dentry * dentry = dget(group->cg_item.ci_dentry);
 	struct dentry *child;
 	struct configfs_dirent *parent_sd;
 	struct configfs_dirent *sd, *next;
 
-	if (!dentry)
-		return;
-
 	parent_sd = dentry->d_fsdata;
 	spin_lock(&configfs_dirent_lock);
 	for (sd = next_dirent(parent_sd, NULL); sd; sd = next) {
@@ -666,7 +662,7 @@ static void detach_groups(struct config_group *group)
 
 		inode_lock(d_inode(child));
 
-		configfs_detach_group(sd->s_element);
+		configfs_detach_group(child);
 		d_inode(child)->i_flags |= S_DEAD;
 		dont_mount(child);
 
@@ -677,11 +673,6 @@ static void detach_groups(struct config_group *group)
 		spin_lock(&configfs_dirent_lock);
 	}
 	spin_unlock(&configfs_dirent_lock);
-
-	/**
-	 * Drop reference from dget() on entrance.
-	 */
-	dput(dentry);
 }
 
 /*
@@ -825,14 +816,10 @@ static void link_group(struct config_group *parent_group, struct config_group *g
 }
 
 /* Caller holds the mutex of the item's inode */
-static void configfs_detach_item(struct config_item *item)
+static void configfs_detach_item(struct dentry *dentry)
 {
-	struct dentry *dentry = dget(item->ci_dentry);
-	if (dentry) {
-		detach_attrs(dentry);
-		configfs_remove_dir(dentry);
-		dput(dentry);
-	}
+	detach_attrs(dentry);
+	configfs_remove_dir(dentry);
 }
 
 /*
@@ -867,7 +854,7 @@ static int configfs_attach_item(struct config_item *parent_item,
 			 * we must lock them as rmdir() would.
 			 */
 			inode_lock(d_inode(dentry));
-			configfs_detach_item(item);
+			configfs_detach_item(dentry);
 			d_inode(dentry)->i_flags |= S_DEAD;
 			dont_mount(dentry);
 			inode_unlock(d_inode(dentry));
@@ -879,10 +866,10 @@ static int configfs_attach_item(struct config_item *parent_item,
 }
 
 /* Caller holds the mutex of the group's inode */
-static void configfs_detach_group(struct config_item *item)
+static void configfs_detach_group(struct dentry *dentry)
 {
-	detach_groups(to_config_group(item));
-	configfs_detach_item(item);
+	detach_groups(dentry);
+	configfs_detach_item(dentry);
 }
 
 static int configfs_attach_group(struct config_item *parent_item,
@@ -911,7 +898,7 @@ static int configfs_attach_group(struct config_item *parent_item,
 		configfs_adjust_dir_dirent_depth_before_populate(sd);
 		ret = populate_groups(to_config_group(item), frag);
 		if (ret) {
-			configfs_detach_group(item);
+			configfs_detach_group(dentry);
 			d_inode(dentry)->i_flags |= S_DEAD;
 			dont_mount(dentry);
 		}
@@ -1549,13 +1536,13 @@ static int configfs_rmdir(struct inode *dir, struct dentry *dentry)
 		dead_item_owner = item->ci_type->ct_owner;
 
 	if (sd->s_type & CONFIGFS_USET_DIR) {
-		configfs_detach_group(item);
+		configfs_detach_group(dentry);
 
 		mutex_lock(&subsys->su_mutex);
 		client_disconnect_notify(parent_item, item);
 		unlink_group(to_config_group(item));
 	} else {
-		configfs_detach_item(item);
+		configfs_detach_item(dentry);
 
 		mutex_lock(&subsys->su_mutex);
 		client_disconnect_notify(parent_item, item);
@@ -1808,7 +1795,7 @@ void configfs_unregister_group(struct config_group *group)
 	configfs_detach_prep(sd, NULL);
 	spin_unlock(&configfs_dirent_lock);
 
-	configfs_detach_group(&group->cg_item);
+	configfs_detach_group(dentry);
 	d_inode(dentry)->i_flags |= S_DEAD;
 	dont_mount(dentry);
 	d_drop(dentry);
@@ -1957,7 +1944,7 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
 	}
 	spin_unlock(&configfs_dirent_lock);
 	mutex_unlock(&configfs_symlink_mutex);
-	configfs_detach_group(&group->cg_item);
+	configfs_detach_group(dentry);
 	d_inode(dentry)->i_flags |= S_DEAD;
 	dont_mount(dentry);
 	inode_unlock(d_inode(dentry));
-- 
2.47.3

[PATCH v2 12/18] configfs: dentry refcount needs to be pinned only once

[Al Viro]

currently we have a weird situation where
	* symlinks and roots of subtrees created by mkdir are pinned once
	* subdirectories of subtrees created by mkdir are pinned twice
	* roots of subtrees created by register_{group,subsystem} are pinned
twice.

It makes things harder to follow for no good reason.  The goal is to
encapsulate the unbalanced dget/dput into d_{make,discard}_persisitent()
and, preferably, allow a use of simple_recursive_removal() or analogue
thereof.  So let's regularize that and pin things only once.

create_default_group() and configfs_register_subsystem() don't need to
keep their reference around on success - configfs_create_dir() has pinned
the sucker already.  So we can drop the reference passed to
configfs_create_dir() (via configfs_attach_group(), etc.) both on success
and on failure.  On the removal side we no longer have the double references,
so we need an explicit dget() to compensate.

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 87a0847d88b9..4836ffb3b70f 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -657,7 +657,7 @@ static void detach_groups(struct dentry *dentry)
 		if (!(sd->s_type & CONFIGFS_USET_DEFAULT))
 			continue;
 
-		child = sd->s_dentry;
+		child = dget(sd->s_dentry);
 		spin_unlock(&configfs_dirent_lock);
 
 		inode_lock(d_inode(child));
@@ -708,8 +708,8 @@ static int create_default_group(struct config_group *parent_group,
 		} else {
 			BUG_ON(d_inode(child));
 			d_drop(child);
-			dput(child);
 		}
+		dput(child);
 	}
 
 	return ret;
@@ -1781,7 +1781,7 @@ EXPORT_SYMBOL(configfs_register_group);
 void configfs_unregister_group(struct config_group *group)
 {
 	struct configfs_subsystem *subsys = group->cg_subsys;
-	struct dentry *dentry = group->cg_item.ci_dentry;
+	struct dentry *dentry = dget(group->cg_item.ci_dentry);
 	struct dentry *parent = group->cg_item.ci_parent->ci_dentry;
 	struct configfs_dirent *sd = dentry->d_fsdata;
 	struct configfs_fragment *frag = sd->s_frag;
@@ -1896,12 +1896,12 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
 		if (err) {
 			BUG_ON(d_inode(dentry));
 			d_drop(dentry);
-			dput(dentry);
 		} else {
 			spin_lock(&configfs_dirent_lock);
 			configfs_dir_set_ready(dentry->d_fsdata);
 			spin_unlock(&configfs_dirent_lock);
 		}
+		dput(dentry);
 	}
 
 	inode_unlock(d_inode(root));
@@ -1920,7 +1920,7 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
 void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
 {
 	struct config_group *group = &subsys->su_group;
-	struct dentry *dentry = group->cg_item.ci_dentry;
+	struct dentry *dentry = dget(group->cg_item.ci_dentry);
 	struct dentry *root = dentry->d_sb->s_root;
 	struct configfs_dirent *sd = dentry->d_fsdata;
 	struct configfs_fragment *frag = sd->s_frag;
-- 
2.47.3

[PATCH v2 13/18] configfs: mark pinned dentries persistent

[Al Viro]

on the removal side we can (finally) get rid of __simple_unlink()
and __simple_rmdir() kludges now that dentries in question are
properly marked persistent - simple_unlink() and simple_rmdir()
will do the right thing for those.

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c     | 13 +++----------
 fs/configfs/symlink.c |  3 +--
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 4836ffb3b70f..479d37b8d806 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -315,9 +315,7 @@ static int configfs_create_dir(struct config_item *item, struct dentry *dentry,
 	inode->i_fop = &configfs_dir_operations;
 	/* directory inodes start off with i_nlink == 2 (for "." entry) */
 	inc_nlink(inode);
-	d_instantiate(dentry, inode);
-	/* already hashed */
-	dget(dentry);  /* pin directory dentries in core */
+	d_make_persistent(dentry, inode);
 	inc_nlink(d_inode(p));
 	item->ci_dentry = dentry;
 	return 0;
@@ -385,8 +383,7 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
 
 	inode->i_link = body;
 	inode->i_op = &configfs_symlink_inode_operations;
-	d_instantiate(dentry, inode);
-	dget(dentry);  /* pin link dentries in core */
+	d_make_persistent(dentry, inode);
 	return 0;
 
 out_remove:
@@ -413,12 +410,8 @@ static void configfs_remove_dir(struct dentry *d)
 	configfs_remove_dirent(d);
 
 	if (d_really_is_positive(d)) {
-		if (likely(simple_empty(d))) {
-			__simple_rmdir(d_inode(parent),d);
-			dput(d);
-		} else {
+		if (unlikely(simple_rmdir(d_inode(parent), d)))
 			pr_warn("remove_dir (%pd): attributes remain", d);
-		}
 	}
 
 	pr_debug(" o %pd removing done (%d)\n", d, d_count(d));
diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
index f3f79c67add5..31eb28b27309 100644
--- a/fs/configfs/symlink.c
+++ b/fs/configfs/symlink.c
@@ -229,9 +229,8 @@ int configfs_unlink(struct inode *dir, struct dentry *dentry)
 	spin_lock(&configfs_dirent_lock);
 	list_del_init(&sd->s_sibling);
 	spin_unlock(&configfs_dirent_lock);
-	configfs_drop_dentry(sd, dentry->d_parent);
-	dput(dentry);
 	configfs_put(sd);
+	simple_unlink(dir, dentry);
 
 	/*
 	 * drop_link() must be called before
-- 
2.47.3

[PATCH v2 14/18] kill configfs_drop_dentry()

[Al Viro]

Fold into the only remaining user, don't bother with the timestamps
of parent - we are going to rmdir it shortly anyway, which will
override those.

Fix the locking of inode, while we are at it - updating the link
count and timestamps ought to be done with the inode locked.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/configfs_internal.h |  1 -
 fs/configfs/dir.c               | 24 +++++++++++++++++++++++-
 fs/configfs/inode.c             | 22 ----------------------
 3 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/fs/configfs/configfs_internal.h b/fs/configfs/configfs_internal.h
index 0b969d0eb8ff..acdeea8e2d69 100644
--- a/fs/configfs/configfs_internal.h
+++ b/fs/configfs/configfs_internal.h
@@ -76,7 +76,6 @@ extern int configfs_make_dirent(struct configfs_dirent *, struct dentry *,
 extern int configfs_dirent_is_ready(struct configfs_dirent *);
 
 extern const unsigned char * configfs_get_name(struct configfs_dirent *sd);
-extern void configfs_drop_dentry(struct configfs_dirent *sd, struct dentry *parent);
 extern int configfs_setattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, struct iattr *iattr);
 
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 479d37b8d806..4a4bad1741cf 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -581,12 +581,34 @@ static void detach_attrs(struct dentry *dentry)
 
 	spin_lock(&configfs_dirent_lock);
 	for (sd = next_dirent(parent_sd, NULL); sd; sd = next) {
+		struct dentry *child;
+
 		next = next_dirent(parent_sd, sd);
 		if (!(sd->s_type & CONFIGFS_NOT_PINNED))
 			continue;
 		list_del_init(&sd->s_sibling);
+		child = sd->s_dentry;
+		if (child) {
+			spin_lock(&child->d_lock);
+			if (simple_positive(child)) {
+				dget_dlock(child);
+				__d_drop(child);
+				spin_unlock(&child->d_lock);
+			} else {
+				spin_unlock(&child->d_lock);
+				child = NULL;
+			}
+		}
 		spin_unlock(&configfs_dirent_lock);
-		configfs_drop_dentry(sd, dentry);
+		if (child) {
+			struct inode *inode = child->d_inode;
+
+			inode_lock_nested(inode, I_MUTEX_NONDIR2);
+			inode_set_ctime_current(inode);
+			drop_nlink(inode);
+			inode_unlock(inode);
+			dput(child);
+		}
 		configfs_put(sd);
 		spin_lock(&configfs_dirent_lock);
 	}
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index bc507b720a01..3c26b6c443be 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -195,25 +195,3 @@ const unsigned char * configfs_get_name(struct configfs_dirent *sd)
 	}
 	return NULL;
 }
-
-
-/*
- * Unhashes the dentry corresponding to given configfs_dirent
- * Called with parent inode's i_mutex held.
- */
-void configfs_drop_dentry(struct configfs_dirent * sd, struct dentry * parent)
-{
-	struct dentry * dentry = sd->s_dentry;
-
-	if (dentry) {
-		spin_lock(&dentry->d_lock);
-		if (simple_positive(dentry)) {
-			dget_dlock(dentry);
-			__d_drop(dentry);
-			spin_unlock(&dentry->d_lock);
-			__simple_unlink(d_inode(parent), dentry);
-			dput(dentry);
-		} else
-			spin_unlock(&dentry->d_lock);
-	}
-}
-- 
2.47.3

[PATCH v2 15/18] configfs_create(): lift parent timestamp updates into callers

[Al Viro]

... and do *not* do it in ->lookup() case.  stat foo/bar
should not update mtime of foo, TYVM...

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c   | 6 +++++-
 fs/configfs/inode.c | 3 ---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 4a4bad1741cf..b382d60e7ebc 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -296,6 +296,7 @@ static int configfs_create_dir(struct config_item *item, struct dentry *dentry,
 	int error;
 	umode_t mode = S_IFDIR| S_IRWXU | S_IRUGO | S_IXUGO;
 	struct dentry *p = dentry->d_parent;
+	struct inode *p_inode = d_inode(p);
 	struct inode *inode;
 
 	BUG_ON(!item);
@@ -316,7 +317,8 @@ static int configfs_create_dir(struct config_item *item, struct dentry *dentry,
 	/* directory inodes start off with i_nlink == 2 (for "." entry) */
 	inc_nlink(inode);
 	d_make_persistent(dentry, inode);
-	inc_nlink(d_inode(p));
+	inc_nlink(p_inode);
+	inode_set_mtime_to_ts(p_inode, inode_set_ctime_current(p_inode));
 	item->ci_dentry = dentry;
 	return 0;
 
@@ -370,6 +372,7 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
 	int err = 0;
 	umode_t mode = S_IFLNK | S_IRWXUGO;
 	struct configfs_dirent *p = parent->d_fsdata;
+	struct inode *p_inode = d_inode(parent);
 	struct inode *inode;
 
 	err = configfs_make_dirent(p, dentry, target, mode, CONFIGFS_ITEM_LINK,
@@ -384,6 +387,7 @@ int configfs_create_link(struct configfs_dirent *target, struct dentry *parent,
 	inode->i_link = body;
 	inode->i_op = &configfs_symlink_inode_operations;
 	d_make_persistent(dentry, inode);
+	inode_set_mtime_to_ts(p_inode, inode_set_ctime_current(p_inode));
 	return 0;
 
 out_remove:
diff --git a/fs/configfs/inode.c b/fs/configfs/inode.c
index 3c26b6c443be..68290fe0e374 100644
--- a/fs/configfs/inode.c
+++ b/fs/configfs/inode.c
@@ -157,7 +157,6 @@ struct inode *configfs_create(struct dentry *dentry, umode_t mode)
 {
 	struct inode *inode = NULL;
 	struct configfs_dirent *sd;
-	struct inode *p_inode;
 
 	if (!dentry)
 		return ERR_PTR(-ENOENT);
@@ -170,8 +169,6 @@ struct inode *configfs_create(struct dentry *dentry, umode_t mode)
 	if (!inode)
 		return ERR_PTR(-ENOMEM);
 
-	p_inode = d_inode(dentry->d_parent);
-	inode_set_mtime_to_ts(p_inode, inode_set_ctime_current(p_inode));
 	configfs_set_inode_lock_class(sd, inode);
 	return inode;
 }
-- 
2.47.3

[PATCH v2 16/18] configs_attach_item(): drop unused parent_item argument

[Al Viro]

That argument has been unused since the initial merge in 2005.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index b382d60e7ebc..2f3f3f504e2c 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -856,8 +856,7 @@ static void configfs_detach_item(struct dentry *dentry)
  * clean up the configfs items, and they expect their callers will
  * handle the dcache bits.
  */
-static int configfs_attach_item(struct config_item *parent_item,
-				struct config_item *item,
+static int configfs_attach_item(struct config_item *item,
 				struct dentry *dentry,
 				struct configfs_fragment *frag)
 {
@@ -899,7 +898,7 @@ static int configfs_attach_group(struct config_item *parent_item,
 	int ret;
 	struct configfs_dirent *sd;
 
-	ret = configfs_attach_item(parent_item, item, dentry, frag);
+	ret = configfs_attach_item(item, dentry, frag);
 	if (!ret) {
 		sd = dentry->d_fsdata;
 		sd->s_type |= CONFIGFS_USET_DIR;
@@ -1426,7 +1425,7 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	if (group)
 		ret = configfs_attach_group(parent_item, item, dentry, frag);
 	else
-		ret = configfs_attach_item(parent_item, item, dentry, frag);
+		ret = configfs_attach_item(item, dentry, frag);
 
 	spin_lock(&configfs_dirent_lock);
 	sd->s_type &= ~CONFIGFS_USET_IN_MKDIR;
-- 
2.47.3

[PATCH v2 17/18] configfs_attach_group(): drop the unused parent_item argument

[Al Viro]

This one *was* used - for passing it to configfs_attach_item(), which
didn't use the value passed to it.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 2f3f3f504e2c..8fc05fe69992 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -657,8 +657,7 @@ static int populate_attrs(struct config_item *item)
 	return 0;
 }
 
-static int configfs_attach_group(struct config_item *parent_item,
-				 struct config_item *item,
+static int configfs_attach_group(struct config_item *item,
 				 struct dentry *dentry,
 				 struct configfs_fragment *frag);
 static void configfs_detach_group(struct dentry *dentry);
@@ -719,8 +718,7 @@ static int create_default_group(struct config_group *parent_group,
 	if (child) {
 		d_add(child, NULL);
 
-		ret = configfs_attach_group(&parent_group->cg_item,
-					    &group->cg_item, child, frag);
+		ret = configfs_attach_group(&group->cg_item, child, frag);
 		if (!ret) {
 			sd = child->d_fsdata;
 			sd->s_type |= CONFIGFS_USET_DEFAULT;
@@ -890,8 +888,7 @@ static void configfs_detach_group(struct dentry *dentry)
 	configfs_detach_item(dentry);
 }
 
-static int configfs_attach_group(struct config_item *parent_item,
-				 struct config_item *item,
+static int configfs_attach_group(struct config_item *item,
 				 struct dentry *dentry,
 				 struct configfs_fragment *frag)
 {
@@ -1423,7 +1420,7 @@ static struct dentry *configfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	spin_unlock(&configfs_dirent_lock);
 
 	if (group)
-		ret = configfs_attach_group(parent_item, item, dentry, frag);
+		ret = configfs_attach_group(item, dentry, frag);
 	else
 		ret = configfs_attach_item(item, dentry, frag);
 
@@ -1908,8 +1905,7 @@ int configfs_register_subsystem(struct configfs_subsystem *subsys)
 
 		err = configfs_dirent_exists(dentry);
 		if (!err)
-			err = configfs_attach_group(sd->s_element,
-						    &group->cg_item,
+			err = configfs_attach_group(&group->cg_item,
 						    dentry, frag);
 		if (err) {
 			BUG_ON(d_inode(dentry));
-- 
2.47.3

[PATCH v2 18/18] create_default_group(): pass parent's dentry instead of config_group

[Al Viro]

the only way parent_group is used there...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 fs/configfs/dir.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 8fc05fe69992..4d82375b0cfd 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -701,14 +701,14 @@ static void detach_groups(struct dentry *dentry)
  * We could, perhaps, tweak our parent's ->mkdir for a minute and
  * try using vfs_mkdir.  Just a thought.
  */
-static int create_default_group(struct config_group *parent_group,
+static int create_default_group(struct dentry *parent,
 				struct config_group *group,
 				struct configfs_fragment *frag)
 {
 	int ret;
 	struct configfs_dirent *sd;
 	/* We trust the caller holds a reference to parent */
-	struct dentry *child, *parent = parent_group->cg_item.ci_dentry;
+	struct dentry *child;
 
 	if (!group->cg_item.ci_name)
 		group->cg_item.ci_name = group->cg_item.ci_namebuf;
@@ -735,10 +735,11 @@ static int create_default_group(struct config_group *parent_group,
 static int populate_groups(struct config_group *group,
 			   struct configfs_fragment *frag)
 {
+	struct dentry *parent = group->cg_item.ci_dentry;
 	struct config_group *new_group;
 
 	list_for_each_entry(new_group, &group->default_groups, group_entry) {
-		int ret = create_default_group(group, new_group, frag);
+		int ret = create_default_group(parent, new_group, frag);
 		if (ret)
 			return ret;
 	}
@@ -1767,7 +1768,7 @@ int configfs_register_group(struct config_group *parent_group,
 	parent = parent_group->cg_item.ci_dentry;
 
 	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
-	ret = create_default_group(parent_group, group, frag);
+	ret = create_default_group(parent, group, frag);
 	if (ret)
 		goto err_out;
 
-- 
2.47.3