Re: [rfc][patch] mm: dirty page accounting race fix

Peter Zijlstra
2008-08-14T12:49:03Z

On Thu, 2008-08-14 at 12:55 +0100, Hugh Dickins wrote:

> Am I confused, or is your "do_wp_page calls ptep_clear_flush_notify"
> example a very bad one? The page it's dealing with there doesn't
> go back into the page table (its COW does), and the dirty_accounting
> case doesn't even get down there, it's dealt with in the reuse case
> above, which uses ptep_set_access_flags.

Also, the new page is only added to the rmap _after_ it has been
installed. So page_mkclean() will never get to it to see the empty pte.

> Now, I think that one may
> well behave as you suggest on some arches (though it's extending
> permissions not restricting them, so maybe not); but please check
> that out and improve your example.

Another case I just looked at is if ptep_clear_flush_young() actually
does the clear bit. But the few arches (x86_64, ppc64) that I looked at
don't seem to do so.

If someone would, you could hit this race.

/me continues searching for a convincing candidate..

> Even if it does, it's not clear to me that your fix is the answer.
> That may well be because the whole of dirty page accounting grew too
> subtle for me!

CPU1 CPU2

lock(pte_lock)
ptep_clear_flush(ptep)
page_mkclean()
page_check_address()
!pte_present(ptep)
return NULL
ptep_set(ptep, new_pte);
unlock(pte_lock)

Now, if page_check_address() doesn't return prematurely, but is forced
to take the pte_lock, we won't see that hole and will not skip the page.

> But holding the page table lock on one pte of the
> page doesn't guarantee much about the integrity of the whole dance:
> do_wp_page does its set_page_dirty_balance for this case, you'd
> need to spell out the bad sequence more to convince me.

Now you're confusing me... are you saying ptes can be changed from under
your feet even while holding the pte_lock?