On 08.12.22 20:46, Peter Xu wrote:
This patch is a cleanup to always wr-protect pte/pmd in mkuffd_wp paths.
The reasons I still think this patch is worthwhile, are:
(1) It is a cleanup already; diffstat tells.
(2) It just feels natural after I thought about this, if the pte is uffd
protected, let's remove the write bit no matter what it was.
(2) Since x86 is the only arch that supports uffd-wp, it also redefines
pte|pmd_mkuffd_wp() in that it should always contain removals of
write bits. It means any future arch that want to implement uffd-wp
should naturally follow this rule too. It's good to make it a
default, even if with vm_page_prot changes on VM_UFFD_WP.
(3) It covers more than vm_page_prot. So no chance of any potential
future "accident" (like pte_mkdirty() sparc64 or loongarch, even
though it just got its pte_mkdirty fixed <1 month ago). It'll be
fairly clear when reading the code too that we don't worry anything
before a pte_mkuffd_wp() on uncertainty of the write bit.
Don't necessarily agree with (3). If you'd have a broken pte_mkdirty()
and do the pte_mkdirty() after pte_mkuffd_wp() it would still be broken.
Because sparc64 and loongarch are simply broken.
We may call pte_wrprotect() one more time in some paths (e.g. thp split),
but that should be fully local bitop instruction so the overhead should be
negligible.
Although this patch should logically also fix all the known issues on
uffd-wp too recently on either page migration or numa balancing, but this
is not the plan for that fix. So no fixes, and stable doesn't need this.
I don't see how this would fix do_numa_page(), where we only do a
pte_modify().
Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
---
Note: this patch should be able to apply cleanly with/without the other
mm/migrate patch, or David's vm_page_prot changes.
---
arch/x86/include/asm/pgtable.h | 24 ++++++++++++------------
include/asm-generic/hugetlb.h | 16 ++++++++--------
mm/huge_memory.c | 8 +++-----
mm/hugetlb.c | 4 ++--
mm/memory.c | 8 +++-----
mm/mprotect.c | 6 ++----
mm/userfaultfd.c | 18 ++----------------
7 files changed, 32 insertions(+), 52 deletions(-)
It's certainly a cleanup, even though we might unnecessarily wrprotect
(I don't think we care).
--
Thanks,
David / dhildenb
