Re: Return failure from take_dentry_name_snapshot() (Was: [GIT PULL] overlayfs fixes for 7.3)
From: Linus Torvalds
Date: Mon Nov 02 2026 - 17:23:27 EST
On Sun, 1 Nov 2026 at 03:50, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Sat, Oct 31, 2026 at 8:19 PM Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> >
> > // 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;
This was wrong: it sets 'name->name.name' too late:
release_dentry_name_snapshot() already needs it set, because it then
compares that name pointer to the inline pointer to see if it was an
external name or not.
The fix is to just have a separate "release_external_name()" helper,
since that's what we actually want here.
> Attached proper patch.
Your proper patch ended up having my bug in it, so it will leak the
external name in the (admittedly very unlikely) case of
take_dentry_name_snapshot having gotten an external name reference but
then raced with a rename.
I think this attached patch is correct.
Linus
fs/dcache.c | 77 ++++++++++++++++++++++----------------------------
fs/overlayfs/super.c | 10 ++-----
include/linux/dcache.h | 3 +-
3 files changed, 37 insertions(+), 53 deletions(-)
diff --git a/fs/dcache.c b/fs/dcache.c
index 409f841e058e..79ce1d0e52c2 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -335,6 +335,12 @@ struct external_name {
unsigned char name[] __aligned(sizeof(unsigned long));
};
+static inline void release_external_name(struct external_name *p)
+{
+ if (unlikely(atomic_dec_and_test(&p->count)))
+ kfree_rcu(p, head);
+}
+
static inline struct external_name *external_name(struct dentry *dentry)
{
return container_of(dentry->d_name.name, struct external_name, name[0]);
@@ -359,65 +365,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)
+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;
- } else {
+ guard(rcu)();
+ for (;;) {
+ unsigned seq = read_seqcount_begin(&dentry->d_seq);
+ const unsigned char *s = READ_ONCE(dentry->d_name.name);
struct external_name *p;
+
+ 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))
+ continue;
+ return true;
+ }
+
+ // Try to get a valid external reference
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();
+ if (read_seqcount_retry(&dentry->d_seq, seq))
+ continue;
return false;
}
- 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);
+ // Ok, we have a name reference. Make sure it is still valid
+ name->name.name = s;
+ if (!read_seqcount_retry(&dentry->d_seq, seq))
+ return true;
+
+ release_external_name(p);
+ continue;
+ }
}
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)) {
struct external_name *p;
p = container_of(name->name.name, struct external_name, name[0]);
- if (unlikely(atomic_dec_and_test(&p->count)))
- kfree_rcu(p, head);
+ release_external_name(p);
}
}
EXPORT_SYMBOL(release_dentry_name_snapshot);
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)