On Thursday, May 23, 2024 11:27:28 AM CEST Dominique Martinet wrote: > Christian Schoenebeck wrote on Thu, May 23, 2024 at 10:34:14AM +0200: > > > The comment still works -- if detry->d_fsdata is NULL then > > > hlist_for_each_entry will stop short and not iterate over anything (it > > > won't bug out), so that part is fine in my opinion. > > > > I meant the opposite: dentry->d_fsdata not being NULL. > > I also meant that in the d_fsdata not being NULL branch, if d_fsdata > turns out to be NULL when it is read under lock later. > > > In this case v9fs_fid_find() takes a local copy of the list head > > pointer as `h` without taking a lock before. > > It doesn't, it takes &dentry->d_fsdata so the address of d_fsdata before > the lock, but that address cannot change here (another thread cannot > change the address of the dentry) ...(continuing below) Aaah right, I was missing the `&`, my bad! > > Then v9fs_fid_find() takes the lock to run hlist_for_each_entry(), but at this > > point `h` could already point at garbage. > > ... so *h (in practice, head->first in hlist_for_each_entry()) will > properly contain the first node of the list under lock: either NULL if > we just cleared it (at which point the loop won't iterate anything), or > a new list if other items have been added meanwhile. Yeah, looks fine to me. > I really think it's safe, but I do agree that it's hard to read, happy > to move the `h = &dentry->d_fsdata` inside the lock if you prefer -- it > compiles to the same code for me (x86_64/gcc 13.2.0) No need, you can add my RB. Thanks for the clarification! Reviewed-by: Christian Schoenebeck <linux_oss@xxxxxxxxxxxxx>
