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@xxxxxxxxxxxxxxx
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@xxxxxxxxx>
> ---
>  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;