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é -+-
