Hugh Dickins wrote:
Zach, while looking at your recent patches, I ran across the comment
on pte_update_defer, and where it was being used, and now think that
David's patch is actually incorrect. Previously pte_update_defer
was being used where a flush_tlb_page followed immediately after
within the same macro; with David's patch, mm's clear_refs_pte_range
is calling ptep_test_and_clear_young (including pte_update_defer) on
several ptes, then unlocking the page table, and later flushing TLB.
That's exactly wrong for pte_update_defer, isn't it?
Ok, disregard most of my last e-mail. It is fine to decouple the flush
from the update, as long as they stay close enough that you can reason
they happen together. I guess I hadn't seen the other parts of the
patch which release the page table spinlock in between the two, and
somehow missed it again when responding to the above as I got too
excited explaining why the decoupling is ok. It is not ok to release
the spinlock when using shadow page tables on SMP. There are some
rather complex races that can result. Here's one case:
CPU-0 CPU-1
----------------------- ---------------------------
test_and_clear_dirty(x)
spin_unlock(ptl)
write address mapped by X
(harware updates dirty bit)
spin_lock(ptl)
set_pte_wrprotect(x)
flush
flush
Now, the write protected pte which maps a dirty page gets broken in two
ways; it is unclear if dirty bit or entiry PTE from CPU-0 is deferred
until flush, so either write protected PTE for modified page loses the
dirty bit (BAD!), or write protected PTE loses both dirty and write
protect bits (VERY BAD!).
To prevent this, we need a flush before dropping the spinlock. If that
gets too complicated, we can drop the defer logic and just use
pte_update instead, which notifies the hypervisor immediately of the
mapping change.
Zach
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
