On Thu, May 23, 2019 at 03:39:59PM -0400, Jerome Glisse wrote: > On Thu, May 23, 2019 at 04:10:38PM -0300, Jason Gunthorpe wrote: > > > > On Thu, May 23, 2019 at 02:24:58PM -0400, Jerome Glisse wrote: > > > I can not take mmap_sem in range_register, the READ_ONCE is fine and > > > they are no race as we do take a reference on the hmm struct thus > > > > Of course there are use after free races with a READ_ONCE scheme, I > > shouldn't have to explain this. > > Well i can not think of anything again here the mm->hmm can not > change while driver is calling hmm_range_register() Oh of course! It is so confusing because the new code makes it look like it could be changing... > so if you want i can remove the READ_ONCE() this does not change > anything. Please just write it as: /* The caller must hold a valid struct hmm_mirror to call this api, * and a valid hmm_mirror guarantees mm->hmm is valid. */ range->hmm = mm->hmm; kref_get(&range->hmm->kref); All the READ_ONCE/kref_get_not_zero/!hmm just obfuscates the reality that hmm is non-NULL and constant here or the caller is using the API wrong. kref_get will reliably oops or WARN_ON if it is misused which is a fine amount of debugging. Jason
