Re: [PATCH v7 3/4] fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs

On Mon, Jan 09, 2023 at 11:45:18AM +0500, Muhammad Usama Anjum wrote:

This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear the info about page table entries. The following operations are supported in this ioctl:

• Get the information if the pages have been written-to (PAGE_IS_Wt), file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped (PAGE_IS_SWAPPED).
• Write-protect the pages (PAGEMAP_WP_ENGAGE) to start finding which pages have been written-to.
• Find pages which have been written-to and write protect the pages (atomic PAGE_IS_NOT_WP + PAGEMAP_WP_ENGAGE)

The uffd should have been registered on the memory range before performing any WP/WT (Write Protect/Writtern-To) related operations with the IOCTL.

struct pagemap_scan_args is used as the argument of the IOCTL. In this struct:

• The range is specified through start and len.
• The output buffer and size is specified as vec and vec_len.
• The optional maximum requested pages are specified in the max_pages.
• The flags can be specified in the flags field. The PAGEMAP_WP_ENGAGE is the only added flag at this time.
• The masks are specified in required_mask, anyof_mask, excluded_ mask and return_mask.

This IOCTL can be extended to get information about more PTE bits. This patch has evolved from a basic patch from Gabriel Krisman Bertazi.

Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com

Changes in v7:

• Rebase on top of latest next
• Fix some corner cases
• Base soft-dirty on the uffd wp async
• Update the terminologies
• Optimize the memory usage inside the ioctl

Changes in v6:

• Rename variables and update comments
• Make IOCTL independent of soft_dirty config
• Change masks and bitmap type to _u64
• Improve code quality

Changes in v5:

• Remove tlb flushing even for clear operation

Changes in v4:

• Update the interface and implementation

Changes in v3:

• Tighten the user-kernel interface by using explicit types and add more error checking

Changes in v2:

• Convert the interface from syscall to ioctl
• Remove pidfd support as it doesn't make sense in ioctl

---

fs/proc/task_mmu.c | 300 ++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 50 ++++++ tools/include/uapi/linux/fs.h | 50 ++++++ 3 files changed, 400 insertions(+)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index e35a0398db63..ba70faadf403 100644 --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -19,6 +19,7 @@ #include <linux/shmem_fs.h> #include <linux/uaccess.h> #include <linux/pkeys.h> +#include <linux/minmax.h> #include <asm/elf.h> #include <asm/tlb.h> @@ -1135,6 +1136,22 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma, } #endif +static inline bool is_pte_uffd_wp(pte_t pte) +{

• if ((pte_present(pte) && pte_uffd_wp(pte)) ||

•    (is_swap_pte(pte) && pte_swp_uffd_wp(pte)))
  

• return true;
  

This is an interesting way to detect whether the page is written.. I would have thought you would reuse soft-dirty bit.

For swap case, you missed the pte markers (which can exist for !anon memory). You can have a look at pte_swp_uffd_wp_any().

• return false;

+}

+static inline bool is_pmd_uffd_wp(pmd_t pmd) +{

• if ((pmd_present(pmd) && pmd_uffd_wp(pmd)) ||

•    (is_swap_pmd(pmd) && pmd_swp_uffd_wp(pmd)))
  

• return true;
  

• return false;

+}

#if defined(CONFIG_MEM_SOFT_DIRTY) && defined(CONFIG_TRANSPARENT_HUGEPAGE) static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma, unsigned long addr, pmd_t *pmdp) @@ -1763,11 +1780,294 @@ static int pagemap_release(struct inode *inode, struct file *file) return 0; } +#define PAGEMAP_OP_MASK (PAGE_IS_WT | PAGE_IS_FILE | \

• 		 PAGE_IS_PRESENT | PAGE_IS_SWAPPED)
  

+#define PAGEMAP_NONWT_OP_MASK (PAGE_IS_FILE | PAGE_IS_PRESENT | PAGE_IS_SWAPPED) +#define IS_WP_ENGAGE_OP(a) (a->flags & PAGEMAP_WP_ENGAGE) +#define IS_GET_OP(a) (a->vec) +#define PAGEMAP_SCAN_BITMAP(wt, file, present, swap) \

• (wt | file << 1 | present << 2 | swap << 3)

+#define IS_WT_REQUIRED(a) \

• ((a->required_mask & PAGE_IS_WT) || \

• (a->anyof_mask & PAGE_IS_WT))
  

+struct pagemap_scan_private {

• struct page_region *vec;
• struct page_region prev;
• unsigned long vec_len, vec_index;
• unsigned int max_pages, found_pages, flags;
• unsigned long required_mask, anyof_mask, excluded_mask, return_mask;

+};

+static int pagemap_scan_test_walk(unsigned long start, unsigned long end, struct mm_walk *walk) +{

• struct pagemap_scan_private *p = walk->private;
• struct vm_area_struct *vma = walk->vma;
• if (IS_WT_REQUIRED(p) && !vma_can_userfault(vma, vma->vm_flags))

• return -EPERM;
  

vma_can_userfault() is too coarse. Maybe what you wanted to check is userfaultfd_wp(vma)?

• if (IS_GET_OP(p) && p->max_pages && (p->found_pages == p->max_pages))

• return -ENOSPC;
  

This is the function to test "whether the walker should walk the vma specified". This check should IIUC be meaningless because found_pages doesn't boost during vma switching, while OTOH your pmd walker fn should do proper check when increasing found_pages and return -ENOSPC properly when the same condition met. That should be enough, IMHO.

I saw it already a few times in this patch, I think maybe you want a helper for this one taking *p as argument.

• if (vma->vm_flags & VM_PFNMAP)

• return 1;
  

• return 0;

+}

+static inline int add_to_out(bool wt, bool file, bool pres, bool swap,

• 	     struct pagemap_scan_private *p, unsigned long addr, unsigned int len)
  

+{

• unsigned long bitmap, cur = PAGEMAP_SCAN_BITMAP(wt, file, pres, swap);
• bool cpy = true;
• struct page_region *prev = &p->prev;
• if (p->required_mask)

• cpy = ((p->required_mask & cur) == p->required_mask);
  

• if (cpy && p->anyof_mask)

• cpy = (p->anyof_mask & cur);
  

• if (cpy && p->excluded_mask)

• cpy = !(p->excluded_mask & cur);
  

• bitmap = cur & p->return_mask;
• if (cpy && bitmap) {

• if ((prev->len) && (prev->bitmap == bitmap) &&
  

•     (prev->start + prev->len * PAGE_SIZE == addr)) {
  

• 	prev->len += len;
  

• 	p->found_pages += len;
  

• } else if (p->vec_index < p->vec_len) {
  

• 	if (prev->len) {
  

• 		memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region));
  

• 		p->vec_index++;
  

• 	}
  

• 	prev->start = addr;
  

• 	prev->len = len;
  

• 	prev->bitmap = bitmap;
  

• 	p->found_pages += len;
  

• } else {
  

• 	return -ENOSPC;
  

• }
  

[1]

• }
• return 0;

+}

+static inline int export_prev_to_out(struct pagemap_scan_private *p, struct page_region __user *vec,

• 		     unsigned long *vec_index)
  

+{

• struct page_region *prev = &p->prev;
• if (prev->len) {

• if (copy_to_user(&vec[*vec_index], prev, sizeof(struct page_region)))
  

• 	return -EFAULT;
  

• p->vec_index++;
  

• (*vec_index)++;
  

• prev->len = 0;
  

• }
• return 0;

+}

I'd rather not have this helper; it doesn't really help a lot.

+static inline int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long addr,

• 			 unsigned long end, struct mm_walk *walk)
  

+{

• struct pagemap_scan_private *p = walk->private;
• struct vm_area_struct *vma = walk->vma;
• unsigned long start = addr;
• unsigned int len;
• spinlock_t *ptl;
• int ret = 0;
• pte_t *pte;
• bool pmd_wt;
• if ((walk->vma->vm_end < addr) || (p->max_pages && p->found_pages == p->max_pages))

• return 0;
  

• end = min(end, walk->vma->vm_end);

None of the check above is needed, I think..

vma ranges are checked before pmd_entry() called. found_pages should be checked when increased (I think it's [1] above).

+#ifdef CONFIG_TRANSPARENT_HUGEPAGE

• ptl = pmd_trans_huge_lock(pmd, vma);
• if (ptl) {

• pmd_wt = !is_pmd_uffd_wp(*pmd);
  

• /*
  

•  * Break huge page into small pages if operation needs to be performed is
  

•  * on a portion of the huge page or the return buffer cannot store complete
  

•  * data.
  

•  */
  

• if (pmd_wt && (IS_WP_ENGAGE_OP(p) && (end - addr < HPAGE_SIZE))) {
  

• 	spin_unlock(ptl);
  

• 	split_huge_pmd(vma, pmd, addr);
  

• 	goto process_smaller_pages;
  

• }
  

• if (IS_GET_OP(p)) {
  

• 	len = (end - addr)/PAGE_SIZE;
  

• 	if (p->max_pages && p->found_pages + len > p->max_pages)
  

• 		len = p->max_pages - p->found_pages;
  

• 	ret = add_to_out(pmd_wt, vma->vm_file, pmd_present(*pmd),
  

• 			 is_swap_pmd(*pmd), p, addr, len);
  

• 	if (ret) {
  

• 		spin_unlock(ptl);
  

• 		return ret;
  

• 	}
  

• }
  

• spin_unlock(ptl);
  

• if (IS_WP_ENGAGE_OP(p) && pmd_wt) {
  

• 	BUG_ON(start & ~HPAGE_MASK);
  

• 	ret = wp_range_async(vma, addr, HPAGE_SIZE);
  

• }
  

• return ret;
  

• }

+process_smaller_pages:

• if (pmd_trans_unstable(pmd))

• return 0;
  

+#endif /* CONFIG_TRANSPARENT_HUGEPAGE */

• pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
• for (; addr < end && !ret && (!p->max_pages || (p->found_pages < p->max_pages))

•     ; pte++, addr += PAGE_SIZE) {
  

• if (IS_GET_OP(p)) {
  

This 'if' should be out - skip loop if not needed.

• 	ret = add_to_out(!is_pte_uffd_wp(*pte), vma->vm_file, pte_present(*pte),
  

• 			 is_swap_pte(*pte), p, addr, 1);
  

• 	if (ret)
  

• 		break;
  

• }
  

• }
• pte_unmap_unlock(pte - 1, ptl);
• if (IS_WP_ENGAGE_OP(p)) {

• BUG_ON(start & ~PAGE_MASK);
  

• ret = wp_range_async(vma, start, addr - start);
  

• }
• cond_resched();
• return ret;

+}

+static int pagemap_scan_pte_hole(unsigned long addr, unsigned long end, int depth,

• 		 struct mm_walk *walk)
  

+{

• struct pagemap_scan_private *p = walk->private;
• struct vm_area_struct *vma = walk->vma;
• unsigned int len;
• int ret = 0;
• if (vma) {

• len = (end - addr)/PAGE_SIZE;
  

• if (p->max_pages && p->found_pages + len > p->max_pages)
  

• 	len = p->max_pages - p->found_pages;
  

• ret = add_to_out(false, vma->vm_file, false, false, p, addr, len);
  

• }
• return ret;

+}

+static const struct mm_walk_ops pagemap_scan_ops = {

• .test_walk = pagemap_scan_test_walk,
• .pmd_entry = pagemap_scan_pmd_entry,

Do we care about hugetlb at all? So far it seems you don't. It's fine, then if you decided to go the uffd-wp way you can explicit declare no support on hugetlb, as uffd-wp sync supports it so it should be by default supported otherwise.

• .pte_hole = pagemap_scan_pte_hole,

+};