On Sat, Nov 30, 2024 at 10:02:38AM -0800, Linus Torvalds wrote: > On Sat, 30 Nov 2024 at 04:30, Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > What does the smp_load_acquire() pair with? > > I'm not sure we have them everywhere, but at least this one at dentry > creation time. > > __d_alloc(): > /* Make sure we always see the terminating NUL character */ > smp_store_release(&dentry->d_name.name, dname); /* ^^^ */ > > so even at rename time, when we swap the d_name.name pointers > (*without* using a store-release at that time), both of the dentry > names had memory orderings before. > > That said, looking at swap_name() at the non-"swap just the pointers" > case, there we do just "memcpy()" the name, and it would probably be > good to update the target d_name.name with a smp_store_release. > > In practice, none of this ever matters. Anybody who uses the dentry > name without locking either doesn't care enough (like comm[]) or will > use the sequence number thing to serialize at a much higher level. So > the smp_load_acquire() could probably be a READ_ONCE(), and nobody > would ever see the difference. Right now it's confusing. So no matter if we do READ_ONCE() or smp_load_acquire() there'd please be a comment explaing why so we don't pointlessly leave everyone wondering about that barrier. /* * Hold rcu lock to keep the name from being freed behind our back. * Use cquire semantics to make sure the terminating NUL from * __d_alloc() is seen. * * Note, we're deliberately sloppy here. We don't need to care about * detecting a concurrent rename and just want a sensible name. */ rcu_read_lock(); __set_task_comm(me, smp_load_acquire(&file_dentry(bprm->file)->d_name.name), true); rcu_read_unlock(); or something better.
