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...