Messages in this thread | | | Date | Wed, 11 Jun 2003 01:54:25 +0100 | From | viro@parcelfa ... | Subject | Re: [PATCH] nfs_unlink() race (was: nfs_refresh_inode: inode number mismatch) |
| |
On Mon, Jun 09, 2003 at 06:51:41AM -0700, Frank Cusack wrote:
> When foo is unlinked, nfs_unlink() does a sillyrename, this puts the > dentry on nfs_delete_queue, and (in the VFS) unhashes it from the dcache. > This causes a problem, because dentry->d_parent->d_inode is now guaranteed > to remain stale. (OK, I'm not really sure about this last part.)
????
What does hashed state have to ->d_parent? > Then readdir() returns the new .nfs file, this creates a NEW dentry > (we just moved the first one to nfs_delete_queue and did not create a > negative dentry) which now has d_count==1 so instead of sillyrename we > just remove it (but note, we actually have this file open). Then rmdir > succeeeds. > > Now, we have a dentry on nfs_delete_queue which a) has already been > unlinked and b) whose dentry->d_parent DNE and dentry->d_parent->d_inode > DNE. Of course this will cause confusion later!
b) is bogus. Unhashing does nothing to ->d_parent.
> Note that if a process does a drive by on the .nfs file (another round > of unlinked-but-open) before we unlink it, we would sillyrename it again. > We'd now have two different dentry's on the delete queue for the same > file. (One of them would just leak, I think--possible local DoS?)
Two different dentries for the same file is obviously not a problem...
> 1) Don't unhash the dentry after silly-renaming. In 2.2, each fs is > responsible for doing a d_delete(), in 2.4 it happens in the VFS and > I think it was just an oversight that the 2.4 VFS doesn't consider > sillyrename (considering the code and comments that are cruft). > > This preserves the unlinked-but-open semantic, but breaks rmdir. So > it's not a clear winner from a semantics POV. dentry->d_count is > always correct, which sounds like a plus. > > The patch to make this work is utterly simple, which is a big plus.
... and AFAICS it opens a huge can of worms with races in NFS unlink/rename.
Sigh... I'll look through that code and try to reconstruct the analysis. It used to be a very big mess and there was fairly subtle logics around unhashing/checks for d_count/etc. involved in fixing ;-/ And there was a lot of changes since then. Oh, well... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |