Re: kern/46990: tmpfs_gro_genealogy() panic - rename/rmdir race condition not handled

[Wolfgang Stukenbrock <wolfgang.stukenbrock%nagler-company.com@localhost>]

The following reply was made to PR kern/46990; it has been noted by GNATS.

From: Wolfgang Stukenbrock <wolfgang.stukenbrock%nagler-company.com@localhost>
To: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: kern/46990: tmpfs_gro_genealogy() panic - rename/rmdir race 
condition not handled
Date: Mon, 24 Sep 2012 13:27:06 +0200

 Hi,
 
 I've added your patch version now and running a kernel with DIAGNOSTIC 
 enabled.
 
 Three times dirconc run without triggering the race.
 
 Then I got:
 tttt# ./dirconc3 /tmp | grep -v cleanup
 Concurrent directory ops test
 Starting in /tmp/dirconc
 tmpfs_vnode_get lost race with rmdir
 tmpfs_vnode_get lost race with rmdir
 Cleaning up
 tttt#
 
 remark: the grep in the output simply kills the MAXPATH-related messages 
 during cleanup that would scroll out everything otherwise.
 
 The systems survives without crash and the race happens twice.
 
 I think this patch should be integrated.
 PR46990 and PR46955 can be closed after that from my side.
 
 It would be nice to have a "corrected" tmpfs in 5.x too.
 At least on our systems, amanda-backup will lock-up the system at least 
 every two months.
 If my work will be the base, someone has to look at the auth-integration.
 I'm not shure if my aproach is correct - or in other words: I'm shure 
 that it is not 100% correct. It works for me (till now), but in Head to 
 much has changed since 5.x (see original comments in PR46955).
 I'm not familiar with the listener-list-stuff used now and the semantics 
 of the additional lists added since 5.x in Head that cannot be used in 
 5.x because the rest of the system does not know them as far as I can see.
 I've already added two root-workarounds for chmod and chown, so 
 something seems to be incorrect with my integration.
 
 best regards
 
 W. Stukenbrock
 
 
 
 Taylor R Campbell wrote:
 
 >    Date: Fri, 21 Sep 2012 13:10:26 +0200
 >    From: Wolfgang Stukenbrock 
 > <wolfgang.stukenbrock%nagler-company.com@localhost>
 > 
 >    If we can exclude the first loop pass, than the "problem" must happen 
 >    while walking up the parent chain.
 >    We will vput(vp) after getting tn_vlock on the parent - so we have no 
 >    lock on any vnode anymore.
 >    The normal way is to get tn_vlock and than seaches for the vnode by 
 >    calling tmpfs_vnode_get(). There we have two different ways to handle 
 >    the locks:
 >    ...
 > 
 > It looks to me as though this is the source of the problem.  I didn't
 > look closely enough at tmpfs_vnode_get before, but now that I look at
 > it, it seems that the only invariant it preserves is the existence of
 > the tmpfs node -- that is, tmpfs_reclaim will not free the tmpfs node
 > while we are getting a vnode for it.  All other invariants may be lost
 > because tmpfs_vnode_get drops all locks except the tmpfs reclaiming
 > bit.
 > 
 > In that case, it is necessary to check tmpfs_rmdired_p after
 > tmpfs_vnode_get, so try adding the following fragment at the very end
 > of the loop, after `error = tmpfs_vnode_get(mp, dnode, &vp); if
 > (error) return error;':
 > 
 >              /*
 >               * tmpfs_vnode_get only guarantees that dnode will not
 >               * be freed while we get a vnode for it.  It does not
 >               * preserve any other invariants, so we must check
 >               * whether the parent has been removed in the meantime.
 >               */
 >              if (tmpfs_rmdired_p(vp)) {
 >                      printf("tmpfs_vnode_get lost race with rmdir\n");
 >                      vput(vp);
 >                      return ENOENT;
 >              }
 > 
 > Can you try this and see how dirconc goes?  In particular, please let
 > me know if you hit the printf.
 > 
 >    What will happen if I've a reference to a directory (e.g. current-dir) 
 >    and someone delete it and the parent of it too?
 > 
 > Then you'll have a reference to the vnode for a deleted directory.
 > But if you have the vnode locked, then nothing can delete it.
 > 
 >    I've seen no code that will destroy the link between the vnode and the 
 >    node structure till now, but I assume it is present somewhere.
 > 
 > This happens in tmpfs_reclaim, when the vnode ceases to be referenced.
 > 
 >    As a second action all WHITEOUT children of the just removed directory 
 >    are simply deleted during directory delete operation. (calls to 
 >    tmpfs_dir_detach and tmpfs_free_dirent without any checks for vnode 
 >    references.)
 >    If there are still vnodes associated to the nodes in the dirents, is 
 >    this code save?
 > 
 > WHITEOUT stuff is not relevant unless you are using onionfs, so don't
 > worry about that logic.
 > 
 >    Just as information:
 >    The last problem I've fixed before was the sync with the cache after 
 >    rename occured.
 >    In Head the call to genfs_rename_cache_purge() was commented out in 
 >    tmpfs_gro_rename.
 >    5.1 seems to need it, because I've ended up with having the object in 
 >    the correct tmpfs-structure, but when accessing it in the cache it is 
 >    not found.
 >    Symtom: "ls <dir>" will show the name but "ls -l <dir>" say "file not 
 >    found".
 > 
 > This is not surprising.  It's probably safe to uncomment all of the
 > commented cache purges that I left scattered around -- the only reason
 > I didn't do that was to preserve the existing behaviour and to remind
 > me that the namecache needs careful analysis which I haven't done.
 > But as a stop-gap before that analysis is done, overzealously purging
 > the cache may improve correctness and will probably at worst only hurt
 > performance.
 > 
 >