Re: [PATCH] nfs_unlink() race (was: nfs_refresh_inode: inode number mismatch)

viro@parcelfarce.linux.theplanet.co.uk
Wed, 11 Jun 2003 01:54:25 +0100


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/