Re: [PATCH 2/3] libdiskfs: Do not unlock the old root again with --virtual-root

[Milos Nikic]

Hello Samuel,

Thanks for the review.

The full shape is:

      pthread_mutex_lock (&diskfs_root_node->lock);          /* line 90 */
      ...
      err = diskfs_lookup (diskfs_root_node, _diskfs_chroot_directory,
                           LOOKUP, &np, NULL, rootpi);
      pthread_mutex_unlock (&diskfs_root_node->lock);        /* line 102 */
      ...
      old = diskfs_root_node;
      diskfs_root_node = np;
      if (np != old)
        pthread_mutex_unlock (&np->lock);
      diskfs_nrele (old);

So old is diskfs_root_node, which is already unlocked at line 102 (of
the patched file).  diskfs_nput (old) unlocked it a second time (it
fails when the locks are made PTHREAD_MUTEX_ERRORCHECK);
only its reference is left to drop, hence diskfs_nrele.

Alternatively, would it be simpler to just reject --virtual-root=. ?
Changing:
      if (err == EAGAIN)
        error (1, 0, "`--virtual-root=%s' specifies the real root
directory",
               _diskfs_chroot_directory);

to something like:

      if (err == EAGAIN || (!err && np == diskfs_root_node))
        error (1, 0, "`--virtual-root=%s' specifies the real root
directory",
               _diskfs_chroot_directory);

and then we wouldn't have this confusing case at all.

Thanks,
Milos

On Sun, Nov 1, 2026 at 10:49 AM Samuel Thibault <samuel.thiba...@gnu.org>
wrote:

> Milos Nikic, le jeu. 29 oct. 2026 21:26:40 -0700, a ecrit:
> > diskfs_startup_diskfs unlocks the root node after looking up the
> > virtual root in it, then lets go of it with diskfs_nput, which unlocks
> > it a second time.  Release only its reference.
> >
> > With --virtual-root=., the lookup of "." gives back the root node
> > itself, already unlocked with the root: do not unlock it again either.
> >
> > A default mutex does not check its owner, so the second unlock goes
> > unnoticed unless another thread holds the lock by then, which loses it.
> > With error-checking mutexes, an ext2fs started with --virtual-root=.
> > dies at that unlock.
> > ---
> >  libdiskfs/init-startup.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/libdiskfs/init-startup.c b/libdiskfs/init-startup.c
> > index d51cc2787..736cdb502 100644
> > --- a/libdiskfs/init-startup.c
> > +++ b/libdiskfs/init-startup.c
> > @@ -120,8 +120,10 @@ diskfs_startup_diskfs (mach_port_t fs_bootstrap,
> int flags)
> >        if _diskfs_chroot_directory is non-null.  */
> >        old = diskfs_root_node;
> >        diskfs_root_node = np;
> > -      pthread_mutex_unlock (&np->lock);
> > -      diskfs_nput (old);
> > +      /* A virtual root that is the root itself was unlocked with it.
> */
> > +      if (np != old)
> > +     pthread_mutex_unlock (&np->lock);
> > +      diskfs_nrele (old);
>
> Mmm, but then we don't unlock old?
>
> Samuel
>