trans: fix reference counting and destruction of fake nodes

Previously, fakeroot tried to do too much in netfs_node_norefs.  This
function is meant to deallocate nodes.  Fakeroot however also tries to
remove the node from the hash table and to prolong the lifetime of the
node object by re-referencing it.

Removing the object from the hash table is highly problematic, because
at this point we already have the node locked.  With proper locking in
netfs_S_dir_lookup, acquiring the hash table lock while we hold the
node locked results in dead-locks, releasing the node lock before
acquiring the hash table lock results in a race condition.

Prolonging the lifetime of the node by re-acquiring a reference is
clearly a hack that surprisingly works to some degree.  The nodes
transbox, however, is already gone at this point.

This code was never actually run because of a reference-counting bug
in fakeroot.

Fix this by installing our own clean routine in the
netfs_protid_class.  This function is called without the associated
node being locked, allowing us to acquire the locks in the proper
order and to keep the hash table locked while the node is being
destroyed.

* trans/fakeroot.c (netfs_node_norefs): Just free the associated
resources.
(fakeroot_netfs_release_protid): New function doing cleanly what
netfs_node_norefs did before.
(netfs_S_dir_lookup): Reuse the fake reference.
(main): Install fakeroot_netfs_release_protid as clean routine.

fixup_fix_refc_destruction

1 files changed, 50 insertions, 21 deletions

diff --git a/trans/fakeroot.c b/trans/fakeroot.c
index 12331047..eff52258 100644
--- a/trans/fakeroot.c
+++ b/trans/fakeroot.c
@@ -128,6 +128,31 @@ void
 netfs_node_norefs (struct node *np)
 {
   assert (np->nn->np == np);
+  mach_port_deallocate (mach_task_self (), np->nn->file);
+  mach_port_deallocate (mach_task_self (), np->nn->idport);
+  free (np->nn);
+  free (np);
+}
+
+/* This is the cleanup function we install in netfs_protid_class.  If
+   the associated nodes reference count would also drop to zero, and
+   the node has no faked attributes, we destroy it as well.  */
+static void
+fakeroot_netfs_release_protid (void *cookie)
+{
+  struct node *np = ((struct protid *) cookie)->po->np;
+  assert (np->nn->np == np);
+
+  pthread_mutex_lock (&idport_ihash_lock);
+  pthread_mutex_lock (&np->lock);
+
+  assert ((np->nn->faked & FAKE_REFERENCE) == 0);
+
+  /* Check if someone else reacquired a reference to the node besides
+     ours that is about to be dropped.  */
+  if (np->references > 1)
+    goto out;
+
   if (np->nn->faked != 0
       && netfs_validate_stat (np, 0) == 0 && np->nn_stat.st_nlink > 0)
     {
@@ -139,30 +164,24 @@ netfs_node_norefs (struct node *np)
 	 until this fakeroot filesystem dies.  One easy solution
 	 would be to scan nodes with references=1 for st_nlink=0
 	 at some convenient time, periodically or in syncfs.  */
-      if ((np->nn->faked & FAKE_REFERENCE) == 0)
-	{
-	  np->nn->faked |= FAKE_REFERENCE;
-	  ++np->references;
-	}
-      pthread_mutex_unlock (&np->lock);
-      return;
-    }
 
-  pthread_spin_unlock (&netfs_node_refcnt_lock); /* Avoid deadlock.  */
-  pthread_mutex_lock (&idport_ihash_lock);
-  pthread_spin_lock (&netfs_node_refcnt_lock);
-  /* Previous holder of this lock might have just got a reference.  */
-  if (np->references > 0)
-    {
-      pthread_mutex_unlock (&idport_ihash_lock);
-      return;
+      /* Keep a fake reference.  */
+      netfs_nref (np);
+
+      /* Set the FAKE_REFERENCE flag so that we can properly
+	 account for that fake reference.  */
+      np->nn->faked |= FAKE_REFERENCE;
+
+      /* We keep our node.  */
+      goto out;
     }
+
   hurd_ihash_locp_remove (&idport_ihash, np->nn->idport_locp);
+
+ out:
+  pthread_mutex_unlock (&np->lock);
+  netfs_release_protid (cookie);
   pthread_mutex_unlock (&idport_ihash_lock);
-  mach_port_deallocate (mach_task_self (), np->nn->file);
-  mach_port_deallocate (mach_task_self (), np->nn->idport);
-  free (np->nn);
-  free (np);
 }
 
 /* Given an existing node, make sure it has NEWMODES in its openmodes.
@@ -326,7 +345,14 @@ netfs_S_dir_lookup (struct protid *diruser,
 	      pthread_mutex_lock (&np->lock);
 	      err = check_openmodes (np->nn, (flags & (O_RDWR|O_EXEC)), file);
 	      if (!err)
-		netfs_nref (np);
+		{
+		  /* If the looked-up file carries a fake reference, we
+		     use that and clear the FAKE_REFERENCE flag.  */
+		  if (np->nn->faked & FAKE_REFERENCE)
+		    np->nn->faked &= ~FAKE_REFERENCE;
+		  else
+		    netfs_nref (np);
+		}
 	      pthread_mutex_unlock (&np->lock);
 	      pthread_mutex_unlock (&idport_ihash_lock);
 	    }
@@ -957,6 +983,9 @@ any user to open nodes regardless of permissions as is done for root." };
   task_get_bootstrap_port (mach_task_self (), &bootstrap);
   netfs_init ();
 
+  /* Install our own clean routine.  */
+  netfs_protid_class->clean_routine = fakeroot_netfs_release_protid;
+
   /* Get our underlying node (we presume it's a directory) and use
      that to make the root node of the filesystem.  */
   err = new_node (netfs_startup (bootstrap, O_READ), MACH_PORT_NULL, 0, O_READ,