On Fri, Aug 27, 2010 at 1:56 PM, Christoph Lameter <cl@xxxxxxxxx> wrote: > On Fri, 27 Aug 2010, Hugh Dickins wrote: > >> Nothing ensures that the root pointer was not changed after the >> ACCESS_ONCE, that's exactly why we use ACCESS_ONCE there: once we've >> got the lock and realize that what we've locked may not be what we >> wanted (or may change from what we were wanting at any moment, the >> page no longer being mapped there - but in that case we no longer want >> it), we have to be sure to unlock the one we locked, rather than the >> one which anon_vma->root might subsequently point to. > > I do not see any check after we have taken the lock to verify that we > locked the correct object. Was there a second version of the patch? No second version of the patch, no. As I said already, it's that second page_mapped check which gives the guarantee that the anon_vma has not yet been freed, hence we've locked the correct object. > >> > Since there is no lock taken before the mapped check none of the >> > earlier reads from the anon vma structure nor the page mapped check >> > necessarily reflect a single state of the anon_vma. >> >> There's no lock (other than RCU's read "lock") taken before the >> original mapped check, and that's important, otherwise our attempt to >> lock might actually spinon or corrupt something that was long ago an >> anon_vma. But we do take the anon_vma->root->lock before the second >> mapped check which I added. If the page is still mapped at the point > > You then are using an object from the anon_vma (the pointer) without a > lock! Yes. (not counting RCU's read "lock" as a lock). > This is unstable therefore unless there are other constraints. The > anon_vma->lock must be taken before derefencing that pointer. No, SLAB_DESTROY_BY_RCU gives us just the stablity we need to take the lock