Re: [PATCH] fs: refuse to drop a dentry reference d_make_persistent() never took

[Christian Brauner]

> DCACHE_PERSISTENT records that d_make_persistent() took a reference on
> the dentry with dget_dlock(). d_make_discardable() announces that
> precondition but does not act on it:
> 
> 	spin_lock(&dentry->d_lock);
> 	WARN_ON(!(dentry->d_flags & DCACHE_PERSISTENT));
> 	dentry->d_flags &= ~DCACHE_PERSISTENT;
> 	dentry->d_lockref.count--;
> 	finish_dput(dentry);
> 
> so a caller that never made the dentry persistent still has a reference
> taken from it.  finish_dput() runs dentry_kill(), so the result is not a
> stale flag but a d_lockref underflow that can free a dentry another
> holder still references.
> 
> The same file already enforces the invariant rather than announcing it,
> in select_collect_umount():
> 
> 	if (dentry->d_flags & DCACHE_PERSISTENT) {
> 		dentry->d_flags &= ~DCACHE_PERSISTENT;
> 		dentry->d_lockref.count--;
> 	}
> 
> All seven in-tree callers are correctly paired (fs/libfs.c, fs/devpts,
> fs/autofs, fs/tracefs).  Both d_make_persistent() and
> d_make_discardable() are EXPORT_SYMBOL, so the contract is module-facing
> and currently unenforced.
> 
> Make the check act.  On a mismatch the failure mode becomes a leaked
> pinned dentry with a warning naming the caller, instead of a freed live
> one.  The early return unlocks explicitly, since finish_dput() - which
> normally releases d_lock - is no longer reached on that path.
> 
> Demonstrated with a late_initcall calling d_make_discardable() on a
> d_alloc_name() dentry, which by construction lacks DCACHE_PERSISTENT.
> Same kernel, only fs/dcache.c differs:
> 
> 	unpatched:  count before=2 after=1	(reference dropped)
> 	patched:    count before=2 after=2	(refcount untouched)
> 
> Both kernels emit the warning; only the patched one declines to act on
> it.
> 
> Fixes: bacdf1d70bbe ("primitives for maintaining persisitency")
> Signed-off-by: Narek Jilavyan <njilav@xxxxxxxxx>

I struggle to the see the point in this change.

--