Re: [UNIONFS] 00/29 Unionfs and related patches pre-merge review (v2)

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

After grep for locking-related things:

	* lock_parent(): who said that you won't get dentry moved
before managing to grab i_mutex on parent?  While we are at it,
who said that you won't get dentry moved between fetching d_parent
and doing dget()?  In that case parent could've been _freed_ before
you get to dget().

	* in create_parents():
+                       struct inode *inode = lower_dentry->d_inode;
+                       /*
+                        * If we get here, it means that we created a new
+                        * dentry+inode, but copying permissions failed.
+                        * Therefore, we should delete this inode and dput
+                        * the dentry so as not to leave cruft behind.
+                        */
+                       if (lower_dentry->d_op && lower_dentry->d_op->d_iput)
+                               lower_dentry->d_op->d_iput(lower_dentry,
+                                                          inode);
+                       else
+                               iput(inode);
+                       lower_dentry->d_inode = NULL;
+                       dput(lower_dentry);
+                       lower_dentry = ERR_PTR(err);
+                       goto out;
Really?  So what happens if it had become positive after your test and
somebody had looked it up in lower layer and just now happens to be
in the middle of operations on it?  Will be thucking frilled by that...

	* __unionfs_rename():
+       lock_rename(lower_old_dir_dentry, lower_new_dir_dentry);
+       err = vfs_rename(lower_old_dir_dentry->d_inode, lower_old_dentry,
+                        lower_new_dir_dentry->d_inode, lower_new_dentry);
+       unlock_rename(lower_old_dir_dentry, lower_new_dir_dentry);

Uh-huh...  To start with, what guarantees that your lower_old_dentry
is still a child of your lower_old_dir_dentry?  What's more, you are
not checking the result of lock_rename(), i.e. asking for serious trouble.

	* revalidation stuff: err...  how the devil can it work for
directories, when there's nothing to prevent changes in underlying
layers between ->d_revalidate() and operation itself?  For the upper
layer (unionfs itself) everything's more or less fine, but the rest
of that...