On Thu, Feb 02, 2023 at 04:29:10PM +0500, Muhammad Usama Anjum wrote:

Add new WP Async mode (UFFD_FEATURE_WP_ASYNC) which resolves the page faults on its own. It can be used to track that which pages have been written-to from the time the pages were write-protected. It is very efficient way to track the changes as uffd is by nature pte/pmd based.

UFFD synchronous WP sends the page faults to the userspace where the pages which have been written-to can be tracked. But it is not efficient. This is why this asynchronous version is being added. After setting the WP Async, the pages which have been written to can be found in the pagemap file or information can be obtained from the PAGEMAP_IOCTL.

Suggested-by: Peter Xu peterx@redhat.com Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com

---

Changes in v10:

• Build fix
• Update comments and add error condition to return error from uffd register if hugetlb pages are present when wp async flag is set

Changes in v9:

• Correct the fault resolution with code contributed by Peter

Changes in v7:

• Remove UFFDIO_WRITEPROTECT_MODE_ASYNC_WP and add UFFD_FEATURE_WP_ASYNC
• Handle automatic page fault resolution in better way (thanks to Peter)

update to wp async

uffd wp async

fs/userfaultfd.c | 20 ++++++++++++++++++-- include/linux/userfaultfd_k.h | 11 +++++++++++ include/uapi/linux/userfaultfd.h | 10 +++++++++- mm/memory.c | 23 ++++++++++++++++++++--- 4 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 15a5bf765d43..422f2530c63e 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -1422,10 +1422,15 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, goto out_unlock; /*

•  * Note vmas containing huge pages
  

•  * Note vmas containing huge pages. Hugetlb isn't supported
  

•  * with UFFD_FEATURE_WP_ASYNC.
  

  */

Need to set "ret = -EINVAL;" here. Or..

• if (is_vm_hugetlb_page(cur))
  

• if (is_vm_hugetlb_page(cur)) {
  

• 	if (ctx->features & UFFD_FEATURE_WP_ASYNC)
  

• 		goto out_unlock;
  

.. it'll return -EBUSY, which does not sound like the right errcode here.

Drop this empty line?

	basic_ioctls = true;

• }
  

found = true; }

Other than that looks good, thanks.

Hi Muhammad,

On Thu, Feb 02, 2023 at 04:29:10PM +0500, Muhammad Usama Anjum wrote:

Add new WP Async mode (UFFD_FEATURE_WP_ASYNC) which resolves the page faults on its own. It can be used to track that which pages have been written-to from the time the pages were write-protected. It is very efficient way to track the changes as uffd is by nature pte/pmd based.

UFFD synchronous WP sends the page faults to the userspace where the pages which have been written-to can be tracked. But it is not efficient. This is why this asynchronous version is being added. After setting the WP Async, the pages which have been written to can be found in the pagemap file or information can be obtained from the PAGEMAP_IOCTL.

Suggested-by: Peter Xu peterx@redhat.com Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com

---

Changes in v10:

• Build fix
• Update comments and add error condition to return error from uffd register if hugetlb pages are present when wp async flag is set

Changes in v9:

• Correct the fault resolution with code contributed by Peter

Changes in v7:

• Remove UFFDIO_WRITEPROTECT_MODE_ASYNC_WP and add UFFD_FEATURE_WP_ASYNC
• Handle automatic page fault resolution in better way (thanks to Peter)

update to wp async

uffd wp async

fs/userfaultfd.c | 20 ++++++++++++++++++-- include/linux/userfaultfd_k.h | 11 +++++++++++ include/uapi/linux/userfaultfd.h | 10 +++++++++- mm/memory.c | 23 ++++++++++++++++++++--- 4 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 15a5bf765d43..422f2530c63e 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -1422,10 +1422,15 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, goto out_unlock; /*

•  * Note vmas containing huge pages
  

•  * Note vmas containing huge pages. Hugetlb isn't supported
  

•  * with UFFD_FEATURE_WP_ASYNC.
  

  */

• if (is_vm_hugetlb_page(cur))
  

• if (is_vm_hugetlb_page(cur)) {
  

• 	if (ctx->features & UFFD_FEATURE_WP_ASYNC)
  

• 		goto out_unlock;
  

• basic_ioctls = true;
  

• }
  

found = true; } @@ -1867,6 +1872,10 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx, mode_wp = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP; mode_dontwake = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_DONTWAKE;

• /* The unprotection is not supported if in async WP mode */
• if (!mode_wp && (ctx->features & UFFD_FEATURE_WP_ASYNC))

• return -EINVAL;
  

• if (mode_wp && mode_dontwake) return -EINVAL;

@@ -1950,6 +1959,13 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg) return ret; } +int userfaultfd_wp_async(struct vm_area_struct *vma) +{

• struct userfaultfd_ctx *ctx = vma->vm_userfaultfd_ctx.ctx;
• return (ctx && (ctx->features & UFFD_FEATURE_WP_ASYNC));

+}

static inline unsigned int uffd_ctx_features(__u64 user_features) { /* diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h index 9df0b9a762cc..38c92c2beb16 100644 --- a/include/linux/userfaultfd_k.h +++ b/include/linux/userfaultfd_k.h @@ -179,6 +179,7 @@ extern int userfaultfd_unmap_prep(struct mm_struct *mm, unsigned long start, unsigned long end, struct list_head *uf); extern void userfaultfd_unmap_complete(struct mm_struct *mm, struct list_head *uf); +extern int userfaultfd_wp_async(struct vm_area_struct *vma); #else /* CONFIG_USERFAULTFD */ @@ -189,6 +190,11 @@ static inline vm_fault_t handle_userfault(struct vm_fault *vmf, return VM_FAULT_SIGBUS; } +static inline void uffd_wp_range(struct mm_struct *dst_mm, struct vm_area_struct *vma,

• 		 unsigned long start, unsigned long len, bool enable_wp)
  

+{ +}

static inline bool is_mergeable_vm_userfaultfd_ctx(struct vm_area_struct *vma, struct vm_userfaultfd_ctx vm_ctx) { @@ -274,6 +280,11 @@ static inline bool uffd_disable_fault_around(struct vm_area_struct *vma) return false; } +static inline int userfaultfd_wp_async(struct vm_area_struct *vma) +{

• return false;

+}

#endif /* CONFIG_USERFAULTFD */ static inline bool pte_marker_entry_uffd_wp(swp_entry_t entry) diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h index 005e5e306266..30a6f32cf564 100644 --- a/include/uapi/linux/userfaultfd.h +++ b/include/uapi/linux/userfaultfd.h @@ -38,7 +38,8 @@ UFFD_FEATURE_MINOR_HUGETLBFS | \ UFFD_FEATURE_MINOR_SHMEM | \ UFFD_FEATURE_EXACT_ADDRESS | \

• 	   UFFD_FEATURE_WP_HUGETLBFS_SHMEM)
  

• 	   UFFD_FEATURE_WP_HUGETLBFS_SHMEM |	\
  

• 	   UFFD_FEATURE_WP_ASYNC)
  

#define UFFD_API_IOCTLS \ ((__u64)1 << _UFFDIO_REGISTER | \ (__u64)1 << _UFFDIO_UNREGISTER | \ @@ -203,6 +204,12 @@ struct uffdio_api { * * UFFD_FEATURE_WP_HUGETLBFS_SHMEM indicates that userfaultfd * write-protection mode is supported on both shmem and hugetlbfs.

• *
  

• * UFFD_FEATURE_WP_ASYNC indicates that userfaultfd write-protection
  

• * asynchronous mode is supported in which the write fault is automatically
  

• * resolved and write-protection is un-set. It only supports anon and shmem
  

• * (hugetlb isn't supported). It only takes effect when a vma is registered
  

• * with write-protection mode. Otherwise the flag is ignored.
  

  */

Most of mm/ adheres the 80-character limits. Please make your changes to follow it as well.

#define UFFD_FEATURE_PAGEFAULT_FLAG_WP (1<<0) #define UFFD_FEATURE_EVENT_FORK (1<<1) @@ -217,6 +224,7 @@ struct uffdio_api { #define UFFD_FEATURE_MINOR_SHMEM (1<<10) #define UFFD_FEATURE_EXACT_ADDRESS (1<<11) #define UFFD_FEATURE_WP_HUGETLBFS_SHMEM (1<<12) +#define UFFD_FEATURE_WP_ASYNC (1<<13) __u64 features; __u64 ioctls; diff --git a/mm/memory.c b/mm/memory.c index 4000e9f017e0..75331fbf7cb4 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3351,8 +3351,21 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf) if (likely(!unshare)) { if (userfaultfd_pte_wp(vma, *vmf->pte)) {

• 	pte_unmap_unlock(vmf->pte, vmf->ptl);
  

• 	return handle_userfault(vmf, VM_UFFD_WP);
  

• 	if (userfaultfd_wp_async(vma)) {
  

• 		/*
  

• 		 * Nothing needed (cache flush, TLB invalidations,
  

• 		 * etc.) because we're only removing the uffd-wp bit,
  

• 		 * which is completely invisible to the user.
  

• 		 */
  

• 		pte_t pte = pte_clear_uffd_wp(*vmf->pte);
  

• 		set_pte_at(vma->vm_mm, vmf->address, vmf->pte, pte);
  

• 		/* Update this to be prepared for following up CoW handling */
  

• 		vmf->orig_pte = pte;
  

• 	} else {
  

• 		pte_unmap_unlock(vmf->pte, vmf->ptl);
  

• 		return handle_userfault(vmf, VM_UFFD_WP);
  

• 	}
  

You can revert the condition here and reduce the nesting:

if (!userfaultfd_wp_async(vma)) { pte_unmap_unlock(vmf->pte, vmf->ptl); return handle_userfault(vmf, VM_UFFD_WP); }

/* handle async WP */

}

/* @@ -4812,8 +4825,11 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf) if (vma_is_anonymous(vmf->vma)) { if (likely(!unshare) &&

•     userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd))
  

•     userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd)) {
  

• 	if (userfaultfd_wp_async(vmf->vma))
  

• 		goto split;
  return handle_userfault(vmf, VM_UFFD_WP);
  

• }
  

  return do_huge_pmd_wp_page(vmf); }

@@ -4825,6 +4841,7 @@ static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf) } } +split: /* COW or write-notify handled on pte level: split pmd. */ __split_huge_pmd(vmf->vma, vmf->pmd, vmf->address, false, NULL); -- 2.30.2

On 2/9/23 2:31 AM, Peter Xu wrote:

On Thu, Feb 02, 2023 at 04:29:11PM +0500, Muhammad Usama Anjum wrote:

...

Explain the difference created by UFFD_FEATURE_WP_ASYNC to the write protection (UFFDIO_WRITEPROTECT_MODE_WP) mode.

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

Documentation/admin-guide/mm/userfaultfd.rst | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/Documentation/admin-guide/mm/userfaultfd.rst b/Documentation/admin-guide/mm/userfaultfd.rst index 83f31919ebb3..4747e7bd5b26 100644 --- a/Documentation/admin-guide/mm/userfaultfd.rst +++ b/Documentation/admin-guide/mm/userfaultfd.rst @@ -221,6 +221,13 @@ former will have ``UFFD_PAGEFAULT_FLAG_WP`` set, the latter you still need to supply a page when ``UFFDIO_REGISTER_MODE_MISSING`` was used. +If ``UFFD_FEATURE_WP_ASYNC`` is set while calling ``UFFDIO_API`` ioctl, the +behaviour of ``UFFDIO_WRITEPROTECT_MODE_WP`` changes such that faults for

UFFDIO_WRITEPROTECT_MODE_WP is only a flag in UFFDIO_WRITEPROTECT, while it's forbidden only when not specified.

...

+anon and shmem are resolved automatically by the kernel instead of sending +the message to the userfaultfd. The hugetlb isn't supported. The ``pagemap`` +file can be read to find which pages have ``PM_UFFD_WP`` flag set which +means they are write-protected.

Here's my version. Please feel free to do modifications on top.

If the userfaultfd context (that has ``UFFDIO_REGISTER_MODE_WP`` registered against) has ``UFFD_FEATURE_WP_ASYNC`` feature enabled, it will work in async write protection mode. It can be seen as a more accurate version of soft-dirty tracking, meanwhile the results will not be easily affected by other operations like vma merging.

Comparing to the generic mode, the async mode will not generate any userfaultfd message when the protected memory range is written. Instead, the kernel will automatically resolve the page fault immediately by dropping the uffd-wp bit in the pgtables. The user app can collect the "written/dirty" status by looking up the uffd-wp bit for the pages being interested in /proc/pagemap.

The page will be under track of uffd-wp async mode until the page is explicitly write-protected by ``UFFDIO_WRITEPROTECT`` ioctl with the mode flag ``UFFDIO_WRITEPROTECT_MODE_WP`` set. Trying to resolve a page fault that was tracked by async mode userfaultfd-wp is invalid.

Currently ``UFFD_FEATURE_WP_ASYNC`` only support anonymous and shmem. Hugetlb is not yet supported.

It'll get replaced the documentation. I'll add a suggested by tag as well. Thanks.

On Thu, Feb 02, 2023 at 04:29:12PM +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_WRITTEN), 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_WRITTEN + PAGEMAP_WP_ENGAGE)

To get information about which pages have been written-to and/or write protect the pages, following must be performed first in order:

• The userfaultfd file descriptor is created with userfaultfd syscall.
• The UFFD_FEATURE_WP_ASYNC feature is set by UFFDIO_API IOCTL.
• The memory range is registered with UFFDIO_REGISTER_MODE_WP mode through UFFDIO_REGISTER IOCTL.

Then the any part of the registered memory or the whole memory region can be write protected using the UFFDIO_WRITEPROTECT IOCTL or PAGEMAP_SCAN 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 of struct page_region array 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 IOCTL doesn't support hugetlbs at the moment. No information about hugetlb can be obtained. This patch has evolved from a basic patch from Gabriel Krisman Bertazi.

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

Changes in v10:

• move changes in tools/include/uapi/linux/fs.h to separate patch
• update commit message

Change in v8:

• Correct is_pte_uffd_wp()
• Improve readability and error checks
• Remove some un-needed code

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 | 290 ++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 50 +++++++ 2 files changed, 340 insertions(+)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index e35a0398db63..c6bde19d63d9 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)) ||

•    (pte_swp_uffd_wp_any(pte)))
  

• return true;
  

• return false;

Sorry I should have mentioned this earlier: you can directly return here.

return (pte_present(pte) && pte_uffd_wp(pte)) || pte_swp_uffd_wp_any(pte);

+}

+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;

Same here.

+}

#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,284 @@ static int pagemap_release(struct inode *inode, struct file *file) return 0; } +#define PAGEMAP_BITS_ALL (PAGE_IS_WRITTEN | PAGE_IS_FILE | \

• 			 PAGE_IS_PRESENT | PAGE_IS_SWAPPED)
  

+#define PAGEMAP_NON_WRITTEN_BITS (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 HAS_NO_SPACE(p) (p->max_pages && (p->found_pages == p->max_pages))

+#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_WRITTEN) || \

• (a->anyof_mask & PAGE_IS_WRITTEN))
  

+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) && !userfaultfd_wp(vma) && !userfaultfd_wp_async(vma))

Should this be:

(IS_WT_REQUIRED(p) && (!userfaultfd_wp(vma) || !userfaultfd_wp_async(vma)))

Instead?

• return -EPERM;
  

• if (vma->vm_flags & VM_PFNMAP)

• return 1;
  

• return 0;

+}

+static inline int pagemap_scan_output(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;

Nit: switch the above two lines?

• if (HAS_NO_SPACE(p))

• return -ENOSPC;
  

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

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

If "p->found_pages + len >= p->max_pages", shouldn't this already return -ENOSPC?

• if (!len)

• return -EINVAL;
  

• 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++;
  

• 	}
  

IIUC you can have:

int pagemap_scan_deposit(p) { if (p->vec_index >= p->vec_len) return -ENOSPC;

if (p->prev->len) { memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region)); p->vec_index++; }

return 0; }

Then call it here. I think it can also be called below to replace export_prev_to_out().

• 	prev->start = addr;
  

• 	prev->len = len;
  

• 	prev->bitmap = bitmap;
  

• 	p->found_pages += len;
  

• } else {
  

• 	return -ENOSPC;
  

• }
  

• }
• 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;

+}

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

• 			 unsigned long end, struct mm_walk *walk)
  

+{

• struct pagemap_scan_private *p = walk->private;
• struct vm_area_struct *vma = walk->vma;
• unsigned long addr = end;

This assignment is useless?

• spinlock_t *ptl;
• int ret = 0;
• pte_t *pte;

+#ifdef CONFIG_TRANSPARENT_HUGEPAGE

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

• bool pmd_wt;
  

• 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.
  

•  */
  

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

• 	spin_unlock(ptl);
  

• 	split_huge_pmd(vma, pmd, start);
  

• 	goto process_smaller_pages;
  

• }
  

• if (IS_GET_OP(p))
  

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

• 				  is_swap_pmd(*pmd), p, start,
  

• 				  (end - start)/PAGE_SIZE);
  

• spin_unlock(ptl);
  

• if (!ret) {
  

• 	if (pmd_wt && IS_WP_ENGAGE_OP(p))
  

• 		uffd_wp_range(walk->mm, vma, start, HPAGE_SIZE, true);
  

• }
  

• 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, start, &ptl);
• if (IS_GET_OP(p)) {

• for (addr = start; addr < end; pte++, addr += PAGE_SIZE) {
  

• 	ret = pagemap_scan_output(!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 ((!ret || ret == -ENOSPC) && IS_WP_ENGAGE_OP(p) && (addr - start))

• uffd_wp_range(walk->mm, vma, start, addr - start, true);
  

• 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;
• int ret = 0;
• if (vma)

• ret = pagemap_scan_output(false, vma->vm_file, false, false, p, addr,
  

• 			  (end - addr)/PAGE_SIZE);
  

• return ret;

+}

+/* No hugetlb support is present. */ +static const struct mm_walk_ops pagemap_scan_ops = {

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

+};

+static long do_pagemap_cmd(struct mm_struct *mm, struct pagemap_scan_arg *arg) +{

• unsigned long empty_slots, vec_index = 0;
• unsigned long __user start, end;
• unsigned long __start, __end;
• struct page_region __user *vec;
• struct pagemap_scan_private p;
• int ret = 0;
• start = (unsigned long)untagged_addr(arg->start);
• vec = (struct page_region *)(unsigned long)untagged_addr(arg->vec);
• /* Validate memory ranges */
• if ((!IS_ALIGNED(start, PAGE_SIZE)) || (!access_ok((void __user *)start, arg->len)))

• return -EINVAL;
  

• if (IS_GET_OP(arg) && ((arg->vec_len == 0) ||

•    (!access_ok((void __user *)vec, arg->vec_len * sizeof(struct page_region)))))
  

• return -EINVAL;
  

• /* Detect illegal flags and masks */
• if ((arg->flags & ~PAGEMAP_WP_ENGAGE) || (arg->required_mask & ~PAGEMAP_BITS_ALL) ||

•    (arg->anyof_mask & ~PAGEMAP_BITS_ALL) || (arg->excluded_mask & ~PAGEMAP_BITS_ALL) ||
  

•    (arg->return_mask & ~PAGEMAP_BITS_ALL))
  

• return -EINVAL;
  

• if (IS_GET_OP(arg) && ((!arg->required_mask && !arg->anyof_mask && !arg->excluded_mask) ||

• 		!arg->return_mask))
  

• return -EINVAL;
  

• /* The non-WT flags cannot be obtained if PAGEMAP_WP_ENGAGE is also specified. */
• if (IS_WP_ENGAGE_OP(arg) && ((arg->required_mask & PAGEMAP_NON_WRITTEN_BITS) ||

•    (arg->anyof_mask & PAGEMAP_NON_WRITTEN_BITS)))
  

• return -EINVAL;
  

I think you said you'll clean this up a bit. I don't think so..

• end = start + arg->len;
• p.max_pages = arg->max_pages;
• p.found_pages = 0;
• p.flags = arg->flags;
• p.required_mask = arg->required_mask;
• p.anyof_mask = arg->anyof_mask;
• p.excluded_mask = arg->excluded_mask;
• p.return_mask = arg->return_mask;
• p.prev.len = 0;
• p.vec_len = (PAGEMAP_WALK_SIZE >> PAGE_SHIFT);
• if (IS_GET_OP(arg)) {

• p.vec = kmalloc_array(p.vec_len, sizeof(struct page_region), GFP_KERNEL);
  

• if (!p.vec)
  

• 	return -ENOMEM;
  

• } else {

• p.vec = NULL;
  

• }
• __start = __end = start;
• while (!ret && __end < end) {

• p.vec_index = 0;
  

• empty_slots = arg->vec_len - vec_index;
  

• if (p.vec_len > empty_slots)
  

• 	p.vec_len = empty_slots;
  

• __end = (__start + PAGEMAP_WALK_SIZE) & PAGEMAP_WALK_MASK;
  

• if (__end > end)
  

• 	__end = end;
  

• mmap_read_lock(mm);
  

• ret = walk_page_range(mm, __start, __end, &pagemap_scan_ops, &p);
  

• mmap_read_unlock(mm);
  

• if (!(!ret || ret == -ENOSPC))
  

• 	goto free_data;
  

• __start = __end;
  

• if (IS_GET_OP(arg) && p.vec_index) {
  

• 	if (copy_to_user(&vec[vec_index], p.vec,
  

• 			 p.vec_index * sizeof(struct page_region))) {
  

• 		ret = -EFAULT;
  

• 		goto free_data;
  

• 	}
  

• 	vec_index += p.vec_index;
  

• }
  

I think you can move copy_to_user() to outside the loop, then call pagemap_scan_deposit() before copy_to_user(), then I think you can drop the ugly export_prev_to_out()..

• }
• ret = export_prev_to_out(&p, vec, &vec_index);
• if (!ret)

• ret = vec_index;
  

+free_data:

• if (IS_GET_OP(arg))

• kfree(p.vec);
  

• return ret;

+}

+static long pagemap_scan_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +{

• struct pagemap_scan_arg __user *uarg = (struct pagemap_scan_arg __user *)arg;
• struct mm_struct *mm = file->private_data;
• struct pagemap_scan_arg argument;
• if (cmd == PAGEMAP_SCAN) {

• if (copy_from_user(&argument, uarg, sizeof(struct pagemap_scan_arg)))
  

• 	return -EFAULT;
  

• return do_pagemap_cmd(mm, &argument);
  

• }
• return -EINVAL;

+}

const struct file_operations proc_pagemap_operations = { .llseek = mem_lseek, /* borrow this */ .read = pagemap_read, .open = pagemap_open, .release = pagemap_release,

• .unlocked_ioctl = pagemap_scan_ioctl,
• .compat_ioctl = pagemap_scan_ioctl,

}; #endif /* CONFIG_PROC_PAGE_MONITOR */ diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index b7b56871029c..1ae9a8684b48 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -305,4 +305,54 @@ typedef int __bitwise __kernel_rwf_t; #define RWF_SUPPORTED (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\ RWF_APPEND) +/* Pagemap ioctl */ +#define PAGEMAP_SCAN _IOWR('f', 16, struct pagemap_scan_arg)

+/* Bits are set in the bitmap of the page_region and masks in pagemap_scan_args */ +#define PAGE_IS_WRITTEN (1 << 0) +#define PAGE_IS_FILE (1 << 1) +#define PAGE_IS_PRESENT (1 << 2) +#define PAGE_IS_SWAPPED (1 << 3)

+/*

• • struct page_region - Page region with bitmap flags
• • @start: Start of the region
• • @len: Length of the region
• • bitmap: Bits sets for the region
• */

+struct page_region {

• __u64 start;
• __u64 len;
• __u64 bitmap;

+};

+/*

• • struct pagemap_scan_arg - Pagemap ioctl argument
• • @start: Starting address of the region
• • @len: Length of the region (All the pages in this length are included)
• • @vec: Address of page_region struct array for output
• • @vec_len: Length of the page_region struct array
• • @max_pages: Optional max return pages
• • @flags: Flags for the IOCTL
• • @required_mask: Required mask - All of these bits have to be set in the PTE
• • @anyof_mask: Any mask - Any of these bits are set in the PTE
• • @excluded_mask: Exclude mask - None of these bits are set in the PTE
• • @return_mask: Bits that are to be reported in page_region
• */

+struct pagemap_scan_arg {

• __u64 start;
• __u64 len;
• __u64 vec;
• __u64 vec_len;
• __u32 max_pages;
• __u32 flags;
• __u64 required_mask;
• __u64 anyof_mask;
• __u64 excluded_mask;
• __u64 return_mask;

+};

+/* Special flags */ +#define PAGEMAP_WP_ENGAGE (1 << 0)

#endif /* _UAPI_LINUX_FS_H */

2.30.2

On Mon, Feb 13, 2023 at 05:55:19PM +0500, Muhammad Usama Anjum wrote:

On 2/9/23 3:15 AM, Peter Xu wrote:

...

On Thu, Feb 02, 2023 at 04:29:12PM +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_WRITTEN), 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_WRITTEN + PAGEMAP_WP_ENGAGE)

To get information about which pages have been written-to and/or write protect the pages, following must be performed first in order:

• The userfaultfd file descriptor is created with userfaultfd syscall.
• The UFFD_FEATURE_WP_ASYNC feature is set by UFFDIO_API IOCTL.
• The memory range is registered with UFFDIO_REGISTER_MODE_WP mode through UFFDIO_REGISTER IOCTL.

Then the any part of the registered memory or the whole memory region can be write protected using the UFFDIO_WRITEPROTECT IOCTL or PAGEMAP_SCAN 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 of struct page_region array 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 IOCTL doesn't support hugetlbs at the moment. No information about hugetlb can be obtained. This patch has evolved from a basic patch from Gabriel Krisman Bertazi.

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

Changes in v10:

• move changes in tools/include/uapi/linux/fs.h to separate patch
• update commit message

Change in v8:

• Correct is_pte_uffd_wp()
• Improve readability and error checks
• Remove some un-needed code

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 | 290 ++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 50 +++++++ 2 files changed, 340 insertions(+)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index e35a0398db63..c6bde19d63d9 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)) ||

•    (pte_swp_uffd_wp_any(pte)))
  

• return true;
  

• return false;

Sorry I should have mentioned this earlier: you can directly return here.

No problem at all. I'm replacing these two helper functions with following in next version so that !present pages don't show as dirty:

static inline bool is_pte_written(pte_t pte) { if ((pte_present(pte) && pte_uffd_wp(pte)) || (pte_swp_uffd_wp_any(pte))) return false; return (pte_present(pte) || is_swap_pte(pte)); }

Could you explain why you don't want to return dirty for !present? A page can be written then swapped out. Don't you want to know that happened (from dirty tracking POV)?

The code looks weird to me too.. We only have three types of ptes: (1) present, (2) swap, (3) none.

Then, "(pte_present() || is_swap_pte())" is the same as !pte_none(). Is that what you're really looking for?

static inline bool is_pmd_written(pmd_t pmd) { if ((pmd_present(pmd) && pmd_uffd_wp(pmd)) || (is_swap_pmd(pmd) && pmd_swp_uffd_wp(pmd))) return false; return (pmd_present(pmd) || is_swap_pmd(pmd)); }

[...]

...

...

• 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++;
  

• 	}
  

IIUC you can have:

int pagemap_scan_deposit(p) { if (p->vec_index >= p->vec_len) return -ENOSPC;

    if (p->prev->len) {
            memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region));
            p->vec_index++;
    }

    return 0;

}

Then call it here. I think it can also be called below to replace export_prev_to_out().

No this isn't possible. We fill up prev until the next range doesn't merge with it. At that point, we put prev into the output buffer and new range is put into prev. Now that we have shifted to smaller page walks of <= 512 entries. We want to visit all ranges before finally putting the prev to output. Sorry to have this some what complex method. The problem is that we want to merge the consective matching regions into one entry in the output. So to achieve this among multiple different page walks, the prev is being used.

Lets suppose we want to visit memory from 0x7FFF00000000 to 7FFF00400000 having length of 1024 pages and all of the memory has been written. walk_page_range() will be called 2 times. In the first call, prev will be set having length of 512. In second call, prev will be updated to 1024 as the previous range stored in prev could be extended. After this, the prev will be stored to the user output buffer consuming only 1 struct of page_range.

If we store prev back to output memory in every walk_page_range() call, we wouldn't get 1 struct of page_range with length 1024. Instead we would get 2 elements of page_range structs with half the length.

I didn't mean to merge PREV for each pgtable walk. What I meant is I think with such a pagemap_scan_deposit() you can rewrite it as:

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 (pagemap_scan_deposit(p)) return -ENOSPC; prev->start = addr; prev->len = len; prev->bitmap = bitmap; p->found_pages += len; } }

Then you can reuse pagemap_scan_deposit() when before returning to userspace, just to flush PREV to p->vec properly in a single helper. It also makes the code slightly easier to read.

On Tue, Feb 14, 2023 at 12:57:21PM +0500, Muhammad Usama Anjum wrote:

On 2/14/23 2:42 AM, Peter Xu wrote:

...

On Mon, Feb 13, 2023 at 05:55:19PM +0500, Muhammad Usama Anjum wrote:

...

On 2/9/23 3:15 AM, Peter Xu wrote:

...

On Thu, Feb 02, 2023 at 04:29:12PM +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_WRITTEN), 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_WRITTEN + PAGEMAP_WP_ENGAGE)

To get information about which pages have been written-to and/or write protect the pages, following must be performed first in order:

• The userfaultfd file descriptor is created with userfaultfd syscall.
• The UFFD_FEATURE_WP_ASYNC feature is set by UFFDIO_API IOCTL.
• The memory range is registered with UFFDIO_REGISTER_MODE_WP mode through UFFDIO_REGISTER IOCTL.

Then the any part of the registered memory or the whole memory region can be write protected using the UFFDIO_WRITEPROTECT IOCTL or PAGEMAP_SCAN 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 of struct page_region array 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 IOCTL doesn't support hugetlbs at the moment. No information about hugetlb can be obtained. This patch has evolved from a basic patch from Gabriel Krisman Bertazi.

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

Changes in v10:

• move changes in tools/include/uapi/linux/fs.h to separate patch
• update commit message

Change in v8:

• Correct is_pte_uffd_wp()
• Improve readability and error checks
• Remove some un-needed code

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 | 290 ++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 50 +++++++ 2 files changed, 340 insertions(+)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index e35a0398db63..c6bde19d63d9 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)) ||

•    (pte_swp_uffd_wp_any(pte)))
  

• return true;
  

• return false;

Sorry I should have mentioned this earlier: you can directly return here.

No problem at all. I'm replacing these two helper functions with following in next version so that !present pages don't show as dirty:

static inline bool is_pte_written(pte_t pte) { if ((pte_present(pte) && pte_uffd_wp(pte)) || (pte_swp_uffd_wp_any(pte))) return false; return (pte_present(pte) || is_swap_pte(pte)); }

Could you explain why you don't want to return dirty for !present? A page can be written then swapped out. Don't you want to know that happened (from dirty tracking POV)?

The code looks weird to me too.. We only have three types of ptes: (1) present, (2) swap, (3) none.

Then, "(pte_present() || is_swap_pte())" is the same as !pte_none(). Is that what you're really looking for?

Yes, this is what I've been trying to do. I'll use !pte_none() to make it simpler.

Ah I think I see what you wanted to do now.. But I'm afraid it won't work for all cases.

So IIUC the problem is anon pte can be empty, but since uffd-wp bit doesn't persist on anon (but none) ptes, then we got it lost and we cannot identify it from pages being written. Your solution will solve problem for anonymous, but I think it'll break file memories.

Example:

Consider one shmem page that got mapped, write protected (using UFFDIO_WP ioctl), written again (removing uffd-wp bit automatically), then zapped. The pte will be pte_none() but it's actually written, afaiu.

Maybe it's time we should introduce UFFD_FEATURE_WP_ZEROPAGE, so we'll need to install pte markers for anonymous too (then it will work similarly like shmem/hugetlbfs, that we'll report writting to zero pages), then you'll need to have the new UFFD_FEATURE_WP_ASYNC depend on it. With that I think you can keep using the old check and it should start to work.

Please let me know if my understanding is correct above.

I'll see whether I can quickly play with UFFD_FEATURE_WP_ZEROPAGE with some patch at the meantime. That's something we wanted before too, when the app cares about zero pages on anon. We used to populate the pages before doing ioctl(UFFDIO_WP) to make sure zero pages will be repoted too, but that flag should be more efficient.

...

...

static inline bool is_pmd_written(pmd_t pmd) { if ((pmd_present(pmd) && pmd_uffd_wp(pmd)) || (is_swap_pmd(pmd) && pmd_swp_uffd_wp(pmd))) return false; return (pmd_present(pmd) || is_swap_pmd(pmd)); }

[...]

...

...

...

• 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++;
  

• 	}
  

IIUC you can have:

int pagemap_scan_deposit(p) { if (p->vec_index >= p->vec_len) return -ENOSPC;

    if (p->prev->len) {
            memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region));
            p->vec_index++;
    }

    return 0;

}

Then call it here. I think it can also be called below to replace export_prev_to_out().

No this isn't possible. We fill up prev until the next range doesn't merge with it. At that point, we put prev into the output buffer and new range is put into prev. Now that we have shifted to smaller page walks of <= 512 entries. We want to visit all ranges before finally putting the prev to output. Sorry to have this some what complex method. The problem is that we want to merge the consective matching regions into one entry in the output. So to achieve this among multiple different page walks, the prev is being used.

Lets suppose we want to visit memory from 0x7FFF00000000 to 7FFF00400000 having length of 1024 pages and all of the memory has been written. walk_page_range() will be called 2 times. In the first call, prev will be set having length of 512. In second call, prev will be updated to 1024 as the previous range stored in prev could be extended. After this, the prev will be stored to the user output buffer consuming only 1 struct of page_range.

If we store prev back to output memory in every walk_page_range() call, we wouldn't get 1 struct of page_range with length 1024. Instead we would get 2 elements of page_range structs with half the length.

I didn't mean to merge PREV for each pgtable walk. What I meant is I think with such a pagemap_scan_deposit() you can rewrite it as:

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 (pagemap_scan_deposit(p)) return -ENOSPC; prev->start = addr; prev->len = len; prev->bitmap = bitmap; p->found_pages += len; } }

Then you can reuse pagemap_scan_deposit() when before returning to userspace, just to flush PREV to p->vec properly in a single helper. It also makes the code slightly easier to read.

Yeah, this would have worked as you have described. But in pagemap_scan_output(), we are flushing prev to p->vec. But later in export_prev_to_out() we need to flush prev to user_memory directly.

I think there's a loop to copy_to_user(). Could you use the new helper so the copy_to_user() loop will work without export_prev_to_out()?

I really hope we can get rid of export_prev_to_out(). Thanks,

On Thu, Feb 02, 2023 at 04:29:12PM +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_WRITTEN), 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_WRITTEN + PAGEMAP_WP_ENGAGE)

To get information about which pages have been written-to and/or write protect the pages, following must be performed first in order:

• The userfaultfd file descriptor is created with userfaultfd syscall.
• The UFFD_FEATURE_WP_ASYNC feature is set by UFFDIO_API IOCTL.
• The memory range is registered with UFFDIO_REGISTER_MODE_WP mode through UFFDIO_REGISTER IOCTL.

Then the any part of the registered memory or the whole memory region can be write protected using the UFFDIO_WRITEPROTECT IOCTL or PAGEMAP_SCAN 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 of struct page_region array 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 IOCTL doesn't support hugetlbs at the moment. No information about hugetlb can be obtained. This patch has evolved from a basic patch from Gabriel Krisman Bertazi.

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

Changes in v10:

• move changes in tools/include/uapi/linux/fs.h to separate patch
• update commit message

Change in v8:

• Correct is_pte_uffd_wp()
• Improve readability and error checks
• Remove some un-needed code

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 | 290 ++++++++++++++++++++++++++++++++++++++++ include/uapi/linux/fs.h | 50 +++++++ 2 files changed, 340 insertions(+)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index e35a0398db63..c6bde19d63d9 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)) ||

•    (pte_swp_uffd_wp_any(pte)))
  

• return true;
  

• 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,284 @@ static int pagemap_release(struct inode *inode, struct file *file) return 0; } +#define PAGEMAP_BITS_ALL (PAGE_IS_WRITTEN | PAGE_IS_FILE | \

• 			 PAGE_IS_PRESENT | PAGE_IS_SWAPPED)
  

+#define PAGEMAP_NON_WRITTEN_BITS (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 HAS_NO_SPACE(p) (p->max_pages && (p->found_pages == p->max_pages))

+#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_WRITTEN) || \

• (a->anyof_mask & PAGE_IS_WRITTEN))
  

All these macros are specific to pagemap_scan_ioctl() and should be namespaced accordingly, e.g. PM_SCAN_BITS_ALL, PM_SCAN_BITMAP etc.

Also, IS_<opname>_OP() will be more readable as PM_SCAN_OP_IS_<opname> and I'd suggest to open code IS_WP_ENGAGE_OP() and IS_GET_OP() and make HAS_NO_SPACE() and IS_WT_REQUIRED() static inlines rather than macros.

And I'd also make IS_GET_OP() more explicit by defining a PAGEMAP_WP_GET or similar flag rather than using arg->vec.

+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)

Please keep the lines under 80 characters limit.

+{

• struct pagemap_scan_private *p = walk->private;
• struct vm_area_struct *vma = walk->vma;
• if (IS_WT_REQUIRED(p) && !userfaultfd_wp(vma) && !userfaultfd_wp_async(vma))

• return -EPERM;
  

• if (vma->vm_flags & VM_PFNMAP)

• return 1;
  

• return 0;

+}

+static inline int pagemap_scan_output(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 (HAS_NO_SPACE(p))

• return -ENOSPC;
  

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

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

• if (!len)

• return -EINVAL;
  

• 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;
  

• }
  

• }
• return 0;

Please don't save on empty lines. Empty lines between logical pieces improve readability.

+}

+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;

+}

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

• 			 unsigned long end, struct mm_walk *walk)
  

+{

• struct pagemap_scan_private *p = walk->private;
• struct vm_area_struct *vma = walk->vma;
• unsigned long addr = end;
• spinlock_t *ptl;
• int ret = 0;
• pte_t *pte;

+#ifdef CONFIG_TRANSPARENT_HUGEPAGE

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

• bool pmd_wt;
  

• 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.
  

•  */
  

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

• 	spin_unlock(ptl);
  

• 	split_huge_pmd(vma, pmd, start);
  

• 	goto process_smaller_pages;
  

• }
  

• if (IS_GET_OP(p))
  

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

• 				  is_swap_pmd(*pmd), p, start,
  

• 				  (end - start)/PAGE_SIZE);
  

• spin_unlock(ptl);
  

• if (!ret) {
  

• 	if (pmd_wt && IS_WP_ENGAGE_OP(p))
  

• 		uffd_wp_range(walk->mm, vma, start, HPAGE_SIZE, true);
  

• }
  

• 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, start, &ptl);
• if (IS_GET_OP(p)) {

• for (addr = start; addr < end; pte++, addr += PAGE_SIZE) {
  

• 	ret = pagemap_scan_output(!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 ((!ret || ret == -ENOSPC) && IS_WP_ENGAGE_OP(p) && (addr - start))

• uffd_wp_range(walk->mm, vma, start, addr - start, true);
  

• 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;
• int ret = 0;
• if (vma)

• ret = pagemap_scan_output(false, vma->vm_file, false, false, p, addr,
  

• 			  (end - addr)/PAGE_SIZE);
  

• return ret;

+}

+/* No hugetlb support is present. */ +static const struct mm_walk_ops pagemap_scan_ops = {

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

+};

+static long do_pagemap_cmd(struct mm_struct *mm, struct pagemap_scan_arg *arg) +{

• unsigned long empty_slots, vec_index = 0;
• unsigned long __user start, end;
• unsigned long __start, __end;
• struct page_region __user *vec;
• struct pagemap_scan_private p;
• int ret = 0;
• start = (unsigned long)untagged_addr(arg->start);
• vec = (struct page_region *)(unsigned long)untagged_addr(arg->vec);
• /* Validate memory ranges */
• if ((!IS_ALIGNED(start, PAGE_SIZE)) || (!access_ok((void __user *)start, arg->len)))

• return -EINVAL;
  

• if (IS_GET_OP(arg) && ((arg->vec_len == 0) ||

•    (!access_ok((void __user *)vec, arg->vec_len * sizeof(struct page_region)))))
  

• return -EINVAL;
  

• /* Detect illegal flags and masks */
• if ((arg->flags & ~PAGEMAP_WP_ENGAGE) || (arg->required_mask & ~PAGEMAP_BITS_ALL) ||

•    (arg->anyof_mask & ~PAGEMAP_BITS_ALL) || (arg->excluded_mask & ~PAGEMAP_BITS_ALL) ||
  

•    (arg->return_mask & ~PAGEMAP_BITS_ALL))
  

• return -EINVAL;
  

• if (IS_GET_OP(arg) && ((!arg->required_mask && !arg->anyof_mask && !arg->excluded_mask) ||

• 		!arg->return_mask))
  

• return -EINVAL;
  

• /* The non-WT flags cannot be obtained if PAGEMAP_WP_ENGAGE is also specified. */
• if (IS_WP_ENGAGE_OP(arg) && ((arg->required_mask & PAGEMAP_NON_WRITTEN_BITS) ||

•    (arg->anyof_mask & PAGEMAP_NON_WRITTEN_BITS)))
  

• return -EINVAL;
  

I'd split argument validation into a separate function and split the OR'ed conditions into separate if statements, e.g

bool pm_scan_args_valid(struct pagemap_scan_arg *arg) { if (IS_GET_OP(arg)) { if (!arg->return_mask) return false; if (!arg->required_mask && !arg->anyof_mask && !arg->excluded_mask) return false; }

/* ... */

return true; }

• end = start + arg->len;
• p.max_pages = arg->max_pages;
• p.found_pages = 0;
• p.flags = arg->flags;
• p.required_mask = arg->required_mask;
• p.anyof_mask = arg->anyof_mask;
• p.excluded_mask = arg->excluded_mask;
• p.return_mask = arg->return_mask;
• p.prev.len = 0;
• p.vec_len = (PAGEMAP_WALK_SIZE >> PAGE_SHIFT);
• if (IS_GET_OP(arg)) {

• p.vec = kmalloc_array(p.vec_len, sizeof(struct page_region), GFP_KERNEL);
  

• if (!p.vec)
  

• 	return -ENOMEM;
  

• } else {

• p.vec = NULL;
  

• }
• __start = __end = start;
• while (!ret && __end < end) {

• p.vec_index = 0;
  

• empty_slots = arg->vec_len - vec_index;
  

• if (p.vec_len > empty_slots)
  

• 	p.vec_len = empty_slots;
  

• __end = (__start + PAGEMAP_WALK_SIZE) & PAGEMAP_WALK_MASK;
  

• if (__end > end)
  

• 	__end = end;
  

• mmap_read_lock(mm);
  

• ret = walk_page_range(mm, __start, __end, &pagemap_scan_ops, &p);
  

• mmap_read_unlock(mm);
  

• if (!(!ret || ret == -ENOSPC))
  

• 	goto free_data;
  

• __start = __end;
  

• if (IS_GET_OP(arg) && p.vec_index) {
  

• 	if (copy_to_user(&vec[vec_index], p.vec,
  

• 			 p.vec_index * sizeof(struct page_region))) {
  

• 		ret = -EFAULT;
  

• 		goto free_data;
  

• 	}
  

• 	vec_index += p.vec_index;
  

• }
  

• }
• ret = export_prev_to_out(&p, vec, &vec_index);
• if (!ret)

• ret = vec_index;
  

+free_data:

• if (IS_GET_OP(arg))

• kfree(p.vec);
  

• return ret;

+}

+static long pagemap_scan_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +{

• struct pagemap_scan_arg __user *uarg = (struct pagemap_scan_arg __user *)arg;
• struct mm_struct *mm = file->private_data;
• struct pagemap_scan_arg argument;
• if (cmd == PAGEMAP_SCAN) {

• if (copy_from_user(&argument, uarg, sizeof(struct pagemap_scan_arg)))
  

• 	return -EFAULT;
  

• return do_pagemap_cmd(mm, &argument);
  

• }
• return -EINVAL;

+}

const struct file_operations proc_pagemap_operations = { .llseek = mem_lseek, /* borrow this */ .read = pagemap_read, .open = pagemap_open, .release = pagemap_release,

• .unlocked_ioctl = pagemap_scan_ioctl,
• .compat_ioctl = pagemap_scan_ioctl,

}; #endif /* CONFIG_PROC_PAGE_MONITOR */ diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index b7b56871029c..1ae9a8684b48 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -305,4 +305,54 @@ typedef int __bitwise __kernel_rwf_t; #define RWF_SUPPORTED (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\ RWF_APPEND) +/* Pagemap ioctl */ +#define PAGEMAP_SCAN _IOWR('f', 16, struct pagemap_scan_arg)

+/* Bits are set in the bitmap of the page_region and masks in pagemap_scan_args */ +#define PAGE_IS_WRITTEN (1 << 0) +#define PAGE_IS_FILE (1 << 1) +#define PAGE_IS_PRESENT (1 << 2) +#define PAGE_IS_SWAPPED (1 << 3)

+/*

• • struct page_region - Page region with bitmap flags
• • @start: Start of the region
• • @len: Length of the region
• • bitmap: Bits sets for the region
• */

+struct page_region {

• __u64 start;
• __u64 len;
• __u64 bitmap;

+};

+/*

• • struct pagemap_scan_arg - Pagemap ioctl argument
• • @start: Starting address of the region
• • @len: Length of the region (All the pages in this length are included)
• • @vec: Address of page_region struct array for output
• • @vec_len: Length of the page_region struct array
• • @max_pages: Optional max return pages
• • @flags: Flags for the IOCTL
• • @required_mask: Required mask - All of these bits have to be set in the PTE
• • @anyof_mask: Any mask - Any of these bits are set in the PTE
• • @excluded_mask: Exclude mask - None of these bits are set in the PTE
• • @return_mask: Bits that are to be reported in page_region
• */

+struct pagemap_scan_arg {

• __u64 start;
• __u64 len;
• __u64 vec;
• __u64 vec_len;
• __u32 max_pages;
• __u32 flags;
• __u64 required_mask;
• __u64 anyof_mask;
• __u64 excluded_mask;
• __u64 return_mask;

+};

+/* Special flags */ +#define PAGEMAP_WP_ENGAGE (1 << 0)

#endif /* _UAPI_LINUX_FS_H */

2.30.2

Hello Nadav,

Thank you so much for reviewing!

On 2/19/23 6:52 PM, Nadav Amit wrote:

On 2/2/23 1:29 PM, 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_WRITTEN),

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_WRITTEN + PAGEMAP_WP_ENGAGE)

To get information about which pages have been written-to and/or write protect the pages, following must be performed first in order:

• The userfaultfd file descriptor is created with userfaultfd syscall.
• The UFFD_FEATURE_WP_ASYNC feature is set by UFFDIO_API IOCTL.
• The memory range is registered with UFFDIO_REGISTER_MODE_WP mode

through UFFDIO_REGISTER IOCTL. Then the any part of the registered memory or the whole memory region can be write protected using the UFFDIO_WRITEPROTECT IOCTL or PAGEMAP_SCAN 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 of struct page_region array 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 IOCTL doesn't support hugetlbs at the moment. No information about hugetlb can be obtained. This patch has evolved from a basic patch from Gabriel Krisman Bertazi.

I was not involved before, so I am not commenting on the API and code to avoid making unhelpful noise.

Having said that, some things in the code seem quite dirty and make understanding the code hard to read.

There is a new proposal about the flags in the interface. I'll include you there.

...

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

Changes in v10:

• move changes in tools/include/uapi/linux/fs.h to separate patch
• update commit message

Change in v8:

• Correct is_pte_uffd_wp()
• Improve readability and error checks
• Remove some un-needed code

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      | 290 ++++++++++++++++++++++++++++++++++++++++   include/uapi/linux/fs.h |  50 +++++++   2 files changed, 340 insertions(+)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index e35a0398db63..c6bde19d63d9 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)) || +        (pte_swp_uffd_wp_any(pte))) +        return true; +    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,284 @@ static int pagemap_release(struct inode *inode, struct file *file)       return 0;   }   +#define PAGEMAP_BITS_ALL        (PAGE_IS_WRITTEN | PAGE_IS_FILE |    \ +                     PAGE_IS_PRESENT | PAGE_IS_SWAPPED) +#define PAGEMAP_NON_WRITTEN_BITS    (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 HAS_NO_SPACE(p)            (p->max_pages && (p->found_pages == p->max_pages))

I think that in general it is better to have an inline function instead of macros when possible, as it is clearer and checks types. Anyhow, IMHO most of these macros are better be open-coded.

I'll update most of these in next version.

...

+#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_WRITTEN) ||    \ +     (a->anyof_mask & PAGE_IS_WRITTEN))

+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) && !userfaultfd_wp(vma) && !userfaultfd_wp_async(vma)) +        return -EPERM; +    if (vma->vm_flags & VM_PFNMAP) +        return 1; +    return 0; +}

+static inline int pagemap_scan_output(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 (HAS_NO_SPACE(p)) +        return -ENOSPC;

+    if (p->max_pages && p->found_pages + len >= p->max_pages) +        len = p->max_pages - p->found_pages; +    if (!len) +        return -EINVAL;

+    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;

The use of "len" both for bytes and pages is very confusing. Consider changing the name to n_pages or something similar.

Will update in next version.

...

+            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; +        } +    } +    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; +}

+static inline int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start, +                     unsigned long end, struct mm_walk *walk) +{ +    struct pagemap_scan_private *p = walk->private; +    struct vm_area_struct *vma = walk->vma; +    unsigned long addr = end; +    spinlock_t *ptl; +    int ret = 0; +    pte_t *pte;

+#ifdef CONFIG_TRANSPARENT_HUGEPAGE +    ptl = pmd_trans_huge_lock(pmd, vma); +    if (ptl) { +        bool pmd_wt;

+        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. +         */ +        if (pmd_wt && IS_WP_ENGAGE_OP(p) && (end - start < HPAGE_SIZE)) { +            spin_unlock(ptl); +            split_huge_pmd(vma, pmd, start); +            goto process_smaller_pages;

I think that such goto's are really confusing and should be avoided. And using 'else' (could have easily prevented the need for goto). It is not the best solution though, since I think it would have been better to invert the conditions.

Yeah, else can be used here. But then we'll have to add a tab to all the code after adding else. We have already so many tabs and very less space to right code. Not sure which is better.

...

+        } +        if (IS_GET_OP(p)) +            ret = pagemap_scan_output(pmd_wt, vma->vm_file, pmd_present(*pmd), +                          is_swap_pmd(*pmd), p, start, +                          (end - start)/PAGE_SIZE); +        spin_unlock(ptl); +        if (!ret) { +            if (pmd_wt && IS_WP_ENGAGE_OP(p)) +                uffd_wp_range(walk->mm, vma, start, HPAGE_SIZE, true); +        } +        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, start, &ptl); +    if (IS_GET_OP(p)) { +        for (addr = start; addr < end; pte++, addr += PAGE_SIZE) { +            ret = pagemap_scan_output(!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);

We might have not entered the loop and pte-1 would be wrong.

...

+    if ((!ret || ret == -ENOSPC) && IS_WP_ENGAGE_OP(p) && (addr - start))

What does 'addr - start' mean? If you want to say they are not equal, why not say so?

This has been revamped in the next version.

...

+        uffd_wp_range(walk->mm, vma, start, addr - start, true);

+    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; +    int ret = 0;

+    if (vma) +        ret = pagemap_scan_output(false, vma->vm_file, false, false, p, addr, +                      (end - addr)/PAGE_SIZE); +    return ret; +}

+/* No hugetlb support is present. */ +static const struct mm_walk_ops pagemap_scan_ops = { +    .test_walk = pagemap_scan_test_walk, +    .pmd_entry = pagemap_scan_pmd_entry, +    .pte_hole = pagemap_scan_pte_hole, +};

+static long do_pagemap_cmd(struct mm_struct *mm, struct pagemap_scan_arg *arg) +{ +    unsigned long empty_slots, vec_index = 0; +    unsigned long __user start, end;

The whole point of __user (attribute) is to be assigned to pointers.

I'll remove it.

...

+    unsigned long __start, __end;

I think such names do not convey sufficient information.

I'll update it.

...

+    struct page_region __user *vec; +    struct pagemap_scan_private p; +    int ret = 0;

+    start = (unsigned long)untagged_addr(arg->start); +    vec = (struct page_region *)(unsigned long)untagged_addr(arg->vec);

+    /* Validate memory ranges */ +    if ((!IS_ALIGNED(start, PAGE_SIZE)) || (!access_ok((void __user *)start, arg->len))) +        return -EINVAL; +    if (IS_GET_OP(arg) && ((arg->vec_len == 0) || +        (!access_ok((void __user *)vec, arg->vec_len * sizeof(struct page_region))))) +        return -EINVAL;

+    /* Detect illegal flags and masks */ +    if ((arg->flags & ~PAGEMAP_WP_ENGAGE) || (arg->required_mask & ~PAGEMAP_BITS_ALL) || +        (arg->anyof_mask & ~PAGEMAP_BITS_ALL) || (arg->excluded_mask & ~PAGEMAP_BITS_ALL) || +        (arg->return_mask & ~PAGEMAP_BITS_ALL))

Using bitwise or to check

(arg->required_mask | arg->anyof_mask |      arg->excluded_mask | arg->return_mask) & ~PAGE_MAP_BITS_ALL

Would have been much cleaner, IMHO.

I'll update it.

...

+        return -EINVAL; +    if (IS_GET_OP(arg) && ((!arg->required_mask && !arg->anyof_mask && !arg->excluded_mask) || +                !arg->return_mask)) +        return -EINVAL; +    /* The non-WT flags cannot be obtained if PAGEMAP_WP_ENGAGE is also specified. */ +    if (IS_WP_ENGAGE_OP(arg) && ((arg->required_mask & PAGEMAP_NON_WRITTEN_BITS) || +        (arg->anyof_mask & PAGEMAP_NON_WRITTEN_BITS))) +        return -EINVAL;

+    end = start + arg->len; +    p.max_pages = arg->max_pages; +    p.found_pages = 0; +    p.flags = arg->flags; +    p.required_mask = arg->required_mask; +    p.anyof_mask = arg->anyof_mask; +    p.excluded_mask = arg->excluded_mask; +    p.return_mask = arg->return_mask; +    p.prev.len = 0; +    p.vec_len = (PAGEMAP_WALK_SIZE >> PAGE_SHIFT);

+    if (IS_GET_OP(arg)) { +        p.vec = kmalloc_array(p.vec_len, sizeof(struct page_region), GFP_KERNEL); +        if (!p.vec) +            return -ENOMEM; +    } else { +        p.vec = NULL;

I find it cleaner to initialize 'p.vec = NULL' unconditionally before IS_GET_OP() check.

It'll get updated.

...

+    } +    __start = __end = start; +    while (!ret && __end < end) { +        p.vec_index = 0; +        empty_slots = arg->vec_len - vec_index; +        if (p.vec_len > empty_slots) +            p.vec_len = empty_slots;

+        __end = (__start + PAGEMAP_WALK_SIZE) & PAGEMAP_WALK_MASK; +        if (__end > end) +            __end = end;

Easier to understand using min().

Will update.

...

+        mmap_read_lock(mm); +        ret = walk_page_range(mm, __start, __end, &pagemap_scan_ops, &p); +        mmap_read_unlock(mm); +        if (!(!ret || ret == -ENOSPC))

Double negations complicate things unnecessarily.

And if you already "break" on ret, why do you check the condition in the while loop?

Ohh, good catch.

...

+            goto free_data;

+        __start = __end; +        if (IS_GET_OP(arg) && p.vec_index) { +            if (copy_to_user(&vec[vec_index], p.vec, +                     p.vec_index * sizeof(struct page_region))) { +                ret = -EFAULT; +                goto free_data; +            } +            vec_index += p.vec_index; +        } +    } +    ret = export_prev_to_out(&p, vec, &vec_index); +    if (!ret) +        ret = vec_index; +free_data: +    if (IS_GET_OP(arg)) +        kfree(p.vec);

Just call it unconditionally.

I didn't know it. I'll do it.

...

+    return ret; +}

+static long pagemap_scan_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +{ +    struct pagemap_scan_arg __user *uarg = (struct pagemap_scan_arg __user *)arg; +    struct mm_struct *mm = file->private_data; +    struct pagemap_scan_arg argument;

+    if (cmd == PAGEMAP_SCAN) { +        if (copy_from_user(&argument, uarg, sizeof(struct pagemap_scan_arg))) +            return -EFAULT; +        return do_pagemap_cmd(mm, &argument); +    } +    return -EINVAL; +}

const struct file_operations proc_pagemap_operations = {       .llseek        = mem_lseek, /* borrow this */       .read        = pagemap_read,       .open        = pagemap_open,       .release    = pagemap_release, +    .unlocked_ioctl = pagemap_scan_ioctl, +    .compat_ioctl    = pagemap_scan_ioctl,   };   #endif /* CONFIG_PROC_PAGE_MONITOR */   diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index b7b56871029c..1ae9a8684b48 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -305,4 +305,54 @@ typedef int __bitwise __kernel_rwf_t;   #define RWF_SUPPORTED    (RWF_HIPRI | RWF_DSYNC | RWF_SYNC | RWF_NOWAIT |\                RWF_APPEND)   +/* Pagemap ioctl */ +#define PAGEMAP_SCAN    _IOWR('f', 16, struct pagemap_scan_arg)

+/* Bits are set in the bitmap of the page_region and masks in pagemap_scan_args */ +#define PAGE_IS_WRITTEN        (1 << 0) +#define PAGE_IS_FILE        (1 << 1) +#define PAGE_IS_PRESENT        (1 << 2) +#define PAGE_IS_SWAPPED        (1 << 3)

These names are way too generic and are likely to be misused for the wrong purpose. The "_IS_" part seems confusing as well. So I think the naming needs to be fixed and some new type (using typedef) or enum should be introduced to hold these flags. I understand it is part of uapi and it is less common there, but it is not unheard of and does make things clearer.

Do you think PM_SCAN_PAGE_IS_* work here?

...

+/*

• • struct page_region - Page region with bitmap flags
• • @start:    Start of the region
• • @len:    Length of the region
• • bitmap:    Bits sets for the region
• */

+struct page_region { +    __u64 start; +    __u64 len;

I presume in bytes. Would be useful to mention.

Length of region in pages.

...

+    __u64 bitmap; +};

+/*

• • struct pagemap_scan_arg - Pagemap ioctl argument
• • @start:        Starting address of the region
• • @len:        Length of the region (All the pages in this length are

included)

• • @vec:        Address of page_region struct array for output
• • @vec_len:        Length of the page_region struct array
• • @max_pages:        Optional max return pages
• • @flags:        Flags for the IOCTL
• • @required_mask:    Required mask - All of these bits have to be set

in the PTE

• • @anyof_mask:        Any mask - Any of these bits are set in the PTE
• • @excluded_mask:    Exclude mask - None of these bits are set in the PTE
• • @return_mask:    Bits that are to be reported in page_region
• */

+struct pagemap_scan_arg { +    __u64 start; +    __u64 len; +    __u64 vec; +    __u64 vec_len; +    __u32 max_pages; +    __u32 flags; +    __u64 required_mask; +    __u64 anyof_mask; +    __u64 excluded_mask; +    __u64 return_mask; +};

+/* Special flags */ +#define PAGEMAP_WP_ENGAGE    (1 << 0)

#endif /* _UAPI_LINUX_FS_H */

Hi Nadav, Mike, Michał,

Can you please share your thoughts at [A] below?

On 2/23/23 12:10 AM, Nadav Amit wrote:

...

On Feb 20, 2023, at 5:24 AM, Muhammad Usama Anjum usama.anjum@collabora.com wrote:

...

...

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

•                 unsigned long end, struct mm_walk *walk)
  

+{

• struct pagemap_scan_private *p = walk->private;
• struct vm_area_struct *vma = walk->vma;
• unsigned long addr = end;
• spinlock_t *ptl;
• int ret = 0;
• pte_t *pte;

+#ifdef CONFIG_TRANSPARENT_HUGEPAGE

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

•    bool pmd_wt;
  

•    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.
  

•     */
  

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

•        spin_unlock(ptl);
  

•        split_huge_pmd(vma, pmd, start);
  

•        goto process_smaller_pages;
  

I think that such goto's are really confusing and should be avoided. And using 'else' (could have easily prevented the need for goto). It is not the best solution though, since I think it would have been better to invert the conditions.

Yeah, else can be used here. But then we'll have to add a tab to all the code after adding else. We have already so many tabs and very less space to right code. Not sure which is better.

goto’s are usually not the right solution. You can extract things into a different function if you have to.

I’m not sure why IS_GET_OP(p) might be false and what’s the meaning of taking the lock and dropping it in such a case. I think that the code can be simplified and additional condition nesting can be avoided.