Hello Would replacing the nref/nput sequence with a simple unlock preserve the expected reference ownership semantics in both the success and failure paths?
My understanding is that the additional reference is intended to keep dp alive while diskfs_cached_lookup() executes, without transferring ownership of the caller's reference. If that is the case, directly unlocking dp seems simpler, provided that the existing reference remains valid throughout the lookup and is released appropriately afterward. I also agree that documenting the locking and reference ownership contract of diskfs_lookup_hard() in libdiskfs/diskfs.h would help prevent similar issues. Best regards, Alperen Erkan On Sun, Nov 1, 2026 at 9:55 PM Samuel Thibault <[email protected]> wrote: > Milos Nikic, le jeu. 29 oct. 2026 21:26:41 -0700, a ecrit: > > A lookup of ".." with SPEC_DOTDOT and LOOKUP releases the directory > > only when it succeeds; its callers, checkpath and ancestors in > > diskfs_rename_dir, release it themselves otherwise. > > This should probably be specified in the comment of the prototype of > diskfs_lookup_hard in libdiskfs/diskfs.h > > > diskfs_lookup_hard > > released it before diskfs_cached_lookup of the parent, so when that > > failed the directory was unlocked and dereferenced twice. > > > > Hold a reference across the lookup, and give the directory back locked > > when it fails. > > --- > > ext2fs/dir.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/ext2fs/dir.c b/ext2fs/dir.c > > index 09f5a7225..583232f36 100644 > > --- a/ext2fs/dir.c > > +++ b/ext2fs/dir.c > > @@ -316,10 +316,17 @@ diskfs_lookup_hard (struct node *dp, const char > *name, lookup_flags_t l_flags, > > > > else if (type == LOOKUP) > > { > > + /* Hold DP until the lookup succeeds: on failure the caller > > + still has it, locked. */ > > + diskfs_nref (dp); > > diskfs_nput (dp); > > ? This is taking a hard ref, only to release it immediately after. > Better just unlock dp? > > Samuel > > > err = diskfs_cached_lookup (inum, &np); > > if (err) > > - goto out; > > + { > > + pthread_mutex_lock (&dp->lock); > > + goto out; > > + } > > + diskfs_nrele (dp); > > } > > else > > assert_backtrace (0); > > -- > > 2.56.0 > > > >
