On 9/6/22 07:44, David Hildenbrand wrote: >> Though it is all very unlikely, the general memory model standard is >> to annotate with READ_ONCE. > > The only thing I could see going wrong in the comparison once the stars > alingn would be something like the following: > > if (*a != b) > > implemented as > > if ((*a).lower != b.lower && (*a).higher != b.higher) > > > This could only go wrong if we have more than one change such that: > > Original: > > *a = 0x00000000ffffffffull; > > > First modification: > *a = 0xffffffffffffffffull; > > Second modification: > *a = 0x00000000eeeeeeeeull; > > > If we race with both modifications, we could see that ffffffff matches, > and could see that 00000000 matches as well. > > > So I agree that we should change it, but not necessarily as an urgent > fix and not necessarily in this patch. It's best to adjust all gup_* > functions in one patch. > We had a long thread with Paul McKenney back in May [1] about this exact sort of problem. In that thread, I recall that "someone" tried to claim that a bare one-byte read was safe, and even that innocent-sounding claim got basically torn apart! :) Because the kernel memory model simply does not cover you for bare reads and writes to shared mutable memory. Unfortunately, until now, I'd only really remembered the conclusion: "use READ_ONCE() and WRITE_ONCE() for any touching of shared mutable memory", and not the point about other memory barriers not covering this aspect. Thanks to Jason for reminding us of this. This time I think I will remember it well enough to avoid another long thread. Maybe. [1] https://lore.kernel.org/all/20220524163728.GO1790663@paulmck-ThinkPad-P17-Gen-1/ thanks, -- John Hubbard NVIDIA
