On Mon, Dec 09, 2024 at 11:06:48AM -0800, Linus Torvalds wrote: > 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. D'oh. Right you are; missed it... > 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. Agreed. And yes, separating the fields (and slapping a comment explaining why they can not be combined) would be the easiest solution - any attempts to be clever here would be too brittle for no good reason.
