[PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors

[PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors

[Aldo Ariel Panzardo]

nilfs_find_or_create_root() publishes a new root in the checkpoint
tree before creating its sysfs object.  If sysfs registration fails,
the root is freed while it is still reachable from the tree.  Merely
erasing it in the error path is insufficient because a concurrent
lookup may already hold a reference.

Serialize root creation and removal, finish sysfs registration before
publishing the root, and wait for the embedded kobject release before
freeing its container.  The wait also makes normal root removal safe
when kobject release is delayed.

Fixes: dd70edbde262 ("nilfs2: integrate sysfs support into driver")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 fs/nilfs2/the_nilfs.c | 38 +++++++++++++++++++++++++-------------
 fs/nilfs2/the_nilfs.h |  3 +++
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 7b23e373a..01389dba2 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -71,6 +71,7 @@ struct the_nilfs *alloc_nilfs(struct super_block *sb)
 	spin_lock_init(&nilfs->ns_last_segment_lock);
 	nilfs->ns_cptree = RB_ROOT;
 	spin_lock_init(&nilfs->ns_cptree_lock);
+	mutex_init(&nilfs->ns_cptree_mutex);
 	init_rwsem(&nilfs->ns_segctor_sem);
 	nilfs->ns_sb_update_freq = NILFS_SB_FREQ;
 
@@ -881,8 +882,15 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno)
 	if (!new)
 		return NULL;
 
-	spin_lock(&nilfs->ns_cptree_lock);
+	new->cno = cno;
+	new->ifile = NULL;
+	new->nilfs = nilfs;
+	refcount_set(&new->count, 1);
+	atomic64_set(&new->inodes_count, 0);
+	atomic64_set(&new->blocks_count, 0);
 
+	mutex_lock(&nilfs->ns_cptree_mutex);
+	spin_lock(&nilfs->ns_cptree_lock);
 	p = &nilfs->ns_cptree.rb_node;
 	parent = NULL;
 
@@ -897,29 +905,28 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno)
 		} else {
 			refcount_inc(&root->count);
 			spin_unlock(&nilfs->ns_cptree_lock);
+			mutex_unlock(&nilfs->ns_cptree_mutex);
 			kfree(new);
 			return root;
 		}
 	}
-
-	new->cno = cno;
-	new->ifile = NULL;
-	new->nilfs = nilfs;
-	refcount_set(&new->count, 1);
-	atomic64_set(&new->inodes_count, 0);
-	atomic64_set(&new->blocks_count, 0);
-
-	rb_link_node(&new->rb_node, parent, p);
-	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
-
 	spin_unlock(&nilfs->ns_cptree_lock);
 
 	err = nilfs_sysfs_create_snapshot_group(new);
 	if (err) {
+		mutex_unlock(&nilfs->ns_cptree_mutex);
+		wait_for_completion(&new->snapshot_kobj_unregister);
 		kfree(new);
-		new = NULL;
+		return NULL;
 	}
 
+	spin_lock(&nilfs->ns_cptree_lock);
+	rb_link_node(&new->rb_node, parent, p);
+	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
+
+	spin_unlock(&nilfs->ns_cptree_lock);
+	mutex_unlock(&nilfs->ns_cptree_mutex);
+
 	return new;
 }
 
@@ -927,13 +934,18 @@ void nilfs_put_root(struct nilfs_root *root)
 {
 	struct the_nilfs *nilfs = root->nilfs;
 
+	mutex_lock(&nilfs->ns_cptree_mutex);
 	if (refcount_dec_and_lock(&root->count, &nilfs->ns_cptree_lock)) {
 		rb_erase(&root->rb_node, &nilfs->ns_cptree);
 		spin_unlock(&nilfs->ns_cptree_lock);
 
 		nilfs_sysfs_delete_snapshot_group(root);
+		mutex_unlock(&nilfs->ns_cptree_mutex);
+		wait_for_completion(&root->snapshot_kobj_unregister);
 		iput(root->ifile);
 
 		kfree(root);
+	} else {
+		mutex_unlock(&nilfs->ns_cptree_mutex);
 	}
 }
diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
index 4776a70f0..72affad9e 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -68,6 +68,7 @@ enum {
  * @ns_sufile: segusage file inode
  * @ns_cptree: rb-tree of all mounted checkpoints (nilfs_root)
  * @ns_cptree_lock: lock protecting @ns_cptree
+ * @ns_cptree_mutex: mutex serializing checkpoint root creation and removal
  * @ns_dirty_files: list of dirty files
  * @ns_inode_lock: lock protecting @ns_dirty_files
  * @ns_gc_inodes: dummy inodes to keep live blocks
@@ -151,6 +152,8 @@ struct the_nilfs {
 	/* Checkpoint tree */
 	struct rb_root		ns_cptree;
 	spinlock_t		ns_cptree_lock;
+	/* Serialize root creation and removal. */
+	struct mutex		ns_cptree_mutex;
 
 	/* Dirty inode list */
 	struct list_head	ns_dirty_files;
-- 
2.43.0

Re: [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors

[Viacheslav Dubeyko]

On Tue, 2026-09-15 at 01:12 -0300, Aldo Ariel Panzardo wrote:
> nilfs_find_or_create_root() publishes a new root in the checkpoint
> tree before creating its sysfs object.  If sysfs registration fails,
> the root is freed while it is still reachable from the tree.  Merely
> erasing it in the error path is insufficient because a concurrent
> lookup may already hold a reference.
> 
> Serialize root creation and removal, finish sysfs registration before
> publishing the root, and wait for the embedded kobject release before
> freeing its container.  The wait also makes normal root removal safe
> when kobject release is delayed.
> 
> Fixes: dd70edbde262 ("nilfs2: integrate sysfs support into driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
>  fs/nilfs2/the_nilfs.c | 38 +++++++++++++++++++++++++-------------
>  fs/nilfs2/the_nilfs.h |  3 +++
>  2 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index 7b23e373a..01389dba2 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -71,6 +71,7 @@ struct the_nilfs *alloc_nilfs(struct super_block
> *sb)
>  	spin_lock_init(&nilfs->ns_last_segment_lock);
>  	nilfs->ns_cptree = RB_ROOT;
>  	spin_lock_init(&nilfs->ns_cptree_lock);
> +	mutex_init(&nilfs->ns_cptree_mutex);
>  	init_rwsem(&nilfs->ns_segctor_sem);
>  	nilfs->ns_sb_update_freq = NILFS_SB_FREQ;
>  
> @@ -881,8 +882,15 @@ nilfs_find_or_create_root(struct the_nilfs
> *nilfs, __u64 cno)
>  	if (!new)
>  		return NULL;
>  
> -	spin_lock(&nilfs->ns_cptree_lock);
> +	new->cno = cno;
> +	new->ifile = NULL;
> +	new->nilfs = nilfs;
> +	refcount_set(&new->count, 1);
> +	atomic64_set(&new->inodes_count, 0);
> +	atomic64_set(&new->blocks_count, 0);
>  
> +	mutex_lock(&nilfs->ns_cptree_mutex);
> +	spin_lock(&nilfs->ns_cptree_lock);
>  	p = &nilfs->ns_cptree.rb_node;
>  	parent = NULL;
>  
> @@ -897,29 +905,28 @@ nilfs_find_or_create_root(struct the_nilfs
> *nilfs, __u64 cno)
>  		} else {
>  			refcount_inc(&root->count);
>  			spin_unlock(&nilfs->ns_cptree_lock);
> +			mutex_unlock(&nilfs->ns_cptree_mutex);
>  			kfree(new);
>  			return root;
>  		}
>  	}
> -
> -	new->cno = cno;
> -	new->ifile = NULL;
> -	new->nilfs = nilfs;
> -	refcount_set(&new->count, 1);
> -	atomic64_set(&new->inodes_count, 0);
> -	atomic64_set(&new->blocks_count, 0);
> -
> -	rb_link_node(&new->rb_node, parent, p);
> -	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
> -
>  	spin_unlock(&nilfs->ns_cptree_lock);
>  
>  	err = nilfs_sysfs_create_snapshot_group(new);
>  	if (err) {
> +		mutex_unlock(&nilfs->ns_cptree_mutex);
> +		wait_for_completion(&new->snapshot_kobj_unregister);
>  		kfree(new);
> -		new = NULL;
> +		return NULL;
>  	}
>  
> +	spin_lock(&nilfs->ns_cptree_lock);
> +	rb_link_node(&new->rb_node, parent, p);
> +	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
> +
> +	spin_unlock(&nilfs->ns_cptree_lock);
> +	mutex_unlock(&nilfs->ns_cptree_mutex);
> +
>  	return new;
>  }
>  
> @@ -927,13 +934,18 @@ void nilfs_put_root(struct nilfs_root *root)
>  {
>  	struct the_nilfs *nilfs = root->nilfs;
>  
> +	mutex_lock(&nilfs->ns_cptree_mutex);
>  	if (refcount_dec_and_lock(&root->count, &nilfs-
> >ns_cptree_lock)) {
>  		rb_erase(&root->rb_node, &nilfs->ns_cptree);
>  		spin_unlock(&nilfs->ns_cptree_lock);
>  
>  		nilfs_sysfs_delete_snapshot_group(root);
> +		mutex_unlock(&nilfs->ns_cptree_mutex);
> +		wait_for_completion(&root-
> >snapshot_kobj_unregister);
>  		iput(root->ifile);
>  
>  		kfree(root);
> +	} else {
> +		mutex_unlock(&nilfs->ns_cptree_mutex);
>  	}
>  }
> diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
> index 4776a70f0..72affad9e 100644
> --- a/fs/nilfs2/the_nilfs.h
> +++ b/fs/nilfs2/the_nilfs.h
> @@ -68,6 +68,7 @@ enum {
>   * @ns_sufile: segusage file inode
>   * @ns_cptree: rb-tree of all mounted checkpoints (nilfs_root)
>   * @ns_cptree_lock: lock protecting @ns_cptree
> + * @ns_cptree_mutex: mutex serializing checkpoint root creation and
> removal
>   * @ns_dirty_files: list of dirty files
>   * @ns_inode_lock: lock protecting @ns_dirty_files
>   * @ns_gc_inodes: dummy inodes to keep live blocks
> @@ -151,6 +152,8 @@ struct the_nilfs {
>  	/* Checkpoint tree */
>  	struct rb_root		ns_cptree;
>  	spinlock_t		ns_cptree_lock;
> +	/* Serialize root creation and removal. */
> +	struct mutex		ns_cptree_mutex;

Do we really need another lock here? If the ns_cptree_lock is not good
enough, then, maybe, we need to change the lock type? But adding the
another lock for ns_cptree looks strange for my taste.

Thanks,
Slava.

>  
>  	/* Dirty inode list */
>  	struct list_head	ns_dirty_files;

Re: [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors

[Aldo Ariel Panzardo]

Hi Slava,

You're right, the extra mutex is unnecessary. I checked and all
callers of ns_cptree_lock are in process context, so converting it
to a mutex is safe. nilfs_put_root() can then use
refcount_dec_and_mutex_lock() instead of refcount_dec_and_lock().

I'll send a v2 that replaces the spinlock with a mutex rather than
adding a second lock.

Thanks for the review,
Aldo

[PATCH v2] nilfs2: fix checkpoint root lifetime on sysfs errors

[Aldo Ariel Panzardo]

nilfs_find_or_create_root() publishes a new root in the checkpoint
tree before creating its sysfs object.  If sysfs registration fails,
the root is freed while it is still reachable from the tree.  A
concurrent nilfs_lookup_root() can then dereference freed memory.

The fix needs the lock to be held across the sysfs call, but
nilfs_sysfs_create_snapshot_group() can sleep, so the existing
spinlock is not suitable.

Convert ns_cptree_lock from a spinlock to a mutex.  All existing
callers are in process context (mount, lookup, segctor, recovery),
and nilfs_put_root() can use refcount_dec_and_mutex_lock() as the
atomic decrement-and-acquire primitive.

With the mutex, nilfs_find_or_create_root() can hold it across the
sysfs registration and only insert the root into the rbtree after
sysfs succeeds.  On failure, the root was never visible and can be
freed after waiting for the kobject release callback to complete.

Both the creation error path and the normal removal path must wait
for the embedded kobject release via wait_for_completion() before
freeing the container, because kobject_put() does not guarantee
synchronous release (CONFIG_DEBUG_KOBJECT_RELEASE defers it).

Fixes: dd70edbde262 ("nilfs2: integrate sysfs support into driver")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---

Changes in v2:
  - Convert ns_cptree_lock from spinlock to mutex instead of adding
    a second lock, as suggested by Viacheslav Dubeyko.  Use
    refcount_dec_and_mutex_lock() in nilfs_put_root().

 fs/nilfs2/the_nilfs.c | 35 +++++++++++++++++++----------------
 fs/nilfs2/the_nilfs.h |  2 +-
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 7b23e373a1..5bfa853471 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -70,7 +70,7 @@ struct the_nilfs *alloc_nilfs(struct super_block *sb)
 	spin_lock_init(&nilfs->ns_inode_lock);
 	spin_lock_init(&nilfs->ns_last_segment_lock);
 	nilfs->ns_cptree = RB_ROOT;
-	spin_lock_init(&nilfs->ns_cptree_lock);
+	mutex_init(&nilfs->ns_cptree_lock);
 	init_rwsem(&nilfs->ns_segctor_sem);
 	nilfs->ns_sb_update_freq = NILFS_SB_FREQ;
 
@@ -846,7 +846,7 @@ struct nilfs_root *nilfs_lookup_root(struct the_nilfs *nilfs, __u64 cno)
 	struct rb_node *n;
 	struct nilfs_root *root;
 
-	spin_lock(&nilfs->ns_cptree_lock);
+	mutex_lock(&nilfs->ns_cptree_lock);
 	n = nilfs->ns_cptree.rb_node;
 	while (n) {
 		root = rb_entry(n, struct nilfs_root, rb_node);
@@ -857,11 +857,11 @@ struct nilfs_root *nilfs_lookup_root(struct the_nilfs *nilfs, __u64 cno)
 			n = n->rb_right;
 		} else {
 			refcount_inc(&root->count);
-			spin_unlock(&nilfs->ns_cptree_lock);
+			mutex_unlock(&nilfs->ns_cptree_lock);
 			return root;
 		}
 	}
-	spin_unlock(&nilfs->ns_cptree_lock);
+	mutex_unlock(&nilfs->ns_cptree_lock);
 
 	return NULL;
 }
@@ -881,7 +881,7 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno)
 	if (!new)
 		return NULL;
 
-	spin_lock(&nilfs->ns_cptree_lock);
+	mutex_lock(&nilfs->ns_cptree_lock);
 
 	p = &nilfs->ns_cptree.rb_node;
 	parent = NULL;
@@ -896,7 +896,7 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno)
 			p = &(*p)->rb_right;
 		} else {
 			refcount_inc(&root->count);
-			spin_unlock(&nilfs->ns_cptree_lock);
+			mutex_unlock(&nilfs->ns_cptree_lock);
 			kfree(new);
 			return root;
 		}
@@ -909,17 +909,19 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno)
 	atomic64_set(&new->inodes_count, 0);
 	atomic64_set(&new->blocks_count, 0);
 
-	rb_link_node(&new->rb_node, parent, p);
-	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
-
-	spin_unlock(&nilfs->ns_cptree_lock);
-
 	err = nilfs_sysfs_create_snapshot_group(new);
 	if (err) {
+		mutex_unlock(&nilfs->ns_cptree_lock);
+		wait_for_completion(&new->snapshot_kobj_unregister);
 		kfree(new);
-		new = NULL;
+		return NULL;
 	}
 
+	rb_link_node(&new->rb_node, parent, p);
+	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
+
+	mutex_unlock(&nilfs->ns_cptree_lock);
+
 	return new;
 }
 
@@ -927,13 +929,14 @@ void nilfs_put_root(struct nilfs_root *root)
 {
 	struct the_nilfs *nilfs = root->nilfs;
 
-	if (refcount_dec_and_lock(&root->count, &nilfs->ns_cptree_lock)) {
+	if (refcount_dec_and_mutex_lock(&root->count,
+					&nilfs->ns_cptree_lock)) {
 		rb_erase(&root->rb_node, &nilfs->ns_cptree);
-		spin_unlock(&nilfs->ns_cptree_lock);
-
 		nilfs_sysfs_delete_snapshot_group(root);
-		iput(root->ifile);
+		wait_for_completion(&root->snapshot_kobj_unregister);
+		mutex_unlock(&nilfs->ns_cptree_lock);
 
+		iput(root->ifile);
 		kfree(root);
 	}
 }
diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
index 4776a70f01..074644c64a 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -150,7 +150,7 @@ struct the_nilfs {
 
 	/* Checkpoint tree */
 	struct rb_root		ns_cptree;
-	spinlock_t		ns_cptree_lock;
+	struct mutex		ns_cptree_lock; /* Protects ns_cptree */
 
 	/* Dirty inode list */
 	struct list_head	ns_dirty_files;
-- 
2.43.0

Re: [PATCH v2] nilfs2: fix checkpoint root lifetime on sysfs errors

[Viacheslav Dubeyko]

On Tue, 2026-09-15 at 16:49 -0300, Aldo Ariel Panzardo wrote:
> nilfs_find_or_create_root() publishes a new root in the checkpoint
> tree before creating its sysfs object.  If sysfs registration fails,
> the root is freed while it is still reachable from the tree.  A
> concurrent nilfs_lookup_root() can then dereference freed memory.
> 
> The fix needs the lock to be held across the sysfs call, but
> nilfs_sysfs_create_snapshot_group() can sleep, so the existing
> spinlock is not suitable.
> 
> Convert ns_cptree_lock from a spinlock to a mutex.  All existing
> callers are in process context (mount, lookup, segctor, recovery),
> and nilfs_put_root() can use refcount_dec_and_mutex_lock() as the
> atomic decrement-and-acquire primitive.
> 
> With the mutex, nilfs_find_or_create_root() can hold it across the
> sysfs registration and only insert the root into the rbtree after
> sysfs succeeds.  On failure, the root was never visible and can be
> freed after waiting for the kobject release callback to complete.
> 
> Both the creation error path and the normal removal path must wait
> for the embedded kobject release via wait_for_completion() before
> freeing the container, because kobject_put() does not guarantee
> synchronous release (CONFIG_DEBUG_KOBJECT_RELEASE defers it).
> 
> Fixes: dd70edbde262 ("nilfs2: integrate sysfs support into driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
> 
> Changes in v2:
>   - Convert ns_cptree_lock from spinlock to mutex instead of adding
>     a second lock, as suggested by Viacheslav Dubeyko.  Use
>     refcount_dec_and_mutex_lock() in nilfs_put_root().
> 
>  fs/nilfs2/the_nilfs.c | 35 +++++++++++++++++++----------------
>  fs/nilfs2/the_nilfs.h |  2 +-
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index 7b23e373a1..5bfa853471 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -70,7 +70,7 @@ struct the_nilfs *alloc_nilfs(struct super_block
> *sb)
>  	spin_lock_init(&nilfs->ns_inode_lock);
>  	spin_lock_init(&nilfs->ns_last_segment_lock);
>  	nilfs->ns_cptree = RB_ROOT;
> -	spin_lock_init(&nilfs->ns_cptree_lock);
> +	mutex_init(&nilfs->ns_cptree_lock);
>  	init_rwsem(&nilfs->ns_segctor_sem);
>  	nilfs->ns_sb_update_freq = NILFS_SB_FREQ;
>  
> @@ -846,7 +846,7 @@ struct nilfs_root *nilfs_lookup_root(struct
> the_nilfs *nilfs, __u64 cno)
>  	struct rb_node *n;
>  	struct nilfs_root *root;
>  
> -	spin_lock(&nilfs->ns_cptree_lock);
> +	mutex_lock(&nilfs->ns_cptree_lock);
>  	n = nilfs->ns_cptree.rb_node;
>  	while (n) {
>  		root = rb_entry(n, struct nilfs_root, rb_node);
> @@ -857,11 +857,11 @@ struct nilfs_root *nilfs_lookup_root(struct
> the_nilfs *nilfs, __u64 cno)
>  			n = n->rb_right;
>  		} else {
>  			refcount_inc(&root->count);
> -			spin_unlock(&nilfs->ns_cptree_lock);
> +			mutex_unlock(&nilfs->ns_cptree_lock);
>  			return root;
>  		}
>  	}
> -	spin_unlock(&nilfs->ns_cptree_lock);
> +	mutex_unlock(&nilfs->ns_cptree_lock);
>  
>  	return NULL;
>  }
> @@ -881,7 +881,7 @@ nilfs_find_or_create_root(struct the_nilfs
> *nilfs, __u64 cno)
>  	if (!new)
>  		return NULL;
>  
> -	spin_lock(&nilfs->ns_cptree_lock);
> +	mutex_lock(&nilfs->ns_cptree_lock);
>  
>  	p = &nilfs->ns_cptree.rb_node;
>  	parent = NULL;
> @@ -896,7 +896,7 @@ nilfs_find_or_create_root(struct the_nilfs
> *nilfs, __u64 cno)
>  			p = &(*p)->rb_right;
>  		} else {
>  			refcount_inc(&root->count);
> -			spin_unlock(&nilfs->ns_cptree_lock);
> +			mutex_unlock(&nilfs->ns_cptree_lock);
>  			kfree(new);
>  			return root;
>  		}
> @@ -909,17 +909,19 @@ nilfs_find_or_create_root(struct the_nilfs
> *nilfs, __u64 cno)
>  	atomic64_set(&new->inodes_count, 0);
>  	atomic64_set(&new->blocks_count, 0);
>  
> -	rb_link_node(&new->rb_node, parent, p);
> -	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
> -
> -	spin_unlock(&nilfs->ns_cptree_lock);
> -
>  	err = nilfs_sysfs_create_snapshot_group(new);
>  	if (err) {
> +		mutex_unlock(&nilfs->ns_cptree_lock);
> +		wait_for_completion(&new->snapshot_kobj_unregister);
>  		kfree(new);
> -		new = NULL;
> +		return NULL;
>  	}
>  
> +	rb_link_node(&new->rb_node, parent, p);
> +	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
> +
> +	mutex_unlock(&nilfs->ns_cptree_lock);
> +
>  	return new;
>  }
>  
> @@ -927,13 +929,14 @@ void nilfs_put_root(struct nilfs_root *root)
>  {
>  	struct the_nilfs *nilfs = root->nilfs;
>  
> -	if (refcount_dec_and_lock(&root->count, &nilfs-
> >ns_cptree_lock)) {
> +	if (refcount_dec_and_mutex_lock(&root->count,
> +					&nilfs->ns_cptree_lock)) {
>  		rb_erase(&root->rb_node, &nilfs->ns_cptree);
> -		spin_unlock(&nilfs->ns_cptree_lock);
> -
>  		nilfs_sysfs_delete_snapshot_group(root);
> -		iput(root->ifile);
> +		wait_for_completion(&root-
> >snapshot_kobj_unregister);

Is it right behavior that we are waiting completion under the lock?
Could we have potential deadlock here? I am feeling slightly bad about
waiting something under the lock.

> +		mutex_unlock(&nilfs->ns_cptree_lock);
>  
> +		iput(root->ifile);
>  		kfree(root);
>  	}
>  }
> diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
> index 4776a70f01..074644c64a 100644
> --- a/fs/nilfs2/the_nilfs.h
> +++ b/fs/nilfs2/the_nilfs.h
> @@ -150,7 +150,7 @@ struct the_nilfs {
>  
>  	/* Checkpoint tree */
>  	struct rb_root		ns_cptree;
> -	spinlock_t		ns_cptree_lock;
> +	struct mutex		ns_cptree_lock; /* Protects
> ns_cptree */

I assume that you prefer mutex because of exclusive lock nature. Could
we improve something for the case of rw_semaphore?

Thanks,
Slava.

>  
>  	/* Dirty inode list */
>  	struct list_head	ns_dirty_files;

Re: [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors

[Aldo Ariel Panzardo]

Hi Slava,

Good point about the wait_for_completion under the lock.  There is no
circular dependency (the release callback only calls complete() without
taking ns_cptree_lock), but holding the mutex during an unbounded wait
is unnecessary.  In v3 I move wait_for_completion() and kfree() after
the mutex_unlock() in nilfs_put_root().  By that point rb_erase() has
already removed the root from the tree and the refcount is zero, so no
concurrent lookup can find it and no other put can race.

Regarding rwsem: the kernel does not provide a combined
refcount_dec_and_rwsem_lock() helper, so nilfs_put_root() would need
the two-stage refcount_dec_not_one() / down_write() /
refcount_dec_and_test() pattern.  That is doable, but the lookup
contention on ns_cptree is minimal (mount, segctor, recovery), so the
added complexity does not seem justified here.

v3 follows.

Aldo

[PATCH v3] nilfs2: fix checkpoint root lifetime on sysfs errors

[Aldo Ariel Panzardo]

nilfs_find_or_create_root() publishes a new root in the checkpoint
tree before creating its sysfs object.  If sysfs registration fails,
the root is freed while it is still reachable from the tree.  A
concurrent nilfs_lookup_root() can then dereference freed memory.

The fix needs the lock to be held across the sysfs call, but
nilfs_sysfs_create_snapshot_group() can sleep, so the existing
spinlock is not suitable.

Convert ns_cptree_lock from a spinlock to a mutex.  All existing
callers are in process context (mount, lookup, segctor, recovery),
and nilfs_put_root() can use refcount_dec_and_mutex_lock() as the
atomic decrement-and-acquire primitive.

With the mutex, nilfs_find_or_create_root() can hold it across the
sysfs registration and only insert the root into the rbtree after
sysfs succeeds.  On failure, the root was never visible and can be
freed after waiting for the kobject release callback to complete.

Both the creation error path and the normal removal path must wait
for the embedded kobject release via wait_for_completion() before
freeing the container, because kobject_put() does not guarantee
synchronous release (CONFIG_DEBUG_KOBJECT_RELEASE defers it).

Fixes: dd70edbde262 ("nilfs2: integrate sysfs support into driver")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---

Changes in v3:
  - Move wait_for_completion() after mutex_unlock() in
    nilfs_put_root() to avoid holding the lock during an
    unbounded wait, as noted by Viacheslav Dubeyko.
    After rb_erase() the root is no longer in the tree
    and the refcount is zero, so no concurrent path can
    reach it.

Changes in v2:
  - Convert ns_cptree_lock from spinlock to mutex instead of adding
    a second lock, as suggested by Viacheslav Dubeyko.  Use
    refcount_dec_and_mutex_lock() in nilfs_put_root().

 fs/nilfs2/the_nilfs.c | 37 ++++++++++++++++++++-----------------
 fs/nilfs2/the_nilfs.h |  2 +-
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 7b23e373a1..XXXXXXX 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -70,7 +70,7 @@ struct the_nilfs *alloc_nilfs(struct super_block *sb)
 	spin_lock_init(&nilfs->ns_inode_lock);
 	spin_lock_init(&nilfs->ns_last_segment_lock);
 	nilfs->ns_cptree = RB_ROOT;
-	spin_lock_init(&nilfs->ns_cptree_lock);
+	mutex_init(&nilfs->ns_cptree_lock);
 	init_rwsem(&nilfs->ns_segctor_sem);
 	nilfs->ns_sb_update_freq = NILFS_SB_FREQ;

@@ -846,7 +846,7 @@ struct nilfs_root *nilfs_lookup_root(struct the_nilfs *nilfs, __u64 cno)
 	struct rb_node *n;
 	struct nilfs_root *root;

-	spin_lock(&nilfs->ns_cptree_lock);
+	mutex_lock(&nilfs->ns_cptree_lock);
 	n = nilfs->ns_cptree.rb_node;
 	while (n) {
 		root = rb_entry(n, struct nilfs_root, rb_node);
@@ -857,11 +857,11 @@ struct nilfs_root *nilfs_lookup_root(struct the_nilfs *nilfs, __u64 cno)
 			n = n->rb_right;
 		} else {
 			refcount_inc(&root->count);
-			spin_unlock(&nilfs->ns_cptree_lock);
+			mutex_unlock(&nilfs->ns_cptree_lock);
 			return root;
 		}
 	}
-	spin_unlock(&nilfs->ns_cptree_lock);
+	mutex_unlock(&nilfs->ns_cptree_lock);

 	return NULL;
 }
@@ -881,7 +881,7 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno)
 	if (!new)
 		return NULL;

-	spin_lock(&nilfs->ns_cptree_lock);
+	mutex_lock(&nilfs->ns_cptree_lock);

 	p = &nilfs->ns_cptree.rb_node;
 	parent = NULL;
@@ -896,7 +896,7 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno)
 			p = &(*p)->rb_right;
 		} else {
 			refcount_inc(&root->count);
-			spin_unlock(&nilfs->ns_cptree_lock);
+			mutex_unlock(&nilfs->ns_cptree_lock);
 			kfree(new);
 			return root;
 		}
@@ -909,17 +909,19 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno)
 	atomic64_set(&new->inodes_count, 0);
 	atomic64_set(&new->blocks_count, 0);

-	rb_link_node(&new->rb_node, parent, p);
-	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
-
-	spin_unlock(&nilfs->ns_cptree_lock);
-
 	err = nilfs_sysfs_create_snapshot_group(new);
 	if (err) {
+		mutex_unlock(&nilfs->ns_cptree_lock);
+		wait_for_completion(&new->snapshot_kobj_unregister);
 		kfree(new);
-		new = NULL;
+		return NULL;
 	}

+	rb_link_node(&new->rb_node, parent, p);
+	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
+
+	mutex_unlock(&nilfs->ns_cptree_lock);
+
 	return new;
 }

@@ -927,14 +929,15 @@ void nilfs_put_root(struct nilfs_root *root)
 {
 	struct the_nilfs *nilfs = root->nilfs;

-	if (refcount_dec_and_lock(&root->count, &nilfs->ns_cptree_lock)) {
+	if (refcount_dec_and_mutex_lock(&root->count,
+					&nilfs->ns_cptree_lock)) {
 		rb_erase(&root->rb_node, &nilfs->ns_cptree);
-		spin_unlock(&nilfs->ns_cptree_lock);
-
 		nilfs_sysfs_delete_snapshot_group(root);
-		iput(root->ifile);
+		mutex_unlock(&nilfs->ns_cptree_lock);

+		wait_for_completion(&root->snapshot_kobj_unregister);
+		iput(root->ifile);
 		kfree(root);
 	}
 }
diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
index 4776a70f01..074644c64a 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -150,7 +150,7 @@ struct the_nilfs {

 	/* Checkpoint tree */
 	struct rb_root		ns_cptree;
-	spinlock_t		ns_cptree_lock;
+	struct mutex		ns_cptree_lock; /* Protects ns_cptree */

 	/* Dirty inode list */
 	struct list_head	ns_dirty_files;
--
2.43.0

Re: [PATCH v3] nilfs2: fix checkpoint root lifetime on sysfs errors

[Viacheslav Dubeyko]

On Wed, 2026-09-16 at 20:50 -0300, Aldo Ariel Panzardo wrote:
> nilfs_find_or_create_root() publishes a new root in the checkpoint
> tree before creating its sysfs object.  If sysfs registration fails,
> the root is freed while it is still reachable from the tree.  A
> concurrent nilfs_lookup_root() can then dereference freed memory.
> 
> The fix needs the lock to be held across the sysfs call, but
> nilfs_sysfs_create_snapshot_group() can sleep, so the existing
> spinlock is not suitable.
> 
> Convert ns_cptree_lock from a spinlock to a mutex.  All existing
> callers are in process context (mount, lookup, segctor, recovery),
> and nilfs_put_root() can use refcount_dec_and_mutex_lock() as the
> atomic decrement-and-acquire primitive.
> 
> With the mutex, nilfs_find_or_create_root() can hold it across the
> sysfs registration and only insert the root into the rbtree after
> sysfs succeeds.  On failure, the root was never visible and can be
> freed after waiting for the kobject release callback to complete.
> 
> Both the creation error path and the normal removal path must wait
> for the embedded kobject release via wait_for_completion() before
> freeing the container, because kobject_put() does not guarantee
> synchronous release (CONFIG_DEBUG_KOBJECT_RELEASE defers it).
> 
> Fixes: dd70edbde262 ("nilfs2: integrate sysfs support into driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
> 
> Changes in v3:
>   - Move wait_for_completion() after mutex_unlock() in
>     nilfs_put_root() to avoid holding the lock during an
>     unbounded wait, as noted by Viacheslav Dubeyko.
>     After rb_erase() the root is no longer in the tree
>     and the refcount is zero, so no concurrent path can
>     reach it.
> 
> Changes in v2:
>   - Convert ns_cptree_lock from spinlock to mutex instead of adding
>     a second lock, as suggested by Viacheslav Dubeyko.  Use
>     refcount_dec_and_mutex_lock() in nilfs_put_root().
> 
>  fs/nilfs2/the_nilfs.c | 37 ++++++++++++++++++++-----------------
>  fs/nilfs2/the_nilfs.h |  2 +-
>  2 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index 7b23e373a1..XXXXXXX 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -70,7 +70,7 @@ struct the_nilfs *alloc_nilfs(struct super_block
> *sb)
>  	spin_lock_init(&nilfs->ns_inode_lock);
>  	spin_lock_init(&nilfs->ns_last_segment_lock);
>  	nilfs->ns_cptree = RB_ROOT;
> -	spin_lock_init(&nilfs->ns_cptree_lock);
> +	mutex_init(&nilfs->ns_cptree_lock);
>  	init_rwsem(&nilfs->ns_segctor_sem);
>  	nilfs->ns_sb_update_freq = NILFS_SB_FREQ;
> 
> @@ -846,7 +846,7 @@ struct nilfs_root *nilfs_lookup_root(struct
> the_nilfs *nilfs, __u64 cno)
>  	struct rb_node *n;
>  	struct nilfs_root *root;
> 
> -	spin_lock(&nilfs->ns_cptree_lock);
> +	mutex_lock(&nilfs->ns_cptree_lock);
>  	n = nilfs->ns_cptree.rb_node;
>  	while (n) {
>  		root = rb_entry(n, struct nilfs_root, rb_node);
> @@ -857,11 +857,11 @@ struct nilfs_root *nilfs_lookup_root(struct
> the_nilfs *nilfs, __u64 cno)
>  			n = n->rb_right;
>  		} else {
>  			refcount_inc(&root->count);
> -			spin_unlock(&nilfs->ns_cptree_lock);
> +			mutex_unlock(&nilfs->ns_cptree_lock);
>  			return root;
>  		}
>  	}
> -	spin_unlock(&nilfs->ns_cptree_lock);
> +	mutex_unlock(&nilfs->ns_cptree_lock);
> 
>  	return NULL;
>  }
> @@ -881,7 +881,7 @@ nilfs_find_or_create_root(struct the_nilfs
> *nilfs, __u64 cno)
>  	if (!new)
>  		return NULL;
> 
> -	spin_lock(&nilfs->ns_cptree_lock);
> +	mutex_lock(&nilfs->ns_cptree_lock);
> 
>  	p = &nilfs->ns_cptree.rb_node;
>  	parent = NULL;
> @@ -896,7 +896,7 @@ nilfs_find_or_create_root(struct the_nilfs
> *nilfs, __u64 cno)
>  			p = &(*p)->rb_right;
>  		} else {
>  			refcount_inc(&root->count);
> -			spin_unlock(&nilfs->ns_cptree_lock);
> +			mutex_unlock(&nilfs->ns_cptree_lock);
>  			kfree(new);
>  			return root;
>  		}
> @@ -909,17 +909,19 @@ nilfs_find_or_create_root(struct the_nilfs
> *nilfs, __u64 cno)
>  	atomic64_set(&new->inodes_count, 0);
>  	atomic64_set(&new->blocks_count, 0);
> 
> -	rb_link_node(&new->rb_node, parent, p);
> -	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
> -
> -	spin_unlock(&nilfs->ns_cptree_lock);
> -
>  	err = nilfs_sysfs_create_snapshot_group(new);
>  	if (err) {
> +		mutex_unlock(&nilfs->ns_cptree_lock);
> +		wait_for_completion(&new->snapshot_kobj_unregister);

Should we consider wait_for_completion_killable_timeout()? What do you
think? Do we need to use timeout?

>  		kfree(new);
> -		new = NULL;
> +		return NULL;
>  	}
> 
> +	rb_link_node(&new->rb_node, parent, p);
> +	rb_insert_color(&new->rb_node, &nilfs->ns_cptree);
> +
> +	mutex_unlock(&nilfs->ns_cptree_lock);
> +
>  	return new;
>  }
> 
> @@ -927,14 +929,15 @@ void nilfs_put_root(struct nilfs_root *root)
>  {
>  	struct the_nilfs *nilfs = root->nilfs;
> 
> -	if (refcount_dec_and_lock(&root->count, &nilfs-
> >ns_cptree_lock)) {
> +	if (refcount_dec_and_mutex_lock(&root->count,
> +					&nilfs->ns_cptree_lock)) {
>  		rb_erase(&root->rb_node, &nilfs->ns_cptree);
> -		spin_unlock(&nilfs->ns_cptree_lock);
> -
>  		nilfs_sysfs_delete_snapshot_group(root);
> -		iput(root->ifile);
> +		mutex_unlock(&nilfs->ns_cptree_lock);
> 
> +		wait_for_completion(&root-
> >snapshot_kobj_unregister);

Ditto.

Thanks,
Slava.

> +		iput(root->ifile);
>  		kfree(root);
>  	}
>  }
> diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
> index 4776a70f01..074644c64a 100644
> --- a/fs/nilfs2/the_nilfs.h
> +++ b/fs/nilfs2/the_nilfs.h
> @@ -150,7 +150,7 @@ struct the_nilfs {
> 
>  	/* Checkpoint tree */
>  	struct rb_root		ns_cptree;
> -	spinlock_t		ns_cptree_lock;
> +	struct mutex		ns_cptree_lock; /* Protects
> ns_cptree */
> 
>  	/* Dirty inode list */
>  	struct list_head	ns_dirty_files;
> --
> 2.43.0