Re: [PATCH] Re: Possible FS race condition between iterate_dir and d_alloc_parallel

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

[btrfs and cifs folks Cc'd]

On Sat, Sep 21, 2019 at 03:07:31PM +0100, Al Viro wrote:

> No "take cursors out of the list" parts yet.

Argh...  The things turned interesting.  The tricky part is
where do we handle switching cursors away from something
that gets moved.

What I hoped for was "just do it in simple_rename()".  Which is
almost OK; there are 3 problematic cases.  One is shmem -
there we have a special ->rename(), which handles things
like RENAME_EXCHANGE et.al.  Fair enough - some of that
might've been moved into simple_rename(), but some (whiteouts)
won't be that easy.  Fair enough - we can make kicking the
cursors outs a helper called by simple_rename() and by that.
Exchange case is going to cause a bit of headache (the
pathological case is when the entries being exchanged are
next to each other in the same directory), but it's not
that bad.

Two other cases, though, might be serious trouble.  Those are
btrfs new_simple_dir() and this in cifs_root_iget():
        if (rc && tcon->pipe) {
                cifs_dbg(FYI, "ipc connection - fake read inode\n");
                spin_lock(&inode->i_lock);
                inode->i_mode |= S_IFDIR;
                set_nlink(inode, 2);
                inode->i_op = &cifs_ipc_inode_ops;
                inode->i_fop = &simple_dir_operations;
                inode->i_uid = cifs_sb->mnt_uid;
                inode->i_gid = cifs_sb->mnt_gid;
                spin_unlock(&inode->i_lock);
	}
The trouble is, it looks like d_splice_alias() from a lookup elsewhere
might find an alias of some subdirectory in those.  And in that case
we'll end up with a child of those (dcache_readdir-using) directories
being ripped out and moved elsewhere.  With no calls of ->rename() in
sight, of course, *AND* with only shared lock on the parent.  The
last part is really nasty.  And not just for hanging cursors off the
dentries they point to - it's a problem for dcache_readdir() itself
even in the mainline and with all the lockless crap reverted.

We pass next->d_name.name to dir_emit() (i.e. potentially to
copy_to_user()).  And we have no warranty that it's not a long
(== separately allocated) name, that will be freed while
copy_to_user() is in progress.  Sure, it'll get an RCU delay
before freeing, but that doesn't help us at all.

I'm not familiar with those areas in btrfs or cifs; could somebody
explain what's going on there and can we indeed end up finding aliases
to those suckers?