Milos Nikic, le dim. 01 nov. 2026 22:28:44 -0800, a ecrit: > Indeed, the nref/nput only amounts to unlocking DP: the caller's reference > keeps it alive across the lookup. > v2 just unlocks it, locks it again on failure, and releases the caller's > reference with diskfs_nrele on success. > It also documents in diskfs.h, for both diskfs_lookup_hard and diskfs_lookup, > that a SPEC_DOTDOT LOOKUP leaves DP locked and referenced when it fails.
Applied, thanks! Samuel > > Thanks Alperen, yes, that is the point: the existing reference stays valid > throughout. > > > > On Sun, Nov 1, 2026 at 11:10 AM Alperen Erkan <[1][email protected]> > wrote: > > 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 > <[2][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 > > > > > > References: > > [1] mailto:[email protected] > [2] mailto:[email protected] -- Samuel Hi ! I'm a .signature virus ! Copy me into your ~/.signature, please !
