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

[Ryusuke Konishi]

On Mon, Sep 21, 2026 at 4:47 PM Ryusuke Konishi wrote:
>
> On Sat, Sep 19, 2026 at 6:22 AM Aldo Ariel Panzardo wrote:
> >
> > Hi Slava,
> >
> > No, wait_for_completion() without a timeout is intentional here.
> >
> > The kobject subsystem guarantees that the release callback will
> > eventually run -- CONFIG_DEBUG_KOBJECT_RELEASE only defers it, it
> > never drops it. So the wait is bounded in practice.
> >
> > If we used wait_for_completion_killable_timeout() and the timeout
> > fired (or a signal arrived) before the release callback ran, we
> > would kfree() the root while the kobject release is still pending.
> > When the callback finally runs it would access freed memory -- a
> > use-after-free.
> >
> > wait_for_completion_killable() (without timeout) has the same
> > problem: if the wait is interrupted by a fatal signal, we cannot
> > safely free the container because the release callback may still
> > reference it.
> >
> > The unconditional wait_for_completion() is the only safe choice
> > when the caller must free the container of an embedded kobject.
> > This is the same pattern used by other subsystems (e.g.,
> > blk_mq_tag_set, configfs subsystems).
> >
> > Aldo
>
> Hi Aldo,
>
> Thank you for the patch.
>
> However, in my view, introducing a mutex for synchronization (either
> by replacing the spinlock or adding a mutex alongside it) is
> unnecessary and suboptimal.
>
> nilfs_sysfs_create_snapshot_group() performs memory allocations and
> sysfs node creation, which can sleep.  Holding 'ns_cptree_lock' across
> this operation causes concurrent read-only callers like
> nilfs_lookup_root() to block unnecessarily on a lock that originally
> protected a very fast, in-memory rbtree lookup.
>
> Note that checkpoint and snapshot mounts themselves are serialized by
> 'nilfs->ns_snapshot_mount_mutex', so strict mutual exclusion between
> concurrent insertions is not required; we only need to eliminate the
> race against nilfs_lookup_root().
>
> Instead of converting 'ns_cptree_lock' to a mutex, I think it would be
> cleaner to keep 'ns_cptree_lock' as a spinlock and structure the
> creation flow so that new is fully initialized and registered with
> sysfs *before* acquiring the spinlock:
>
> 1. Allocate new and initialize the basic fields required for sysfs
> creation (cno, nilfs, etc.).
> 2. Call nilfs_sysfs_create_snapshot_group(new) *before* taking the lock.
>     - On error, wait_for_completion(&new->snapshot_kobj_unregister),
>       kfree(new), and return NULL.
> 3. If sysfs creation succeeds, acquire spin_lock(&nilfs->ns_cptree_lock),
>    link new into ns_cptree, and drop the spinlock.
>
> In nilfs_put_root(), we can keep refcount_dec_and_lock() with the
> spinlock, erase the node from the tree, drop the spinlock, delete the
> sysfs group, wait for completion, and free root.
>
> Could you please update the patch to keep 'ns_cptree_lock' as a
> spinlock and complete sysfs registration before inserting the root
> into ns_cptree?
>
> I think this approach should work, but please let me know if I've
> missed anything.
>
> Thanks,
> Ryusuke Konishi

Hi Aldo,

On second thought, regarding
wait_for_completion(&root->snapshot_kobj_unregister),
wouldn't it be even cleaner to keep it inside the sysfs helper
functions instead of exposing it in the cptree functions?

Specifically:

- Inside nilfs_sysfs_create_snapshot_group():
  Call wait_for_completion(&root->snapshot_kobj_unregister) directly in its
  error path after kobject_put().

- Inside nilfs_sysfs_delete_snapshot_group():
  Call wait_for_completion(&root->snapshot_kobj_unregister) after kobject_put().

This way, the completion logic is nicely encapsulated within the sysfs
subsystem, so nilfs_find_or_create_root() and nilfs_put_root() don't
need to manage the internal completion state explicitly they can
simply call the sysfs functions and perform kfree() when needed.

What do you think?

Thanks,
Ryusuke Konishi