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

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Al Viro
Date: Wednesday, January 16, 2008 - 11:00 pm

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...
--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
[PATCH 01/29] Unionfs: documentation, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 03/29] Makefile: hook to compile unionfs, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 04/29] Unionfs: main Makefile, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 05/29] Unionfs: fanout header definitions, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 06/29] Unionfs: main header file, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 08/29] Unionfs: basic file operations, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 09/29] Unionfs: lower-level copyup routines, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 10/29] Unionfs: dentry revalidation, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 11/29] Unionfs: lower-level lookup routines, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 12/29] Unionfs: rename method and helpers, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 16/29] Unionfs: inode operations, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 17/29] Unionfs: unlink/rmdir operations, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 18/29] Unionfs: address-space operations, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 20/29] Unionfs: super_block operations, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 22/29] Unionfs: async I/O queue, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 23/29] Unionfs: miscellaneous helper routines, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 24/29] Unionfs: debugging infrastructure, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 25/29] Unionfs file system magic number, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 27/29] VFS path get/put ops used by Unionfs, Erez Zadok, (Thu Jan 10, 7:59 am)
[PATCH 28/29] VFS: export release_open_intent symbol, Erez Zadok, (Thu Jan 10, 7:59 am)
Re: [UNIONFS] 00/29 Unionfs and related patches pre-merge ..., Christoph Hellwig, (Thu Jan 10, 8:08 am)
Re: [UNIONFS] 00/29 Unionfs and related patches pre-merge ..., Al Viro, (Wed Jan 16, 11:00 pm)