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

[Ryusuke Konishi]

On Tue, Sep 22, 2026 at 12:22 AM Aldo Ariel Panzardo wrote:
>
> nilfs_find_or_create_root() links a new checkpoint root into the
> checkpoint tree and drops ns_cptree_lock before creating the root's
> sysfs group.  If nilfs_sysfs_create_snapshot_group() then fails, the
> root is freed while it is still reachable from ns_cptree, so a
> concurrent nilfs_lookup_root() can dereference freed memory.
>
> nilfs_sysfs_create_snapshot_group() allocates memory and creates sysfs
> nodes and may sleep, so it cannot run under the ns_cptree_lock spinlock.
> Reorder the creation path so that the sysfs group is set up before the
> root is published: initialize the root, create its sysfs group outside
> the lock, and only then take ns_cptree_lock to link it into the tree.
> On sysfs failure the root was never visible and is freed directly; in
> the unlikely case a root with the same checkpoint number is already
> present, the sysfs group of the new root is removed before it is freed.
> Concurrent insertions are already serialized by ns_snapshot_mount_mutex,
> so only the race against nilfs_lookup_root() needs to be closed.
>
> Because kobject_put() does not guarantee that the embedded kobject's
> release callback has run by the time it returns
> (CONFIG_DEBUG_KOBJECT_RELEASE defers it), the container must not be
> freed until the release completes.  Keep that wait_for_completion() of
> snapshot_kobj_unregister inside the sysfs helpers, so both the creation
> error path and nilfs_sysfs_delete_snapshot_group() wait for the release
> before their callers free the root.
>
> Fixes: dd70edbde262 ("nilfs2: integrate sysfs support into driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@xxxxxxxxx>
> ---
> Changes in v4:
>   - Keep ns_cptree_lock as a spinlock instead of converting it to a
>     mutex, and create the sysfs group before taking the lock, as
>     suggested by Ryusuke Konishi. Concurrent insertions are already
>     serialized by ns_snapshot_mount_mutex; only the race against
>     nilfs_lookup_root() needs to be closed.
>   - Move the wait_for_completion() of snapshot_kobj_unregister into the
>     sysfs helpers (create error path and delete), so the checkpoint
>     tree functions no longer manage the kobject completion state, as
>     suggested by Ryusuke Konishi.
>
> Changes in v3:
>   - Move wait_for_completion() out of the locked section in
>     nilfs_put_root() (the mutex-based v2/v3 approach; dropped in v4).
>
> Changes in v2:
>   - Serialize the sysfs registration against the tree instead of adding
>     a second lock, as suggested by Viacheslav Dubeyko.

Acked-by: Ryusuke Konishi <konishi.ryusuke@xxxxxxxxx>

Looks good to me.

Viacheslav, could you please apply this v4 patch directly?
(Please keep the 'Cc: stable' tag this time, since this fixes a
Use-After-Free bug.)

One note regarding this change: if a sysfs directory corresponding to
the checkpoint already exists, nilfs_sysfs_create_snapshot_group()
will not only return an error, but kobject_init_and_add() will also
output an
error message.

However, nilfs_find_or_create_root() checks for an existing entry at
the beginning using nilfs_lookup_root(), and
nilfs_attach_checkpoint(), which calls this function, is always
invoked exclusively — either under 'ns_snapshot_mount_mutex' or during
the serialized initial mount path.
Therefore, this side effect is avoided.

Thanks,
Ryusuke Konishi

>
>  fs/nilfs2/sysfs.c     |  5 ++++-
>  fs/nilfs2/the_nilfs.c | 34 +++++++++++++++++++++-------------
>  2 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/fs/nilfs2/sysfs.c b/fs/nilfs2/sysfs.c
> index bc52afbfc5c7..5bb74a45ad1e 100644
> --- a/fs/nilfs2/sysfs.c
> +++ b/fs/nilfs2/sysfs.c
> @@ -195,8 +195,10 @@ int nilfs_sysfs_create_snapshot_group(struct nilfs_root *root)
>                                             "%llu", root->cno);
>         }
>
> -       if (err)
> +       if (err) {
>                 kobject_put(&root->snapshot_kobj);
> +               wait_for_completion(&root->snapshot_kobj_unregister);
> +       }
>
>         return err;
>  }
> @@ -204,6 +206,7 @@ int nilfs_sysfs_create_snapshot_group(struct nilfs_root *root)
>  void nilfs_sysfs_delete_snapshot_group(struct nilfs_root *root)
>  {
>         kobject_put(&root->snapshot_kobj);
> +       wait_for_completion(&root->snapshot_kobj_unregister);
>  }
>
>  /************************************************************************
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index ecd71c190885..1d5d2293c9c7 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -869,6 +869,26 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno)
>         if (!new)
>                 return NULL;
>
> +       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);
> +
> +       /*
> +        * Register the sysfs group before publishing the root in the
> +        * checkpoint tree.  nilfs_sysfs_create_snapshot_group() can sleep,
> +        * so it must run outside ns_cptree_lock; creating it first also
> +        * ensures a concurrent nilfs_lookup_root() can never observe a root
> +        * whose sysfs registration later fails and gets freed.
> +        */
> +       err = nilfs_sysfs_create_snapshot_group(new);
> +       if (err) {
> +               kfree(new);
> +               return NULL;
> +       }
> +
>         spin_lock(&nilfs->ns_cptree_lock);
>
>         p = &nilfs->ns_cptree.rb_node;
> @@ -885,29 +905,17 @@ nilfs_find_or_create_root(struct the_nilfs *nilfs, __u64 cno)
>                 } else {
>                         refcount_inc(&root->count);
>                         spin_unlock(&nilfs->ns_cptree_lock);
> +                       nilfs_sysfs_delete_snapshot_group(new);
>                         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) {
> -               kfree(new);
> -               new = NULL;
> -       }
> -
>         return new;
>  }
>
> --
> 2.43.0
>