Re: [GIT PULL] overlayfs fixes for 7.3

[Linus Torvalds]

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?

                 Linus
 fs/dcache.c            | 38 +++++++++++++-------------------------
 fs/overlayfs/super.c   |  9 +++------
 include/linux/dcache.h |  3 +--
 3 files changed, 17 insertions(+), 33 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 409f841e058e..fe037a876d76 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 08695a99f4f9..2e160b7dcfe7 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -100,17 +100,14 @@ 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)