* Re: [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors
2026-09-15 4:12 [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors Aldo Ariel Panzardo
@ 2026-09-15 18:49 ` Viacheslav Dubeyko
2026-09-15 19:44 ` Aldo Ariel Panzardo
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Viacheslav Dubeyko @ 2026-09-15 18:49 UTC (permalink / raw)
To: Aldo Ariel Panzardo, Ryusuke Konishi; +Cc: linux-nilfs, linux-kernel, stable
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;
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors
2026-09-15 4:12 [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors Aldo Ariel Panzardo
2026-09-15 18:49 ` Viacheslav Dubeyko
@ 2026-09-15 19:44 ` Aldo Ariel Panzardo
2026-09-15 19:49 ` [PATCH v2] " Aldo Ariel Panzardo
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Aldo Ariel Panzardo @ 2026-09-15 19:44 UTC (permalink / raw)
To: Viacheslav Dubeyko, Ryusuke Konishi
Cc: linux-nilfs, linux-kernel, stable, 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2] nilfs2: fix checkpoint root lifetime on sysfs errors
2026-09-15 4:12 [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors Aldo Ariel Panzardo
2026-09-15 18:49 ` Viacheslav Dubeyko
2026-09-15 19:44 ` Aldo Ariel Panzardo
@ 2026-09-15 19:49 ` Aldo Ariel Panzardo
2026-09-16 18:58 ` Viacheslav Dubeyko
2026-09-16 23:45 ` [PATCH] " Aldo Ariel Panzardo
2026-09-16 23:50 ` [PATCH v3] " Aldo Ariel Panzardo
4 siblings, 1 reply; 7+ messages in thread
From: Aldo Ariel Panzardo @ 2026-09-15 19:49 UTC (permalink / raw)
To: Ryusuke Konishi
Cc: Viacheslav Dubeyko, linux-nilfs, linux-kernel, stable,
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2] nilfs2: fix checkpoint root lifetime on sysfs errors
2026-09-15 19:49 ` [PATCH v2] " Aldo Ariel Panzardo
@ 2026-09-16 18:58 ` Viacheslav Dubeyko
0 siblings, 0 replies; 7+ messages in thread
From: Viacheslav Dubeyko @ 2026-09-16 18:58 UTC (permalink / raw)
To: Aldo Ariel Panzardo, Ryusuke Konishi; +Cc: linux-nilfs, linux-kernel, stable
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors
2026-09-15 4:12 [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors Aldo Ariel Panzardo
` (2 preceding siblings ...)
2026-09-15 19:49 ` [PATCH v2] " Aldo Ariel Panzardo
@ 2026-09-16 23:45 ` Aldo Ariel Panzardo
2026-09-16 23:50 ` [PATCH v3] " Aldo Ariel Panzardo
4 siblings, 0 replies; 7+ messages in thread
From: Aldo Ariel Panzardo @ 2026-09-16 23:45 UTC (permalink / raw)
To: Viacheslav Dubeyko, Ryusuke Konishi
Cc: linux-nilfs, linux-kernel, stable, 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v3] nilfs2: fix checkpoint root lifetime on sysfs errors
2026-09-15 4:12 [PATCH] nilfs2: fix checkpoint root lifetime on sysfs errors Aldo Ariel Panzardo
` (3 preceding siblings ...)
2026-09-16 23:45 ` [PATCH] " Aldo Ariel Panzardo
@ 2026-09-16 23:50 ` Aldo Ariel Panzardo
4 siblings, 0 replies; 7+ messages in thread
From: Aldo Ariel Panzardo @ 2026-09-16 23:50 UTC (permalink / raw)
To: Ryusuke Konishi
Cc: Viacheslav Dubeyko, linux-nilfs, linux-kernel, stable,
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
^ permalink raw reply [flat|nested] 7+ messages in thread