On Mon, Jan 27, 2025 at 09:34:56PM +0000, Al Viro wrote:
> If so, then
> a) it's a false positive (and IIRC, it's not the first time
> kfence gets confused by that)
> b) your bisection will probably converge to bdd9951f60f9
> "dissolve external_name.u into separate members" which is where we'd
> ended up with offsetof(struct external_name, name) being 4 modulo 8.
>
> As a quick test, try to flip the order of head and count in
> struct external_name and see if that makes the warning go away.
> If it does, I'm pretty certain that theory above is correct.
Not quite... dentry_string_cmp() assumes that ->d_name.name is
word-aligned, so load_unaligned_zeropad() is done only to the
second string (the one we compare against).
Linus, does the following look sane to you as replacement for
bdd9951f60f9? I'd rather have explicit __aligned(), along
with the comment spelling the constraints out...
diff --git a/fs/dcache.c b/fs/dcache.c
index f387dc97df86..f8d6a2557736 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -295,12 +295,16 @@ static inline int dentry_cmp(const struct dentry *dentry, const unsigned char *c
return dentry_string_cmp(cs, ct, tcount);
}
+/*
+ * long names are allocated separately from dentry and never modified.
+ * Refcounted, freeing is RCU-delayed. See take_dentry_name_snapshot()
+ * for the reason why ->count and ->head can't be combined into a union.
+ * dentry_string_cmp() relies upon ->name[] being word-aligned.
+ */
struct external_name {
- struct {
- atomic_t count; // ->count and ->head can't be combined
- struct rcu_head head; // see take_dentry_name_snapshot()
- } u;
- unsigned char name[];
+ atomic_t count;
+ struct rcu_head head;
+ unsigned char name[] __aligned(sizeof(unsigned long));
};
static inline struct external_name *external_name(struct dentry *dentry)
@@ -344,7 +348,7 @@ void take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry
struct external_name *p;
p = container_of(s, struct external_name, name[0]);
// get a valid reference
- if (unlikely(!atomic_inc_not_zero(&p->u.count)))
+ if (unlikely(!atomic_inc_not_zero(&p->count)))
goto retry;
name->name.name = s;
}
@@ -361,8 +365,8 @@ 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->u.count)))
- kfree_rcu(p, u.head);
+ if (unlikely(atomic_dec_and_test(&p->count)))
+ kfree_rcu(p, head);
}
}
EXPORT_SYMBOL(release_dentry_name_snapshot);
@@ -400,7 +404,7 @@ static void dentry_free(struct dentry *dentry)
WARN_ON(!hlist_unhashed(&dentry->d_u.d_alias));
if (unlikely(dname_external(dentry))) {
struct external_name *p = external_name(dentry);
- if (likely(atomic_dec_and_test(&p->u.count))) {
+ if (likely(atomic_dec_and_test(&p->count))) {
call_rcu(&dentry->d_u.d_rcu, __d_free_external);
return;
}
@@ -1681,7 +1685,7 @@ static struct dentry *__d_alloc(struct super_block *sb, const struct qstr *name)
kmem_cache_free(dentry_cache, dentry);
return NULL;
}
- atomic_set(&p->u.count, 1);
+ atomic_set(&p->count, 1);
dname = p->name;
} else {
dname = dentry->d_shortname.string;
@@ -2774,15 +2778,15 @@ static void copy_name(struct dentry *dentry, struct dentry *target)
if (unlikely(dname_external(dentry)))
old_name = external_name(dentry);
if (unlikely(dname_external(target))) {
- atomic_inc(&external_name(target)->u.count);
+ atomic_inc(&external_name(target)->count);
dentry->d_name = target->d_name;
} else {
dentry->d_shortname = target->d_shortname;
dentry->d_name.name = dentry->d_shortname.string;
dentry->d_name.hash_len = target->d_name.hash_len;
}
- if (old_name && likely(atomic_dec_and_test(&old_name->u.count)))
- kfree_rcu(old_name, u.head);
+ if (old_name && likely(atomic_dec_and_test(&old_name->count)))
+ kfree_rcu(old_name, head);
}
/*
