Re: Return failure from take_dentry_name_snapshot() (Was: [GIT PULL] overlayfs fixes for 7.3)

[Linus Torvalds]

On Mon, 2 Nov 2026 at 11:55, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> I think this attached patch is correct.

Bah. I think it *was* correct, but I hated the deep indentations and
wanted to add a few comments about the failure case.

So I rewrote it with a helper function, with he outer function doing
both the RCU read-side protection and the sequence number test:

  bool take_dentry_name_snapshot(struct name_snapshot *name, struct
dentry *dentry)
  {
        guard(rcu)();
        for (;;) {
                unsigned seq = read_seqcount_begin(&dentry->d_seq);
                bool retval = try_name_snapshot(name, dentry);
                if (!read_seqcount_retry(&dentry->d_seq, seq))
                        return retval;
                release_dentry_name_snapshot(name);
        }
  }

which makes it fairly straightforward (with the main subtlety just
being that try_name_snapshot() needs to set things up so that
release_dentry_name_snapshot() is ok to call even on failure, but that
was effectively true before too).

I have now looked at this code so much that I have semantic satiation
and it has lost all meaning.

Time to stop.

                   Linus
 fs/dcache.c            | 66 +++++++++++++++++++++-----------------------------
 fs/overlayfs/super.c   | 10 +++-----
 include/linux/dcache.h |  3 +--
 3 files changed, 32 insertions(+), 47 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 409f841e058e..b2fa5f0a68c4 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -359,58 +359,48 @@ static inline int dname_external(const struct dentry *dentry)
 	return dentry->d_name.name != dentry->d_shortname.string;
 }
 
-static bool __take_dentry_name_snapshot(struct name_snapshot *name,
-					struct dentry *dentry, bool retry)
+static bool try_name_snapshot(struct name_snapshot *name, struct dentry *dentry)
 {
-	unsigned seq;
-	const unsigned char *s;
+	const unsigned char *s = READ_ONCE(dentry->d_name.name);
+	struct external_name *p;
 
-	rcu_read_lock();
-retry:
-	seq = read_seqcount_begin(&dentry->d_seq);
-	s = READ_ONCE(dentry->d_name.name);
 	name->name.hash_len = dentry->d_name.hash_len;
 	name->name.name = name->inline_name.string;
 	if (likely(s == dentry->d_shortname.string)) {
 		name->inline_name = dentry->d_shortname;
-	} else {
-		struct external_name *p;
-		p = container_of(s, struct external_name, name[0]);
-		// get a valid reference
-		if (unlikely(!atomic_inc_not_zero(&p->count))) {
-			if (retry)
-				goto retry;
-			rcu_read_unlock();
-			return false;
-		}
-		name->name.name = s;
+		return true;
 	}
-	if (read_seqcount_retry(&dentry->d_seq, seq)) {
-		release_dentry_name_snapshot(name);
-		goto retry;
-	}
-	rcu_read_unlock();
+
+	// Try to get a valid external reference
+	//
+	// In case we fail to get a ref, 'name.name' will
+	// point to the inline_name, which leaves the name
+	// invalid. But release_dentry_name_snapshot() can
+	// be called regardless.
+	p = container_of(s, struct external_name, name[0]);
+	if (unlikely(!atomic_inc_not_zero(&p->count)))
+		return false;
+
+	// We got a ref and the name is now needs to be
+	// free'd regardless of whether the sequence count
+	// matches or not
+	name->name.name = s;
 	return true;
 }
 
-void take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry)
+bool take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry)
 {
-	__take_dentry_name_snapshot(name, dentry, true);
+	guard(rcu)();
+	for (;;) {
+		unsigned seq = read_seqcount_begin(&dentry->d_seq);
+		bool retval = try_name_snapshot(name, dentry);
+		if (!read_seqcount_retry(&dentry->d_seq, seq))
+			return retval;
+		release_dentry_name_snapshot(name);
+	}
 }
 EXPORT_SYMBOL(take_dentry_name_snapshot);
 
-/*
- * For a caller that holds no reference on @dentry, e.g. in RCU walk.
- * The external name of a dead dentry has a zero refcount forever, so
- * take_dentry_name_snapshot() would retry without end.  Fail instead.
- */
-bool try_take_dentry_name_snapshot(struct name_snapshot *name,
-				   struct dentry *dentry)
-{
-	return __take_dentry_name_snapshot(name, dentry, false);
-}
-EXPORT_SYMBOL(try_take_dentry_name_snapshot);
-
 void release_dentry_name_snapshot(struct name_snapshot *name)
 {
 	if (unlikely(name->name.name != name->inline_name.string)) {
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 08695a99f4f9..d9174eaccce2 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -100,17 +100,13 @@ static int ovl_revalidate_real(struct dentry *d, unsigned int flags, bool weak)
 			dir = d_inode_rcu(parent);
 			if (!dir)
 				return -ECHILD;
-			/*
-			 * Nothing pins d in RCU walk: eviction of the overlay
-			 * inode may drop the last reference at any time.
-			 */
-			if (!try_take_dentry_name_snapshot(&n, d))
-				return -ECHILD;
 		} else {
 			parent = dget_parent(d);
 			dir = d_inode(parent);
-			take_dentry_name_snapshot(&n, d);
 		}
+		/* In RCU walk the dentry name may go away at any time */
+		if (!take_dentry_name_snapshot(&n, d))
+			return -ECHILD;
 		ret = d->d_op->d_revalidate(dir, &n.name, d, flags);
 		release_dentry_name_snapshot(&n);
 		if (!(flags & LOOKUP_RCU))
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 84b0cfe4ed0e..aaa4ba7a1299 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -664,8 +664,7 @@ struct name_snapshot {
 	struct qstr name;
 	union shortname_store inline_name;
 };
-void take_dentry_name_snapshot(struct name_snapshot *, struct dentry *);
-bool try_take_dentry_name_snapshot(struct name_snapshot *, struct dentry *);
+bool take_dentry_name_snapshot(struct name_snapshot *, struct dentry *);
 void release_dentry_name_snapshot(struct name_snapshot *);
 
 static inline struct dentry *d_first_child(const struct dentry *dentry)