* Re: [GIT PULL] overlayfs fixes for 7.3
2026-10-31 9:57 [GIT PULL] overlayfs fixes for 7.3 Amir Goldstein
@ 2026-10-31 19:19 ` Linus Torvalds
2026-10-31 19:37 ` pr-tracker-bot
1 sibling, 0 replies; 3+ messages in thread
From: Linus Torvalds @ 2026-10-31 19:19 UTC (permalink / raw)
To: Amir Goldstein
Cc: Miklos Szeredi, Christian Brauner, linux-kernel, linux-fsdevel,
linux-unionfs
[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]
On Sat, 31 Oct 2026 at 02:57, Amir Goldstein <amir73il@gmail.com> 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
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 3922 bytes --]
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)
^ permalink raw reply [flat|nested] 3+ messages in thread