Re: [PATCH v4] nilfs2: fix checkpoint root lifetime on sysfs errors
From: Viacheslav Dubeyko
Date: Tue Sep 22 2026 - 17:52:43 EST
On Tue, 2026-09-22 at 08:55 +0900, Ryusuke Konishi wrote:
> 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
> >
Applied.
Thanks,
Slava.