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

[Ryusuke Konishi]

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