Re: [PATCH 1/2] Re introduces a DCACHE_DENTRY_UNHASHED flag

[Al Viro <viro@ZenIV.linux.org.uk>]

On Thu, Dec 01, 2016 at 07:18:26AM +0300, Alexey Lyashkov wrote:
> rehash process protected with d_seq and d_lock locks, but VFS have access to the
> d_hashed field without any locks sometimes. It produce errors with get cwd
> operations or fsnotify may report an unlink event sometimes. d_seq lock isn’t
> used to protect due possibility to sleep with holding locks, and d_lock is too
> hot to check a simple state, lets return a DCACHE_DENTRY_UNHASHED flag to
> ability to check a unhashed state without locks held.

>   *	Returns true if the dentry passed is not currently hashed.
> + *	hlist_bl_unhashed can't be used as race with d_move().
>   */
>   
>  static inline int d_unhashed(const struct dentry *dentry)
>  {
> -	return hlist_bl_unhashed(&dentry->d_hash);
> +	return dentry->d_flags & DCACHE_DENTRY_UNHASHED;
>  }

The real problem here is the unsafe use of d_unlinked().  You are papering
over that, and doing so in the wrong place.  Note that most of the callers
of d_unhashed() are either under ->d_lock on it or on parent or with
parent locked by ->i_rwsem, or in places where false positive is fine
since it only pushes us to slow path.  Excluding those, we are left with

fs/autofs4/expire.c:226:        if (!simple_positive(top))
fs/autofs4/expire.c:537:                if (d_unhashed(dentry))
fs/ceph/dir.c:649:              BUG_ON(!d_unhashed(dentry));
fs/ceph/inode.c:1074:   if (!d_unhashed(dn))
fs/ceph/inode.c:1322:                           if (have_lease && d_unhashed(dn))
fs/cifs/inode.c:945:            if (!d_unhashed(dentry) || IS_ROOT(dentry)) {
fs/coredump.c:727:              if (d_unhashed(cprm.file->f_path.dentry))
fs/dcache.c:3202:       if (d_unlinked(path->dentry)) {
fs/dcache.c:3366:       if (d_unlinked(dentry)) {
fs/dcache.c:3423:       if (!d_unlinked(pwd.dentry)) {
fs/debugfs/file.c:77:   if (d_unlinked(dentry))
fs/namespace.c:3154:    if (d_unlinked(new.dentry))
fs/namespace.c:738:                     if (d_unlinked(dentry))
fs/notify/inotify/inotify_fsnotify.c:85:                if (d_unlinked(path->dentry))
security/apparmor/file.c:263:   if (d_unlinked(dentry) && d_backing_inode(dentry)->i_nlink == 0)
security/apparmor/path.c:149:   if (d_unlinked(path->dentry) && d_is_positive(path->dentry) &&

VFS part of that consists of
	* d_path() and friends, including getcwd().  Might bloody well turn
those into
	if (unlikely(d_unhashed(dentry))) {
		/* recheck under d_lock */
		spin_lock(&dentry->d_lock);
		if (d_unhashed(dentry))
			...
	* mount(2)/pivot_root(2) vs. rename(2) - userland race; after all,
had you called it just a bit later, you *would* have gotten that -ENOENT.
Nothing the kernel can actually do here.
	* do_coredump() bailing out on races with somebody moving the
coredump to be around.  Cry me a river...
	* idiotify playing with d_unlinked(); same story as with
d_path() et.al.

autofs ones are, AFAICS, harmless - you might spin a bit more (if that), but
no worse than that.  If they can be triggered at all, that is.
No idea about the ceph ones (i.e. whether they can race with d_move()
like that, whether it really cares, etc.).  debugfs looks like it can't race
with d_move() at all.  cifs... no idea, really.  Looks like we _might_
have an odd behaviour from server not spotted in some cases we would've
spotted it otherwise.  Not sure if we care.  Apparmor... you'll need to
ask apparmor folks.

NAK in that form.