[hurd] 70/98: trans: fix reference counting and destruction of fake nodes

[Samuel Thibault]

This is an automated email from the git hooks/post-receive script.

sthibault pushed a commit to branch upstream
in repository hurd.

commit b770147356376ddb0602358a0252c4f68a9c80c6
Author: Justus Winter <address@hidden>
Date:   Thu Dec 5 19:40:31 2013 +0100

    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
---
 trans/fakeroot.c | 71 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 21 deletions(-)

diff --git a/trans/fakeroot.c b/trans/fakeroot.c
index 1233104..eff5225 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,

-- 
Alioth's /usr/local/bin/git-commit-notice on 
/srv/git.debian.org/git/pkg-hurd/hurd.git