On Sun, 8 Dec 2024 at 19:52, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> + struct external_name *p;
> + p = container_of(name->name.name, struct external_name, name[0]);
> + // get a valid reference
> + if (unlikely(!atomic_inc_not_zero(&p->u.count)))
> + goto retry;
Oh - this is very much *not* safe.
The other comment I had was really about "that's bad for performance".
But this is actually actively buggy.
If the external name ref has gone down to zero, we can *not* do that
atomic_inc_not_zero(..)
thing any more, because the recount is in a union with the rcu_head
for delaying the free.
In other words: the *name* will exist for the duration of the
rcu_read_lock() we hold, but that "p->u.count" will not. When the
refcount has gone to zero, the refcount is no longer usable.
IOW, you may be happily incrementing what is now a RCU list head
rather than a count.
So NAK. This cannot work.
It's probably easily fixable by just not using a union in struct
external_name, and just having separate fields for the refcount and
the rcu_head, but in the current state your patch is fundamentally and
dangerously buggy.
(Dangerously, because you will never *ever* actually hit that tiny
race - you need to race with a concurrent rename *and* a drop of the
other dentry that got the external ref, so in practice this is likely
entirely impossible to ever hit, but that only makes it more subtle
and dangerous).
Linus
