Re: [PATCH] mm: prevent mmap_cache race in find_vma()

[Paul E. McKenney]

On Thu, Apr 04, 2013 at 12:01:52PM -0700, Hugh Dickins wrote:
> On Thu, 4 Apr 2013, Linus Torvalds wrote:
> > On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@xxxxxxxxxx> wrote:
> > >
> > > find_vma() can be called by multiple threads with read lock
> > > held on mm->mmap_sem and any of them can update mm->mmap_cache.
> > > Prevent compiler from re-fetching mm->mmap_cache, because other
> > > readers could update it in the meantime:
> > 
> > Ack. I do wonder if we should mark the unlocked update too some way
> > (also in find_vma()), although it's probably not a problem in practice
> > since there's no way the compiler can reasonably really do anything
> > odd with it. We *could* make that an ACCESS_ONCE() write too just to
> > highlight the fact that it's an unlocked write to this optimistic data
> > structure.
> 
> Hah, you beat me to it.
> 
> I wanted to get Jan's patch in first, seeing as it actually fixes his
> observed issue; and it is very nice to have such a good description of
> one of those, when ACCESS_ONCE() is usually just an insurance policy.
> 
> But then I was researching the much rarer "ACCESS_ONCE(x) = y" usage
> (popular in drivers/net/wireless/ath/ath9k and kernel/rcutree* and
> sound/firewire, but few places else).
> 
> When Paul reminded us of it yesterday, I came to wonder if actually
> every use of ACCESS_ONCE in the read form should strictly be matched
> by ACCESS_ONCE whenever modifying the location.

>From a hygiene/insurance/documentation point of view, I agree.  Of course,
it is OK to use things like cmpxchg() in place of ACCESS_ONCE().

The possible exceptions that come to mind are (1) if the access in
question is done holding a lock that excludes all other accesses to that
location, (2) if the access in question happens during initialization
before any other CPU has access to that location, and (3) if the access
in question happens during cleanup after all other CPUs have lost access
to that location.  Any others?

/me goes to look to see if the RCU code follows this good advice...

							Thanx, Paul

> My uneducated guess is that strictly it ought to, in the sense of
> insurance policy; but that (apart from that strange split writing
> issue which came up a couple of months ago) in practice our compilers
> have not "advanced" to the point of making this an issue yet.
> 
> > 
> > Anyway, applied.
> 
> Thanks,
> Hugh
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/