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

[Amir Goldstein]

On Sat, Oct 31, 2026 at 8:19 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Sat, 31 Oct 2026 at 02:57, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > Amir Goldstein (6):
> >       vfs: add try_take_dentry_name_snapshot()
>
> I've pulled this, but I detest this particular commit. It just
> "smells" wrong to me.
>
> I don't doubt that it fixes a real bug in overlayfs, but I think the
> fix is entriely wrong. It is wrong in two ways:
>
>  - it incorrectly fails when the issue is a race with a rename (this
> is harmless for overlayfs and will just result in -ECHILD)
>
>  - it adds a bogus new "try" helper that I don't believe should be
> needed or exist
>
> I think the proper fix would be to just say that the
> !atomic_inc_not_zero() can indeed fail, and we should never retry
> based on that.
>
> But if we should *always* retry based on dentry->d_seq.
>
> And once you retry correctly based on dentry->d_seq, now the "we
> didn't get a name reference" is reliable: if it happened despite d_seq
> being stable, it's because we really don't have a name any more, and
> so we should return failure.
>
> Put another way: I think we should have just one
> "take_dentry_name_snapshot()" function, and it should return a
> boolean, and look somethign like this:
>
>   bool take_dentry_name_snapshot(struct name_snapshot *name, struct
> dentry *dentry)
>   {
>         unsigned seq;
>         const unsigned char *s;
>
>         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;
>                 if (read_seqcount_retry(&dentry->d_seq, seq))
>                         goto retry;
>         } else {
>                 struct external_name *p;
>                 p = container_of(s, struct external_name, name[0]);
>
>                 // Try to get a valid reference
>                 if (unlikely(!atomic_inc_not_zero(&p->count))) {
>                         if (read_seqcount_retry(&dentry->d_seq, seq))
>                                 goto retry;
>                         rcu_read_unlock();
>                         return false;
>                 }
>
>                 // Ok, we have a name reference. Make sure it is still valid
>                 if (read_seqcount_retry(&dentry->d_seq, seq)) {
>                         release_dentry_name_snapshot(name);
>                         goto retry;
>                 }
>
>                 name->name.name = s;
>         }
>         rcu_read_unlock();
>         return true;
>   }
>   EXPORT_SYMBOL(take_dentry_name_snapshot);
>
> (Note that in this version, if it fails it will just reutn the
> shortname in name->name.name, but it should never fail for a dentry
> that you hold a reference to afaik).
>
> Attached it an ENTIRELY UNTESTED patch that does all the other changes
> too to make overlayfs use this version.
>
> Again: ENTIRELY UNTESTED. Maybe I'm missing something and I'm just
> wrong on this. But I really don't like that nasty
> try_take_dentry_name_snapshot() thing. It really looks wrong to me,
> both conceptually and in being actively buggy and racing with renames.
>
> Hmm? Comments?

I like this. Small bug in ovl code.
Forgot to remove old take_dentry_name_snapshot().

Attached proper patch.
It's sanity tested but I did not try Chris' reproducer with added delays.

Feel free to apply and/or wait for ACK from Al/Christian,
or I can include it in my next ovl-fixes PR.

Thanks,
Amir.
From 6b027085c470a6641783a30f827cd047a7f7a072 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Sun, 1 Nov 2026 09:47:19 +0100
Subject: [PATCH] vfs: take_dentry_name_snapshot() may fail

This patch was posted by Linus as a better alternative to my
try_take_dentry_name_snapshot() for fixing the overlayfs issue.

Quoting Linus:
I've pulled this, but I detest this particular commit. It just
"smells" wrong to me.

I don't doubt that it fixes a real bug in overlayfs, but I think the
fix is entriely wrong. It is wrong in two ways:

 - it incorrectly fails when the issue is a race with a rename (this
is harmless for overlayfs and will just result in -ECHILD)

 - it adds a bogus new "try" helper that I don't believe should be
needed or exist

I think the proper fix would be to just say that the
!atomic_inc_not_zero() can indeed fail, and we should never retry
based on that.

But if we should *always* retry based on dentry->d_seq.

And once you retry correctly based on dentry->d_seq, now the "we
didn't get a name reference" is reliable: if it happened despite d_seq
being stable, it's because we really don't have a name any more, and
so we should return failure.

Put another way: I think we should have just one
"take_dentry_name_snapshot()" function, and it should return a
boolean.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/r/CAHk-=wiPhTLn1p0ZHx-7SMdk4_zazqDK1k7MMEn9scRfqeDgtw@mail.gmail.com/
Fixes: 15c6bd5cf1094 ("vfs: add try_take_dentry_name_snapshot()")
Cc: stable@vger.kernel.org
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/dcache.c            | 38 +++++++++++++-------------------------
 fs/overlayfs/super.c   | 10 +++-------
 include/linux/dcache.h |  3 +--
 3 files changed, 17 insertions(+), 34 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 409f841e058e4..fe037a876d767 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -359,8 +359,7 @@ 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)
+bool take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry)
 {
 	unsigned seq;
 	const unsigned char *s;
@@ -373,44 +372,33 @@ static bool __take_dentry_name_snapshot(struct name_snapshot *name,
 	name->name.name = name->inline_name.string;
 	if (likely(s == dentry->d_shortname.string)) {
 		name->inline_name = dentry->d_shortname;
+		if (read_seqcount_retry(&dentry->d_seq, seq))
+			goto retry;
 	} else {
 		struct external_name *p;
 		p = container_of(s, struct external_name, name[0]);
-		// get a valid reference
+
+		// Try to get a valid reference
 		if (unlikely(!atomic_inc_not_zero(&p->count))) {
-			if (retry)
+			if (read_seqcount_retry(&dentry->d_seq, seq))
 				goto retry;
 			rcu_read_unlock();
 			return false;
 		}
+
+		// Ok, we have a name reference. Make sure it is still valid
+		if (read_seqcount_retry(&dentry->d_seq, seq)) {
+			release_dentry_name_snapshot(name);
+			goto retry;
+		}
+
 		name->name.name = s;
 	}
-	if (read_seqcount_retry(&dentry->d_seq, seq)) {
-		release_dentry_name_snapshot(name);
-		goto retry;
-	}
 	rcu_read_unlock();
 	return true;
 }
-
-void take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry)
-{
-	__take_dentry_name_snapshot(name, dentry, true);
-}
 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 08695a99f4f93..d9174eaccce2d 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 84b0cfe4ed0e7..aaa4ba7a12991 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)
-- 
2.55.0