Re: [PATCH 08/13] trans: fix reference counting and destruction of fake nodes

[Samuel Thibault]

Justus Winter, le Mon 09 Dec 2013 15:16:36 +0100, a écrit :
> 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.

Ack.

> * 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,
> -- 
> 1.7.10.4
> 

-- 
Samuel
<m> bouhouhouh, b il m'a abandonné. Tout ca parce que je regardais plus mon 
emacs que lui !
<m> s/lui/ses messages irc/
 -+- #ens-mim esseulé -+-