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.
Thanks Alperen, yes, that is the point: the existing reference stays valid throughout. On Sun, Nov 1, 2026 at 11:10 AM Alperen Erkan <[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 <[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 >> > >> >>
From b4fd2f256c9123f2d440b0bb6e0180ffc5b7fdac Mon Sep 17 00:00:00 2001 From: Milos Nikic <[email protected]> Date: Sun, 1 Nov 2026 21:35:04 -0800 Subject: [PATCH v2 3/3] ext2fs: Keep the directory when a lookup of its parent fails 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. diskfs_lookup_hard released it before diskfs_cached_lookup of the parent, so when that failed the directory was unlocked and dereferenced twice. Only unlock it for the lookup, lock it again if the lookup fails, and release the caller's reference once it succeeds. Say so in diskfs.h, for diskfs_lookup_hard and diskfs_lookup. --- ext2fs/dir.c | 9 +++++++-- libdiskfs/diskfs.h | 10 ++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/ext2fs/dir.c b/ext2fs/dir.c index 09f5a7225..94fedba23 100644 --- a/ext2fs/dir.c +++ b/ext2fs/dir.c @@ -316,10 +316,15 @@ diskfs_lookup_hard (struct node *dp, const char *name, lookup_flags_t l_flags, else if (type == LOOKUP) { - diskfs_nput (dp); + /* On failure the caller still has DP, locked. */ + pthread_mutex_unlock (&dp->lock); err = diskfs_cached_lookup (inum, &np); if (err) - goto out; + { + pthread_mutex_lock (&dp->lock); + goto out; + } + diskfs_nrele (dp); } else assert_backtrace (0); diff --git a/libdiskfs/diskfs.h b/libdiskfs/diskfs.h index 745e7044a..01511c8c5 100644 --- a/libdiskfs/diskfs.h +++ b/libdiskfs/diskfs.h @@ -352,8 +352,9 @@ error_t diskfs_set_statfs (fsys_statfsbuf_t *statfsbuf); diskfs_clear_directory, and diskfs_init_dir, then lookups for `..' might have the flag SPEC_DOTDOT or'd in. This has the following special meaning: - For LOOKUP: DP should be unlocked and its reference dropped before - returning. + For LOOKUP: if the lookup succeeds, DP should be unlocked and its + reference dropped before returning; if it fails, DP is + left as it was given, locked, with its reference. For RENAME and REMOVE: The node being found (*NP) is already held locked, so don't lock it or add a reference to it. (SPEC_DOTDOT will not be given with CREATE.) @@ -896,8 +897,9 @@ diskfs_disknode_node (struct disknode *disknode) diskfs_clear_directory, and diskfs_init_dir, then lookups for `..' might have the flag SPEC_DOTDOT or'd in. This has the following special meaning: - For LOOKUP: DP should be unlocked and its reference dropped before - returning. + For LOOKUP: if the lookup succeeds, DP should be unlocked and its + reference dropped before returning; if it fails, DP is + left as it was given, locked, with its reference. For RENAME and REMOVE: The node being found (*NP) is already held locked, so don't lock it or add a reference to it. (SPEC_DOTDOT will not be given with CREATE.) -- 2.56.0
