*Changes in v18* - Rebase on top of next-20230613 - Minor updates
*Changes in v17* - Rebase on top of next-20230606 - Minor improvements in PAGEMAP_SCAN IOCTL patch
*Changes in v16* - Fix a corner case - Add exclusive PM_SCAN_OP_WP back
*Changes in v15* - Build fix (Add missed build fix in RESEND)
*Changes in v14* - Fix build error caused by #ifdef added at last minute in some configs
*Changes in v13* - Rebase on top of next-20230414 - Give-up on using uffd_wp_range() and write new helpers, flush tlb only once
*Changes in v12* - Update and other memory types to UFFD_FEATURE_WP_ASYNC - Rebaase on top of next-20230406 - Review updates
*Changes in v11* - Rebase on top of next-20230307 - Base patches on UFFD_FEATURE_WP_UNPOPULATED - Do a lot of cosmetic changes and review updates - Remove ENGAGE_WP + !GET operation as it can be performed with UFFDIO_WRITEPROTECT
*Changes in v10* - Add specific condition to return error if hugetlb is used with wp async - Move changes in tools/include/uapi/linux/fs.h to separate patch - Add documentation
*Changes in v9:* - Correct fault resolution for userfaultfd wp async - Fix build warnings and errors which were happening on some configs - Simplify pagemap ioctl's code
*Changes in v8:* - Update uffd async wp implementation - Improve PAGEMAP_IOCTL implementation
*Changes in v7:* - Add uffd wp async - Update the IOCTL to use uffd under the hood instead of soft-dirty flags
*Motivation* The real motivation for adding PAGEMAP_SCAN IOCTL is to emulate Windows GetWriteWatch() syscall [1]. The GetWriteWatch{} retrieves the addresses of the pages that are written to in a region of virtual memory.
This syscall is used in Windows applications and games etc. This syscall is being emulated in pretty slow manner in userspace. Our purpose is to enhance the kernel such that we translate it efficiently in a better way. Currently some out of tree hack patches are being used to efficiently emulate it in some kernels. We intend to replace those with these patches. So the whole gaming on Linux can effectively get benefit from this. It means there would be tons of users of this code.
CRIU use case [2] was mentioned by Andrei and Danylo:
Use cases for migrating sparse VMAs are binaries sanitized with ASAN, MSAN or TSAN [3]. All of these sanitizers produce sparse mappings of shadow memory [4]. Being able to migrate such binaries allows to highly reduce the amount of work needed to identify and fix post-migration crashes, which happen constantly.
Andrei's defines the following uses of this code: * it is more granular and allows us to track changed pages more effectively. The current interface can clear dirty bits for the entire process only. In addition, reading info about pages is a separate operation. It means we must freeze the process to read information about all its pages, reset dirty bits, only then we can start dumping pages. The information about pages becomes more and more outdated, while we are processing pages. The new interface solves both these downsides. First, it allows us to read pte bits and clear the soft-dirty bit atomically. It means that CRIU will not need to freeze processes to pre-dump their memory. Second, it clears soft-dirty bits for a specified region of memory. It means CRIU will have actual info about pages to the moment of dumping them. * The new interface has to be much faster because basic page filtering is happening in the kernel. With the old interface, we have to read pagemap for each page.
*Implementation Evolution (Short Summary)* From the definition of GetWriteWatch(), we feel like kernel's soft-dirty feature can be used under the hood with some additions like: * reset soft-dirty flag for only a specific region of memory instead of clearing the flag for the entire process * get and clear soft-dirty flag for a specific region atomically
So we decided to use ioctl on pagemap file to read or/and reset soft-dirty flag. But using soft-dirty flag, sometimes we get extra pages which weren't even written. They had become soft-dirty because of VMA merging and VM_SOFTDIRTY flag. This breaks the definition of GetWriteWatch(). We were able to by-pass this short coming by ignoring VM_SOFTDIRTY until David reported that mprotect etc messes up the soft-dirty flag while ignoring VM_SOFTDIRTY [5]. This wasn't happening until [6] got introduced. We discussed if we can revert these patches. But we could not reach to any conclusion. So at this point, I made couple of tries to solve this whole VM_SOFTDIRTY issue by correcting the soft-dirty implementation: * [7] Correct the bug fixed wrongly back in 2014. It had potential to cause regression. We left it behind. * [8] Keep a list of soft-dirty part of a VMA across splits and merges. I got the reply don't increase the size of the VMA by 8 bytes.
At this point, we left soft-dirty considering it is too much delicate and userfaultfd [9] seemed like the only way forward. From there onward, we have been basing soft-dirty emulation on userfaultfd wp feature where kernel resolves the faults itself when WP_ASYNC feature is used. It was straight forward to add WP_ASYNC feature in userfautlfd. Now we get only those pages dirty or written-to which are really written in reality. (PS There is another WP_UNPOPULATED userfautfd feature is required which is needed to avoid pre-faulting memory before write-protecting [9].)
All the different masks were added on the request of CRIU devs to create interface more generic and better.
[1] https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-g... [2] https://lore.kernel.org/all/20221014134802.1361436-1-mdanylo@google.com [3] https://github.com/google/sanitizers [4] https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm#64-bit [5] https://lore.kernel.org/all/bfcae708-db21-04b4-0bbe-712badd03071@redhat.com [6] https://lore.kernel.org/all/20220725142048.30450-1-peterx@redhat.com/ [7] https://lore.kernel.org/all/20221122115007.2787017-1-usama.anjum@collabora.c... [8] https://lore.kernel.org/all/20221220162606.1595355-1-usama.anjum@collabora.c... [9] https://lore.kernel.org/all/20230306213925.617814-1-peterx@redhat.com [10] https://lore.kernel.org/all/20230125144529.1630917-1-mdanylo@google.com
* Original Cover letter from v8* Hello,
Note: Soft-dirty pages and pages which have been written-to are synonyms. As kernel already has soft-dirty feature inside which we have given up to use, we are using written-to terminology while using UFFD async WP under the hood.
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)
It is possible to find and clear soft-dirty pages entirely in userspace. But it isn't efficient: - The mprotect and SIGSEGV handler for bookkeeping - The userfaultfd wp (synchronous) with the handler for bookkeeping
Some benchmarks can be seen here[1]. This series adds features that weren't present earlier: - There is no atomic get soft-dirty/Written-to status and clear present in the kernel. - The pages which have been written-to can not be found in accurate way. (Kernel's soft-dirty PTE bit + sof_dirty VMA bit shows more soft-dirty pages than there actually are.)
Historically, soft-dirty PTE bit tracking has been used in the CRIU project. The procfs interface is enough for finding the soft-dirty bit status and clearing the soft-dirty bit of all the pages of a process. We have the use case where we need to track the soft-dirty PTE bit for only specific pages on-demand. We need this tracking and clear mechanism of a region of memory while the process is running to emulate the getWriteWatch() syscall of Windows.
*(Moved to using UFFD instead of soft-dirtyi feature to find pages which have been written-to from v7 patch series)*: Stop using the soft-dirty flags for finding which pages have been written to. It is too delicate and wrong as it shows more soft-dirty pages than the actual soft-dirty pages. There is no interest in correcting it [2][3] as this is how the feature was written years ago. It shouldn't be updated to changed behaviour. Peter Xu has suggested using the async version of the UFFD WP [4] as it is based inherently on the PTEs.
So in this patch series, I've added a new mode to the UFFD which is asynchronous version of the write protect. When this variant of the UFFD WP is used, the page faults are resolved automatically by the kernel. The pages which have been written-to can be found by reading pagemap file (!PM_UFFD_WP). This feature can be used successfully to find which pages have been written to from the time the pages were write protected. This works just like the soft-dirty flag without showing any extra pages which aren't soft-dirty in reality.
The information related to pages if the page is file mapped, present and swapped is required for the CRIU project [5][6]. The addition of the required mask, any mask, excluded mask and return masks are also required for the CRIU project [5].
The IOCTL returns the addresses of the pages which match the specific masks. The page addresses are returned in struct page_region in a compact form. The max_pages is needed to support a use case where user only wants to get a specific number of pages. So there is no need to find all the pages of interest in the range when max_pages is specified. The IOCTL returns when the maximum number of the pages are found. The max_pages is optional. If max_pages is specified, it must be equal or greater than the vec_size. This restriction is needed to handle worse case when one page_region only contains info of one page and it cannot be compacted. This is needed to emulate the Windows getWriteWatch() syscall.
The patch series include the detailed selftest which can be used as an example for the uffd async wp test and PAGEMAP_IOCTL. It shows the interface usages as well.
[1] https://lore.kernel.org/lkml/54d4c322-cd6e-eefd-b161-2af2b56aae24@collabora.... [2] https://lore.kernel.org/all/20221220162606.1595355-1-usama.anjum@collabora.c... [3] https://lore.kernel.org/all/20221122115007.2787017-1-usama.anjum@collabora.c... [4] https://lore.kernel.org/all/Y6Hc2d+7eTKs7AiH@x1n [5] https://lore.kernel.org/all/YyiDg79flhWoMDZB@gmail.com/ [6] https://lore.kernel.org/all/20221014134802.1361436-1-mdanylo@google.com/
Regards, Muhammad Usama Anjum
Muhammad Usama Anjum (4): fs/proc/task_mmu: Implement IOCTL to get and optionally clear info about PTEs tools headers UAPI: Update linux/fs.h with the kernel sources mm/pagemap: add documentation of PAGEMAP_SCAN IOCTL selftests: mm: add pagemap ioctl tests
Peter Xu (1): userfaultfd: UFFD_FEATURE_WP_ASYNC
Documentation/admin-guide/mm/pagemap.rst | 58 + Documentation/admin-guide/mm/userfaultfd.rst | 35 + fs/proc/task_mmu.c | 513 ++++++ fs/userfaultfd.c | 26 +- include/linux/hugetlb.h | 1 + include/linux/userfaultfd_k.h | 21 +- include/uapi/linux/fs.h | 53 + include/uapi/linux/userfaultfd.h | 9 +- mm/hugetlb.c | 34 +- mm/memory.c | 27 +- tools/include/uapi/linux/fs.h | 53 + tools/testing/selftests/mm/.gitignore | 2 + tools/testing/selftests/mm/Makefile | 3 +- tools/testing/selftests/mm/config | 1 + tools/testing/selftests/mm/pagemap_ioctl.c | 1459 ++++++++++++++++++ tools/testing/selftests/mm/run_vmtests.sh | 4 + 16 files changed, 2275 insertions(+), 24 deletions(-) create mode 100644 tools/testing/selftests/mm/pagemap_ioctl.c mode change 100644 => 100755 tools/testing/selftests/mm/run_vmtests.sh
From: Peter Xu peterx@redhat.com
This patch adds a new userfaultfd-wp feature UFFD_FEATURE_WP_ASYNC, that allows userfaultfd wr-protect faults to be resolved by the kernel directly.
It can be used like a high accuracy version of soft-dirty, without vma modifications during tracking, and also with ranged support by default rather than for a whole mm when reset the protections due to existence of ioctl(UFFDIO_WRITEPROTECT).
Several goals of such a dirty tracking interface:
1. All types of memory should be supported and tracable. This is nature for soft-dirty but should mention when the context is userfaultfd, because it used to only support anon/shmem/hugetlb. The problem is for a dirty tracking purpose these three types may not be enough, and it's legal to track anything e.g. any page cache writes from mmap.
2. Protections can be applied to partial of a memory range, without vma split/merge fuss. The hope is that the tracking itself should not affect any vma layout change. It also helps when reset happens because the reset will not need mmap write lock which can block the tracee.
3. Accuracy needs to be maintained. This means we need pte markers to work on any type of VMA.
One could question that, the whole concept of async dirty tracking is not really close to fundamentally what userfaultfd used to be: it's not "a fault to be serviced by userspace" anymore. However, using userfaultfd-wp here as a framework is convenient for us in at least:
1. VM_UFFD_WP vma flag, which has a very good name to suite something like this, so we don't need VM_YET_ANOTHER_SOFT_DIRTY. Just use a new feature bit to identify from a sync version of uffd-wp registration.
2. PTE markers logic can be leveraged across the whole kernel to maintain the uffd-wp bit as long as an arch supports, this also applies to this case where uffd-wp bit will be a hint to dirty information and it will not go lost easily (e.g. when some page cache ptes got zapped).
3. Reuse ioctl(UFFDIO_WRITEPROTECT) interface for either starting or resetting a range of memory, while there's no counterpart in the old soft-dirty world, hence if this is wanted in a new design we'll need a new interface otherwise.
We can somehow understand that commonality because uffd-wp was fundamentally a similar idea of write-protecting pages just like soft-dirty.
This implementation allows WP_ASYNC to imply WP_UNPOPULATED, because so far WP_ASYNC seems to not usable if without WP_UNPOPULATE. This also gives us chance to modify impl of WP_ASYNC just in case it could be not depending on WP_UNPOPULATED anymore in the future kernels. It's also fine to imply that because both features will rely on PTE_MARKER_UFFD_WP config option, so they'll show up together (or both missing) in an UFFDIO_API probe.
vma_can_userfault() now allows any VMA if the userfaultfd registration is only about async uffd-wp. So we can track dirty for all kinds of memory including generic file systems (like XFS, EXT4 or BTRFS).
One trick worth mention in do_wp_page() is that we need to manually update vmf->orig_pte here because it can be used later with a pte_same() check - this path always has FAULT_FLAG_ORIG_PTE_VALID set in the flags.
The major defect of this approach of dirty tracking is we need to populate the pgtables when tracking starts. Soft-dirty doesn't do it like that. It's unwanted in the case where the range of memory to track is huge and unpopulated (e.g., tracking updates on a 10G file with mmap() on top, without having any page cache installed yet). One way to improve this is to allow pte markers exist for larger than PTE level for PMD+. That will not change the interface if to implemented, so we can leave that for later.
Co-developed-by: Muhammad Usama Anjum usama.anjum@collabora.com Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com Signed-off-by: Peter Xu peterx@redhat.com --- Changes in v18: - Rebase on top of next-20230613 (use ptep_get)
Changes in v17: - Rebase on top of next-20230525
Changes in v12: - Peter added the hugetlb support and revamped some other implementation - Transferred the authorship to Peter - Merge documentation to this patch
Changes in v11: - Fix return code in userfaultfd_register() and minor changes here and there - Rebase on top of next-20230307 - Base patches on UFFD_FEATURE_WP_UNPOPULATED https://lore.kernel.org/all/20230306213925.617814-1-peterx@redhat.com - UFFD_FEATURE_WP_ASYNC depends on UFFD_FEATURE_WP_UNPOPULATED to work (correctly)
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) --- Documentation/admin-guide/mm/userfaultfd.rst | 35 ++++++++++++++++++++ fs/userfaultfd.c | 26 ++++++++++++--- include/linux/userfaultfd_k.h | 21 +++++++++++- include/uapi/linux/userfaultfd.h | 9 ++++- mm/hugetlb.c | 32 ++++++++++-------- mm/memory.c | 27 +++++++++++++-- 6 files changed, 128 insertions(+), 22 deletions(-)
diff --git a/Documentation/admin-guide/mm/userfaultfd.rst b/Documentation/admin-guide/mm/userfaultfd.rst index 7c304e432205..4b7f43fbbe18 100644 --- a/Documentation/admin-guide/mm/userfaultfd.rst +++ b/Documentation/admin-guide/mm/userfaultfd.rst @@ -244,6 +244,41 @@ write-protected (so future writes will also result in a WP fault). These ioctls support a mode flag (``UFFDIO_COPY_MODE_WP`` or ``UFFDIO_CONTINUE_MODE_WP`` respectively) to configure the mapping this way.
+If the userfaultfd context has ``UFFD_FEATURE_WP_ASYNC`` feature bit set, +any vma registered with write-protection will work in async mode rather +than the default sync mode. + +In async mode, there will be no message generated when a write operation +happens, meanwhile the write-protection will be resolved automatically by +the kernel. It can be seen as a more accurate version of soft-dirty +tracking and it can be different in a few ways: + + - The dirty result will not be affected by vma changes (e.g. vma + merging) because the dirty is only tracked by the pte. + + - It supports range operations by default, so one can enable tracking on + any range of memory as long as page aligned. + + - Dirty information will not get lost if the pte was zapped due to + various reasons (e.g. during split of a shmem transparent huge page). + + - Due to a reverted meaning of soft-dirty (page clean when uffd-wp bit + set; dirty when uffd-wp bit cleared), it has different semantics on + some of the memory operations. For example: ``MADV_DONTNEED`` on + anonymous (or ``MADV_REMOVE`` on a file mapping) will be treated as + dirtying of memory by dropping uffd-wp bit during the procedure. + +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 not be under track of uffd-wp async mode until the page is +explicitly write-protected by ``ioctl(UFFDIO_WRITEPROTECT)`` 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. + +When userfaultfd-wp async mode is used alone, it can be applied to all +kinds of memory. + QEMU/KVM ========
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 7cecd49e078b..81b98e4a9f55 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -123,6 +123,11 @@ static bool userfaultfd_is_initialized(struct userfaultfd_ctx *ctx) return ctx->features & UFFD_FEATURE_INITIALIZED; }
+static bool userfaultfd_wp_async_ctx(struct userfaultfd_ctx *ctx) +{ + return ctx && (ctx->features & UFFD_FEATURE_WP_ASYNC); +} + /* * Whether WP_UNPOPULATED is enabled on the uffd context. It is only * meaningful when userfaultfd_wp()==true on the vma and when it's @@ -1322,6 +1327,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, bool basic_ioctls; unsigned long start, end, vma_end; struct vma_iterator vmi; + bool wp_async = userfaultfd_wp_async_ctx(ctx); pgoff_t pgoff;
user_uffdio_register = (struct uffdio_register __user *) arg; @@ -1396,7 +1402,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx,
/* check not compatible vmas */ ret = -EINVAL; - if (!vma_can_userfault(cur, vm_flags)) + if (!vma_can_userfault(cur, vm_flags, wp_async)) goto out_unlock;
/* @@ -1457,7 +1463,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, for_each_vma_range(vmi, vma, end) { cond_resched();
- BUG_ON(!vma_can_userfault(vma, vm_flags)); + BUG_ON(!vma_can_userfault(vma, vm_flags, wp_async)); BUG_ON(vma->vm_userfaultfd_ctx.ctx && vma->vm_userfaultfd_ctx.ctx != ctx); WARN_ON(!(vma->vm_flags & VM_MAYWRITE)); @@ -1557,6 +1563,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx, unsigned long start, end, vma_end; const void __user *buf = (void __user *)arg; struct vma_iterator vmi; + bool wp_async = userfaultfd_wp_async_ctx(ctx); pgoff_t pgoff;
ret = -EFAULT; @@ -1611,7 +1618,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx, * provides for more strict behavior to notice * unregistration errors. */ - if (!vma_can_userfault(cur, cur->vm_flags)) + if (!vma_can_userfault(cur, cur->vm_flags, wp_async)) goto out_unlock;
found = true; @@ -1627,7 +1634,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx, for_each_vma_range(vmi, vma, end) { cond_resched();
- BUG_ON(!vma_can_userfault(vma, vma->vm_flags)); + BUG_ON(!vma_can_userfault(vma, vma->vm_flags, wp_async));
/* * Nothing to do: this vma is already registered into this @@ -1965,6 +1972,11 @@ static int userfaultfd_continue(struct userfaultfd_ctx *ctx, unsigned long arg) return ret; }
+bool userfaultfd_wp_async(struct vm_area_struct *vma) +{ + return userfaultfd_wp_async_ctx(vma->vm_userfaultfd_ctx.ctx); +} + static inline unsigned int uffd_ctx_features(__u64 user_features) { /* @@ -1998,6 +2010,11 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx, ret = -EPERM; if ((features & UFFD_FEATURE_EVENT_FORK) && !capable(CAP_SYS_PTRACE)) goto err_out; + + /* WP_ASYNC relies on WP_UNPOPULATED, choose it unconditionally */ + if (features & UFFD_FEATURE_WP_ASYNC) + features |= UFFD_FEATURE_WP_UNPOPULATED; + /* report all available features and ioctls to userland */ uffdio_api.features = UFFD_API_FEATURES; #ifndef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR @@ -2010,6 +2027,7 @@ static int userfaultfd_api(struct userfaultfd_ctx *ctx, #ifndef CONFIG_PTE_MARKER_UFFD_WP uffdio_api.features &= ~UFFD_FEATURE_WP_HUGETLBFS_SHMEM; uffdio_api.features &= ~UFFD_FEATURE_WP_UNPOPULATED; + uffdio_api.features &= ~UFFD_FEATURE_WP_ASYNC; #endif uffdio_api.ioctls = UFFD_API_IOCTLS; ret = -EFAULT; diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h index ac7b0c96d351..3b684770c3f8 100644 --- a/include/linux/userfaultfd_k.h +++ b/include/linux/userfaultfd_k.h @@ -157,11 +157,22 @@ static inline bool userfaultfd_armed(struct vm_area_struct *vma) }
static inline bool vma_can_userfault(struct vm_area_struct *vma, - unsigned long vm_flags) + unsigned long vm_flags, + bool wp_async) { + vm_flags &= __VM_UFFD_FLAGS; + if ((vm_flags & VM_UFFD_MINOR) && (!is_vm_hugetlb_page(vma) && !vma_is_shmem(vma))) return false; + + /* + * If wp async enabled, and WP is the only mode enabled, allow any + * memory type. + */ + if (wp_async && (vm_flags == VM_UFFD_WP)) + return true; + #ifndef CONFIG_PTE_MARKER_UFFD_WP /* * If user requested uffd-wp but not enabled pte markers for @@ -171,6 +182,8 @@ static inline bool vma_can_userfault(struct vm_area_struct *vma, if ((vm_flags & VM_UFFD_WP) && !vma_is_anonymous(vma)) return false; #endif + + /* By default, allow any of anon|shmem|hugetlb */ return vma_is_anonymous(vma) || is_vm_hugetlb_page(vma) || vma_is_shmem(vma); } @@ -193,6 +206,7 @@ extern int userfaultfd_unmap_prep(struct vm_area_struct *vma, extern void userfaultfd_unmap_complete(struct mm_struct *mm, struct list_head *uf); extern bool userfaultfd_wp_unpopulated(struct vm_area_struct *vma); +extern bool userfaultfd_wp_async(struct vm_area_struct *vma);
#else /* CONFIG_USERFAULTFD */
@@ -293,6 +307,11 @@ static inline bool userfaultfd_wp_unpopulated(struct vm_area_struct *vma) return false; }
+static inline bool userfaultfd_wp_async(struct vm_area_struct *vma) +{ + return false; +} + #endif /* CONFIG_USERFAULTFD */
static inline bool userfaultfd_wp_use_markers(struct vm_area_struct *vma) diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h index 66dd4cd277bd..cfb87a112a9f 100644 --- a/include/uapi/linux/userfaultfd.h +++ b/include/uapi/linux/userfaultfd.h @@ -39,7 +39,8 @@ UFFD_FEATURE_MINOR_SHMEM | \ UFFD_FEATURE_EXACT_ADDRESS | \ UFFD_FEATURE_WP_HUGETLBFS_SHMEM | \ - UFFD_FEATURE_WP_UNPOPULATED) + UFFD_FEATURE_WP_UNPOPULATED | \ + UFFD_FEATURE_WP_ASYNC) #define UFFD_API_IOCTLS \ ((__u64)1 << _UFFDIO_REGISTER | \ (__u64)1 << _UFFDIO_UNREGISTER | \ @@ -210,6 +211,11 @@ struct uffdio_api { * (i.e. empty ptes). This will be the default behavior for shmem * & hugetlbfs, so this flag only affects anonymous memory behavior * when userfault write-protection mode is registered. + * + * 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 implies UFFD_FEATURE_WP_UNPOPULATED. */ #define UFFD_FEATURE_PAGEFAULT_FLAG_WP (1<<0) #define UFFD_FEATURE_EVENT_FORK (1<<1) @@ -225,6 +231,7 @@ struct uffdio_api { #define UFFD_FEATURE_EXACT_ADDRESS (1<<11) #define UFFD_FEATURE_WP_HUGETLBFS_SHMEM (1<<12) #define UFFD_FEATURE_WP_UNPOPULATED (1<<13) +#define UFFD_FEATURE_WP_ASYNC (1<<14) __u64 features;
__u64 ioctls; diff --git a/mm/hugetlb.c b/mm/hugetlb.c index d76574425da3..0db13167b1ee 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -6156,21 +6156,27 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma, /* Handle userfault-wp first, before trying to lock more pages */ if (userfaultfd_wp(vma) && huge_pte_uffd_wp(huge_ptep_get(ptep)) && (flags & FAULT_FLAG_WRITE) && !huge_pte_write(entry)) { - struct vm_fault vmf = { - .vma = vma, - .address = haddr, - .real_address = address, - .flags = flags, - }; + if (!userfaultfd_wp_async(vma)) { + struct vm_fault vmf = { + .vma = vma, + .address = haddr, + .real_address = address, + .flags = flags, + };
- spin_unlock(ptl); - if (pagecache_folio) { - folio_unlock(pagecache_folio); - folio_put(pagecache_folio); + spin_unlock(ptl); + if (pagecache_folio) { + folio_unlock(pagecache_folio); + folio_put(pagecache_folio); + } + hugetlb_vma_unlock_read(vma); + mutex_unlock(&hugetlb_fault_mutex_table[hash]); + return handle_userfault(&vmf, VM_UFFD_WP); } - hugetlb_vma_unlock_read(vma); - mutex_unlock(&hugetlb_fault_mutex_table[hash]); - return handle_userfault(&vmf, VM_UFFD_WP); + + entry = huge_pte_clear_uffd_wp(entry); + set_huge_pte_at(mm, haddr, ptep, entry); + /* Fallthrough to CoW */ }
/* diff --git a/mm/memory.c b/mm/memory.c index e7046cc88596..cf75d2943538 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3340,11 +3340,28 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf) const bool unshare = vmf->flags & FAULT_FLAG_UNSHARE; struct vm_area_struct *vma = vmf->vma; struct folio *folio = NULL; + pte_t pte;
if (likely(!unshare)) { if (userfaultfd_pte_wp(vma, ptep_get(vmf->pte))) { - pte_unmap_unlock(vmf->pte, vmf->ptl); - return handle_userfault(vmf, VM_UFFD_WP); + if (!userfaultfd_wp_async(vma)) { + pte_unmap_unlock(vmf->pte, vmf->ptl); + return handle_userfault(vmf, VM_UFFD_WP); + } + + /* + * Nothing needed (cache flush, TLB invalidations, + * etc.) because we're only removing the uffd-wp bit, + * which is completely invisible to the user. + */ + pte = pte_clear_uffd_wp(ptep_get(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; }
/* @@ -4824,8 +4841,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); }
@@ -4837,6 +4857,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);
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). - Find pages which have been written-to and/or write protect the pages (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP)
This IOCTL can be extended to get information about more PTE bits.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- Changes in v18: - Rebased on top of next-20230613 - ptep_get() updates - remove pmd_trans_unstable() and add ACTION_AGAIN - Review updates (Micheal)
Changes in v17: - Rebased on next-20230606 - Made make_uffd_wp_*_pte() better and minor changes
Changes in v16: - Fixed a corner case where kernel writes beyond user buffer by one element - Bring back exclusive PM_SCAN_OP_WP - Cosmetic changes
Changes in v15: - Build fix: - Use generic tlb flush function in pagemap_scan_pmd_entry() instead of using x86 specific flush function in do_pagemap_scan() - Remove #ifdef from pagemap_scan_hugetlb_entry() - Use mm instead of undefined vma->vm_mm
Changes in v14: - Fix build error caused by #ifdef added at last minute in some configs
Changes in v13: - Review updates - mmap_read_lock_killable() instead of mmap_read_lock() - Replace uffd_wp_range() with helpers which increases performance drastically for OP_WP operations by reducing the number of tlb flushing etc - Add MMU_NOTIFY_PROTECTION_VMA notification for the memory range
Changes in v12: - Add hugetlb support to cover all memory types - Merge "userfaultfd: Define dummy uffd_wp_range()" with this patch - Review updates to the code
Changes in v11: - Find written pages in a better way - Fix a corner case (thanks Paul) - Improve the code/comments - remove ENGAGE_WP + ! GET operation - shorten the commit message in favour of moving documentation to pagemap.rst
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 | 513 ++++++++++++++++++++++++++++++++++++++++ include/linux/hugetlb.h | 1 + include/uapi/linux/fs.h | 53 +++++ mm/hugetlb.c | 2 +- 4 files changed, 568 insertions(+), 1 deletion(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 507cd4e59d07..0d49e14e97aa 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> @@ -1765,11 +1766,523 @@ static int pagemap_release(struct inode *inode, struct file *file) return 0; }
+#define PM_SCAN_OP_WRITE BIT(63) +#define PM_SCAN_FOUND_MAX_PAGES (1) +#define PM_SCAN_BITS_ALL (PAGE_IS_WRITTEN | PAGE_IS_FILE | \ + PAGE_IS_PRESENT | PAGE_IS_SWAPPED) +#define PM_SCAN_OPS (PM_SCAN_OP_GET | PM_SCAN_OP_WP) +#define IS_PM_SCAN_GET(flags) (flags & PM_SCAN_OP_GET) +#define IS_PM_SCAN_WP(flags) (flags & PM_SCAN_OP_WP) +#define PM_SCAN_BITMAP(wt, file, present, swap) \ + ((wt) | ((file) << 1) | ((present) << 2) | ((swap) << 3)) + +struct pagemap_scan_private { + struct page_region *vec_buf, cur_buf; + unsigned long vec_buf_len, vec_buf_index, max_pages, found_pages, flags; + unsigned long required_mask, anyof_mask, excluded_mask, return_mask; +}; + +static inline bool is_pte_uffd_wp(pte_t pte) +{ + return (pte_present(pte) && pte_uffd_wp(pte)) || + pte_swp_uffd_wp_any(pte); +} + +static inline void make_uffd_wp_pte(struct vm_area_struct *vma, + unsigned long addr, pte_t *pte) +{ + pte_t ptent = ptep_get(pte); + + if (pte_present(ptent)) { + pte_t old_pte; + + old_pte = ptep_modify_prot_start(vma, addr, pte); + ptent = pte_mkuffd_wp(ptent); + ptep_modify_prot_commit(vma, addr, pte, old_pte, ptent); + } else if (is_swap_pte(ptent)) { + ptent = pte_swp_mkuffd_wp(ptent); + set_pte_at(vma->vm_mm, addr, pte, ptent); + } else { + set_pte_at(vma->vm_mm, addr, pte, + make_pte_marker(PTE_MARKER_UFFD_WP)); + } +} + +#ifdef CONFIG_TRANSPARENT_HUGEPAGE +static inline bool is_pmd_uffd_wp(pmd_t pmd) +{ + return (pmd_present(pmd) && pmd_uffd_wp(pmd)) || + (is_swap_pmd(pmd) && pmd_swp_uffd_wp(pmd)); +} + +static inline void make_uffd_wp_pmd(struct vm_area_struct *vma, + unsigned long addr, pmd_t *pmdp) +{ + pmd_t old, pmd = *pmdp; + + if (pmd_present(pmd)) { + old = pmdp_invalidate_ad(vma, addr, pmdp); + pmd = pmd_mkuffd_wp(old); + set_pmd_at(vma->vm_mm, addr, pmdp, pmd); + } else if (is_migration_entry(pmd_to_swp_entry(pmd))) { + pmd = pmd_swp_mkuffd_wp(pmd); + set_pmd_at(vma->vm_mm, addr, pmdp, pmd); + } +} +#endif + +#ifdef CONFIG_HUGETLB_PAGE +static inline bool is_huge_pte_uffd_wp(pte_t pte) +{ + return (pte_present(pte) && huge_pte_uffd_wp(pte)) || + pte_swp_uffd_wp_any(pte); +} + +static inline void make_uffd_wp_huge_pte(struct vm_area_struct *vma, + unsigned long addr, pte_t *ptep, + pte_t ptent) +{ + if (is_hugetlb_entry_hwpoisoned(ptent) || is_pte_marker(ptent)) + return; + + if (is_hugetlb_entry_migration(ptent)) + set_huge_pte_at(vma->vm_mm, addr, ptep, + pte_swp_mkuffd_wp(ptent)); + else if (!huge_pte_none(ptent)) + huge_ptep_modify_prot_commit(vma, addr, ptep, ptent, + huge_pte_mkuffd_wp(ptent)); + else + set_huge_pte_at(vma->vm_mm, addr, ptep, + make_pte_marker(PTE_MARKER_UFFD_WP)); +} +#endif + +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 ((p->flags & PM_SCAN_OP_WRITE) && (!userfaultfd_wp_async(vma) || + !userfaultfd_wp_use_markers(vma))) + return -EPERM; + + if (vma->vm_flags & VM_PFNMAP) + return 1; + + return 0; +} + +static int pagemap_scan_output(bool wt, bool file, bool pres, bool swap, + struct pagemap_scan_private *p, + unsigned long addr, unsigned int n_pages) +{ + unsigned long bitmap = PM_SCAN_BITMAP(wt, file, pres, swap); + struct page_region *cur_buf = &p->cur_buf; + + if (!n_pages) + return -EINVAL; + + if ((p->required_mask & bitmap) != p->required_mask) + return 0; + if (p->anyof_mask && !(p->anyof_mask & bitmap)) + return 0; + if (p->excluded_mask & bitmap) + return 0; + + bitmap &= p->return_mask; + if (!bitmap) + return 0; + + if (cur_buf->bitmap == bitmap && + cur_buf->start + cur_buf->len * PAGE_SIZE == addr) { + cur_buf->len += n_pages; + p->found_pages += n_pages; + } else { + if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len) + return -ENOMEM; + + if (cur_buf->len) { + memcpy(&p->vec_buf[p->vec_buf_index], cur_buf, + sizeof(*p->vec_buf)); + p->vec_buf_index++; + } + + cur_buf->start = addr; + cur_buf->len = n_pages; + cur_buf->bitmap = bitmap; + p->found_pages += n_pages; + } + + if (p->max_pages && (p->found_pages == p->max_pages)) + return PM_SCAN_FOUND_MAX_PAGES; + + return 0; +} + +static 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; + bool is_written, flush = false; + pte_t *pte, *orig_pte, ptent; + unsigned long addr = end; + spinlock_t *ptl; + int ret = 0; + + arch_enter_lazy_mmu_mode(); + +#ifdef CONFIG_TRANSPARENT_HUGEPAGE + ptl = pmd_trans_huge_lock(pmd, vma); + if (ptl) { + unsigned long n_pages = (end - start)/PAGE_SIZE; + + if (p->max_pages && n_pages > p->max_pages - p->found_pages) + n_pages = p->max_pages - p->found_pages; + + is_written = !is_pmd_uffd_wp(*pmd); + + /* + * Break huge page into small pages if the WP operation need to + * be performed is on a portion of the huge page. + */ + if (is_written && IS_PM_SCAN_WP(p->flags) && + n_pages < HPAGE_SIZE/PAGE_SIZE) { + spin_unlock(ptl); + + split_huge_pmd(vma, pmd, start); + goto process_smaller_pages; + } + + if (IS_PM_SCAN_GET(p->flags)) + ret = pagemap_scan_output(is_written, vma->vm_file, + pmd_present(*pmd), + is_swap_pmd(*pmd), + p, start, n_pages); + + if (ret >= 0 && is_written && IS_PM_SCAN_WP(p->flags)) + make_uffd_wp_pmd(vma, addr, pmd); + + if (is_written && IS_PM_SCAN_WP(p->flags)) + flush_tlb_range(vma, start, end); + + spin_unlock(ptl); + + arch_leave_lazy_mmu_mode(); + return ret; + } + +process_smaller_pages: +#endif + + orig_pte = pte = pte_offset_map_lock(vma->vm_mm, pmd, start, &ptl); + if (!pte) { + walk->action = ACTION_AGAIN; + return 0; + } + for (addr = start; addr < end && !ret; pte++, addr += PAGE_SIZE) { + ptent = ptep_get(pte); + is_written = !is_pte_uffd_wp(ptent); + + if (IS_PM_SCAN_GET(p->flags)) + ret = pagemap_scan_output(is_written, vma->vm_file, + pte_present(ptent), + is_swap_pte(ptent), + p, addr, 1); + + if (ret >= 0 && is_written && IS_PM_SCAN_WP(p->flags)) { + make_uffd_wp_pte(vma, addr, pte); + flush = true; + } + } + + if (flush) + flush_tlb_range(vma, start, addr); + + pte_unmap_unlock(orig_pte, ptl); + arch_leave_lazy_mmu_mode(); + + cond_resched(); + return ret; +} + +#ifdef CONFIG_HUGETLB_PAGE +static int pagemap_scan_hugetlb_entry(pte_t *ptep, unsigned long hmask, + unsigned long start, unsigned long end, + struct mm_walk *walk) +{ + unsigned long n_pages = (end - start)/PAGE_SIZE; + struct pagemap_scan_private *p = walk->private; + struct vm_area_struct *vma = walk->vma; + struct hstate *h = hstate_vma(vma); + spinlock_t *ptl; + bool is_written; + int ret = 0; + pte_t ptent; + + if (IS_PM_SCAN_WP(p->flags) && n_pages < HPAGE_SIZE/PAGE_SIZE) + return -EINVAL; + + if (p->max_pages && n_pages > p->max_pages - p->found_pages) + n_pages = p->max_pages - p->found_pages; + + if (IS_PM_SCAN_WP(p->flags)) { + i_mmap_lock_write(vma->vm_file->f_mapping); + ptl = huge_pte_lock(h, vma->vm_mm, ptep); + } + + ptent = huge_ptep_get(ptep); + is_written = !is_huge_pte_uffd_wp(ptent); + + /* + * Partial hugetlb page clear isn't supported + */ + if (is_written && IS_PM_SCAN_WP(p->flags) && + n_pages < HPAGE_SIZE/PAGE_SIZE) { + ret = -ENOSPC; + goto unlock_and_return; + } + + if (IS_PM_SCAN_GET(p->flags)) { + ret = pagemap_scan_output(is_written, vma->vm_file, + pte_present(ptent), is_swap_pte(ptent), + p, start, n_pages); + if (ret < 0) + goto unlock_and_return; + } + + if (is_written && IS_PM_SCAN_WP(p->flags)) { + make_uffd_wp_huge_pte(vma, start, ptep, ptent); + flush_hugetlb_tlb_range(vma, start, end); + } + +unlock_and_return: + if (IS_PM_SCAN_WP(p->flags)) { + spin_unlock(ptl); + i_mmap_unlock_write(vma->vm_file->f_mapping); + } + + return ret; +} +#else +#define pagemap_scan_hugetlb_entry NULL +#endif + +static int pagemap_scan_pte_hole(unsigned long addr, unsigned long end, + int depth, struct mm_walk *walk) +{ + unsigned long n_pages = (end - addr)/PAGE_SIZE; + struct pagemap_scan_private *p = walk->private; + struct vm_area_struct *vma = walk->vma; + int ret = 0; + + if (!vma || !IS_PM_SCAN_GET(p->flags)) + return 0; + + if (p->max_pages && n_pages > p->max_pages - p->found_pages) + n_pages = p->max_pages - p->found_pages; + + ret = pagemap_scan_output(false, vma->vm_file, false, false, p, addr, + n_pages); + + return ret; +} + +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, + .hugetlb_entry = pagemap_scan_hugetlb_entry, +}; + +static inline unsigned long pagemap_scan_check_page_written(struct pagemap_scan_private *p) +{ + return ((p->required_mask | p->anyof_mask | p->excluded_mask) & + PAGE_IS_WRITTEN) ? PM_SCAN_OP_WRITE : 0; +} + +static int pagemap_scan_args_valid(struct pm_scan_arg *arg, unsigned long start, + struct page_region __user *vec) +{ + /* Detect illegal size, flags, len and masks */ + if (arg->size != sizeof(struct pm_scan_arg)) + return -EINVAL; + if (arg->flags & ~PM_SCAN_OPS) + return -EINVAL; + if (!arg->len) + return -EINVAL; + if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask | + arg->return_mask) & ~PM_SCAN_BITS_ALL) + return -EINVAL; + if (!arg->required_mask && !arg->anyof_mask && + !arg->excluded_mask) + return -EINVAL; + if (!arg->return_mask) + return -EINVAL; + + /* Validate memory range */ + if (!IS_ALIGNED(start, PAGE_SIZE)) + return -EINVAL; + if (!access_ok((void __user *)start, arg->len)) + return -EFAULT; + + if (IS_PM_SCAN_GET(arg->flags)) { + if (!arg->vec) + return -EINVAL; + if (arg->vec_len == 0) + return -EINVAL; + } + + if (IS_PM_SCAN_WP(arg->flags)) { + if (!IS_PM_SCAN_GET(arg->flags) && arg->max_pages) + return -EINVAL; + + if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask) & + ~PAGE_IS_WRITTEN) + return -EINVAL; + } + + return 0; +} + +static long do_pagemap_scan(struct mm_struct *mm, unsigned long __arg) +{ + struct pm_scan_arg __user *uarg = (struct pm_scan_arg __user *)__arg; + unsigned long start, end, walk_start, walk_end; + unsigned long empty_slots, vec_index = 0; + struct mmu_notifier_range range; + struct page_region __user *vec; + struct pagemap_scan_private p; + struct pm_scan_arg arg; + int ret = 0; + + if (copy_from_user(&arg, uarg, sizeof(arg))) + return -EFAULT; + + start = untagged_addr((unsigned long)arg.start); + vec = (struct page_region *)untagged_addr((unsigned long)arg.vec); + + ret = pagemap_scan_args_valid(&arg, start, vec); + if (ret) + return ret; + + end = start + arg.len; + p.max_pages = arg.max_pages; + p.found_pages = 0; + 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.flags = arg.flags; + p.flags |= pagemap_scan_check_page_written(&p); + p.cur_buf.start = p.cur_buf.len = p.cur_buf.bitmap = 0; + p.vec_buf = NULL; + p.vec_buf_len = PAGEMAP_WALK_SIZE >> PAGE_SHIFT; + + /* + * Allocate smaller buffer to get output from inside the page walk + * functions and walk page range in PAGEMAP_WALK_SIZE size chunks. As + * we want to return output to user in compact form where no two + * consecutive regions should be continuous and have the same flags. + * So store the latest element in p.cur_buf between different walks and + * store the p.cur_buf at the end of the walk to the user buffer. + */ + if (IS_PM_SCAN_GET(p.flags)) { + p.vec_buf = kmalloc_array(p.vec_buf_len, sizeof(*p.vec_buf), + GFP_KERNEL); + if (!p.vec_buf) + return -ENOMEM; + } + + if (IS_PM_SCAN_WP(p.flags)) { + mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_VMA, 0, + mm, start, end); + mmu_notifier_invalidate_range_start(&range); + } + + walk_start = walk_end = start; + while (walk_end < end && !ret) { + if (IS_PM_SCAN_GET(p.flags)) { + p.vec_buf_index = 0; + + /* + * All data is copied to cur_buf first. When more data + * is found, we push cur_buf to vec_buf and copy new + * data to cur_buf. Subtract 1 from length as the + * index of cur_buf isn't counted in length. + */ + empty_slots = arg.vec_len - vec_index; + p.vec_buf_len = min(p.vec_buf_len, empty_slots - 1); + } + + walk_end = (walk_start + PAGEMAP_WALK_SIZE) & PAGEMAP_WALK_MASK; + if (walk_end > end) + walk_end = end; + + ret = mmap_read_lock_killable(mm); + if (ret) + goto free_data; + ret = walk_page_range(mm, walk_start, walk_end, + &pagemap_scan_ops, &p); + mmap_read_unlock(mm); + + if (ret && ret != -ENOMEM && ret != PM_SCAN_FOUND_MAX_PAGES) + goto free_data; + + walk_start = walk_end; + if (IS_PM_SCAN_GET(p.flags) && p.vec_buf_index) { + if (copy_to_user(&vec[vec_index], p.vec_buf, + p.vec_buf_index * sizeof(*p.vec_buf))) { + /* + * Return error even though the OP succeeded + */ + ret = -EFAULT; + goto free_data; + } + vec_index += p.vec_buf_index; + } + } + + if (p.cur_buf.len) { + if (copy_to_user(&vec[vec_index], &p.cur_buf, sizeof(p.cur_buf))) { + ret = -EFAULT; + goto free_data; + } + vec_index++; + } + + ret = vec_index; + +free_data: + if (IS_PM_SCAN_WP(p.flags)) + mmu_notifier_invalidate_range_end(&range); + + kfree(p.vec_buf); + return ret; +} + +static long do_pagemap_cmd(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct mm_struct *mm = file->private_data; + + switch (cmd) { + case PAGEMAP_SCAN: + return do_pagemap_scan(mm, arg); + + default: + 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 = do_pagemap_cmd, + .compat_ioctl = do_pagemap_cmd, }; #endif /* CONFIG_PROC_PAGE_MONITOR */
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index beb7c63d2871..a6e773c3e2b4 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -261,6 +261,7 @@ long hugetlb_change_protection(struct vm_area_struct *vma, unsigned long cp_flags);
bool is_hugetlb_entry_migration(pte_t pte); +bool is_hugetlb_entry_hwpoisoned(pte_t pte); void hugetlb_unshare_all_pmds(struct vm_area_struct *vma);
#else /* !CONFIG_HUGETLB_PAGE */ diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index b7b56871029c..47879c38ce2f 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -305,4 +305,57 @@ 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 pm_scan_arg) + +/* Bits are set in the bitmap of the page_region and masks in pm_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 in pages + * bitmap: Bits sets for the region + */ +struct page_region { + __u64 start; + __u64 len; + __u64 bitmap; +}; + +/* + * struct pm_scan_arg - Pagemap ioctl argument + * @size: Size of the structure + * @flags: Flags for the IOCTL + * @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 + * @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 pm_scan_arg { + __u64 size; + __u64 flags; + __u64 start; + __u64 len; + __u64 vec; + __u64 vec_len; + __u64 max_pages; + __u64 required_mask; + __u64 anyof_mask; + __u64 excluded_mask; + __u64 return_mask; +}; + +/* Supported flags */ +#define PM_SCAN_OP_GET (1 << 0) +#define PM_SCAN_OP_WP (1 << 1) + #endif /* _UAPI_LINUX_FS_H */ diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 0db13167b1ee..7e60f0f3fd03 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -4980,7 +4980,7 @@ bool is_hugetlb_entry_migration(pte_t pte) return false; }
-static bool is_hugetlb_entry_hwpoisoned(pte_t pte) +bool is_hugetlb_entry_hwpoisoned(pte_t pte) { swp_entry_t swp;
Hi Muhammad,
kernel test robot noticed the following build warnings:
[auto build test WARNING on akpm-mm/mm-everything] [also build test WARNING on next-20230613] [cannot apply to linus/master v6.4-rc6] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Muhammad-Usama-Anjum/userfaul... base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything patch link: https://lore.kernel.org/r/20230613102905.2808371-3-usama.anjum%40collabora.c... patch subject: [PATCH v18 2/5] fs/proc/task_mmu: Implement IOCTL to get and optionally clear info about PTEs config: hexagon-randconfig-r041-20230612 (https://download.01.org/0day-ci/archive/20230613/202306132324.Npiix2ct-lkp@i...) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce (this is a W=1 build): mkdir -p ~/bin wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git remote add akpm-mm https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git git fetch akpm-mm mm-everything git checkout akpm-mm/mm-everything b4 shazam https://lore.kernel.org/r/20230613102905.2808371-3-usama.anjum@collabora.com # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash fs/proc/
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202306132324.Npiix2ct-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from fs/proc/task_mmu.c:3: In file included from include/linux/mm_inline.h:7: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:26: In file included from include/linux/kernel_stat.h:9: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:334: include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 547 | val = __raw_readb(PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) | ^ In file included from fs/proc/task_mmu.c:3: In file included from include/linux/mm_inline.h:7: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:26: In file included from include/linux/kernel_stat.h:9: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:334: include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) | ^ In file included from fs/proc/task_mmu.c:3: In file included from include/linux/mm_inline.h:7: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:13: In file included from include/linux/cgroup.h:26: In file included from include/linux/kernel_stat.h:9: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/hexagon/include/asm/io.h:334: include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 584 | __raw_writeb(value, PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^
fs/proc/task_mmu.c:1866:18: warning: shift count >= width of type [-Wshift-count-overflow]
1866 | if ((p->flags & PM_SCAN_OP_WRITE) && (!userfaultfd_wp_async(vma) || | ^~~~~~~~~~~~~~~~ fs/proc/task_mmu.c:1769:26: note: expanded from macro 'PM_SCAN_OP_WRITE' 1769 | #define PM_SCAN_OP_WRITE BIT(63) | ^~~~~~~ include/vdso/bits.h:7:26: note: expanded from macro 'BIT' 7 | #define BIT(nr) (UL(1) << (nr)) | ^ ~~~~ fs/proc/task_mmu.c:2102:22: warning: shift count >= width of type [-Wshift-count-overflow] 2102 | PAGE_IS_WRITTEN) ? PM_SCAN_OP_WRITE : 0; | ^~~~~~~~~~~~~~~~ fs/proc/task_mmu.c:1769:26: note: expanded from macro 'PM_SCAN_OP_WRITE' 1769 | #define PM_SCAN_OP_WRITE BIT(63) | ^~~~~~~ include/vdso/bits.h:7:26: note: expanded from macro 'BIT' 7 | #define BIT(nr) (UL(1) << (nr)) | ^ ~~~~ 8 warnings generated.
vim +1866 fs/proc/task_mmu.c
1859 1860 static int pagemap_scan_test_walk(unsigned long start, unsigned long end, 1861 struct mm_walk *walk) 1862 { 1863 struct pagemap_scan_private *p = walk->private; 1864 struct vm_area_struct *vma = walk->vma; 1865
1866 if ((p->flags & PM_SCAN_OP_WRITE) && (!userfaultfd_wp_async(vma) ||
1867 !userfaultfd_wp_use_markers(vma))) 1868 return -EPERM; 1869 1870 if (vma->vm_flags & VM_PFNMAP) 1871 return 1; 1872 1873 return 0; 1874 } 1875
On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum usama.anjum@collabora.com 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).
- Find pages which have been written-to and/or write protect the pages (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP)
This IOCTL can be extended to get information about more PTE bits.
[...]
--- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c
[...]
+static int pagemap_scan_output(bool wt, bool file, bool pres, bool swap,
struct pagemap_scan_private *p,unsigned long addr, unsigned int n_pages)+{
unsigned long bitmap = PM_SCAN_BITMAP(wt, file, pres, swap);struct page_region *cur_buf = &p->cur_buf;
Maybe we can go a step further and say here `cur_buf = &p->vec_buf[p->vec_buf_index];` and remove `p->cur_buf` entirely?
if (!n_pages)return -EINVAL;if ((p->required_mask & bitmap) != p->required_mask)return 0;if (p->anyof_mask && !(p->anyof_mask & bitmap))return 0;if (p->excluded_mask & bitmap)return 0;bitmap &= p->return_mask;if (!bitmap)return 0;if (cur_buf->bitmap == bitmap &&cur_buf->start + cur_buf->len * PAGE_SIZE == addr) {cur_buf->len += n_pages;p->found_pages += n_pages;} else {if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len)return -ENOMEM;
Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel ran out of memory when allocating, not that there is no space in a user-provided buffer.
BTW, the check could be inside the if() below for easier reading and less redundancy.
if (cur_buf->len) {memcpy(&p->vec_buf[p->vec_buf_index], cur_buf,sizeof(*p->vec_buf));p->vec_buf_index++;}cur_buf->start = addr;cur_buf->len = n_pages;cur_buf->bitmap = bitmap;p->found_pages += n_pages;}if (p->max_pages && (p->found_pages == p->max_pages))
Since `p->found_pages > 0` holds here, the first check is redundant. Nit: the parentheses around == are not needed.
return PM_SCAN_FOUND_MAX_PAGES;return 0;+}
[...]
+static inline unsigned long pagemap_scan_check_page_written(struct pagemap_scan_private *p) +{
return ((p->required_mask | p->anyof_mask | p->excluded_mask) &PAGE_IS_WRITTEN) ? PM_SCAN_OP_WRITE : 0;+}
Please inline at the single callsite.
For flags name: PM_REQUIRE_WRITE_ACCESS? Or Is it intended to be checked only if doing WP (as the current name suggests) and so it would be redundant as WP currently requires `p->required_mask = PAGE_IS_WRITTEN`?
+static int pagemap_scan_args_valid(struct pm_scan_arg *arg, unsigned long start,
struct page_region __user *vec)+{
/* Detect illegal size, flags, len and masks */if (arg->size != sizeof(struct pm_scan_arg))return -EINVAL;if (arg->flags & ~PM_SCAN_OPS)return -EINVAL;if (!arg->len)return -EINVAL;if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask |arg->return_mask) & ~PM_SCAN_BITS_ALL)return -EINVAL;if (!arg->required_mask && !arg->anyof_mask &&!arg->excluded_mask)return -EINVAL;if (!arg->return_mask)return -EINVAL;
I just noticed that `!arg->return_mask == !IS_PM_SCAN_GET()`. So the OP_GET is redundant and can be removed from the `flags` if checking `return_mask` instead. That way there won't be a "flags-encoded ops" thing as it would be a 'scan' with optional 'write-protect'.
/* Validate memory range */if (!IS_ALIGNED(start, PAGE_SIZE))return -EINVAL;if (!access_ok((void __user *)start, arg->len))return -EFAULT;if (IS_PM_SCAN_GET(arg->flags)) {if (!arg->vec)return -EINVAL;
access_ok() -> -EFAULT (an early fail-check) (the vec_len should be checked first then, failing with -EINVAL if 0)
if (arg->vec_len == 0)return -EINVAL;}if (IS_PM_SCAN_WP(arg->flags)) {if (!IS_PM_SCAN_GET(arg->flags) && arg->max_pages)return -EINVAL;if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask) &~PAGE_IS_WRITTEN)
Is `excluded_mask = PAGE_IS_WRITTEN` intended to be allowed? It would make WP do nothing, unless the required/anyof/excluded masks are not supposed to limit WP?
return -EINVAL;}
If `flags == 0` (and `return_mask == 0` in case my earlier comment is applied) it should fail with -EINVAL.
[...]
--- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h +/*
- struct page_region - Page region with bitmap flags
- @start: Start of the region
- @len: Length of the region in pages
- bitmap: Bits sets for the region
'@' is missing for the third field. BTW, maybe we can call it something like `flags` or `category` (something that hints at the meaning of the value instead of its data representation).
+/*
- struct pm_scan_arg - Pagemap ioctl argument
- @size: Size of the structure
- @flags: Flags for the IOCTL
- @start: Starting address of the region
- @len: Length of the region (All the pages in this length are included)
Maybe `scan_start`, `scan_len` - so that there is a better distinction from the structure's `size` field?
- @vec: Address of page_region struct array for output
- @vec_len: Length of the page_region struct array
- @max_pages: Optional max return pages
- @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
- */
I skipped most of the page walk implementation as maybe the comments above could make it simpler. Reading this patch and the documentation I still feel confused about how the filtering/limiting parameters should affect GET, WP and WP+GET. Should they limit the pages walked (and WP-ed)? Or only the GET's output? How about GET+WP case?
Best Regards
Michał Mirosław
On 6/14/23 3:36 AM, Michał Mirosław wrote:
On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum usama.anjum@collabora.com 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).
- Find pages which have been written-to and/or write protect the pages (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP)
This IOCTL can be extended to get information about more PTE bits.
[...]
--- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c
[...]
+static int pagemap_scan_output(bool wt, bool file, bool pres, bool swap,
struct pagemap_scan_private *p,unsigned long addr, unsigned int n_pages)+{
unsigned long bitmap = PM_SCAN_BITMAP(wt, file, pres, swap);struct page_region *cur_buf = &p->cur_buf;Maybe we can go a step further and say here `cur_buf = &p->vec_buf[p->vec_buf_index];` and remove `p->cur_buf` entirely?
No, this cannot be done. vec_buf_index represents overall index in vec_buf which is a user buffer. We cannot access it conveniently. This is why I'd added cur_buf in the fisrt place.
if (!n_pages)return -EINVAL;if ((p->required_mask & bitmap) != p->required_mask)return 0;if (p->anyof_mask && !(p->anyof_mask & bitmap))return 0;if (p->excluded_mask & bitmap)return 0;bitmap &= p->return_mask;if (!bitmap)return 0;if (cur_buf->bitmap == bitmap &&cur_buf->start + cur_buf->len * PAGE_SIZE == addr) {cur_buf->len += n_pages;p->found_pages += n_pages;} else {if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len)return -ENOMEM;Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel ran out of memory when allocating, not that there is no space in a user-provided buffer.
There are 3 kinds of return values here: * PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the page walk from next entry * 0 ---> continue the page walk * -ENOMEM --> Abort the page walk from current entry, user buffer is full which is not error, but only a stop signal. This -ENOMEM is just differentiater from (1). This -ENOMEM is for internal use and isn't returned to user.
I think it would be better to use following in place of -ENOMEM: #define PM_SCAN_BUFFER_FULL (-256)
I'll update.
BTW, the check could be inside the if() below for easier reading and less redundancy.
Sure.
if (cur_buf->len) {memcpy(&p->vec_buf[p->vec_buf_index], cur_buf,sizeof(*p->vec_buf));p->vec_buf_index++;}cur_buf->start = addr;cur_buf->len = n_pages;cur_buf->bitmap = bitmap;p->found_pages += n_pages;}if (p->max_pages && (p->found_pages == p->max_pages))Since `p->found_pages > 0` holds here, the first check is redundant.
I didn't get it. As the max_pages is optional, its validity needs to be checked first.
Nit: the parentheses around == are not needed.
Sure, I'll update.
return PM_SCAN_FOUND_MAX_PAGES;return 0;+}
[...]
+static inline unsigned long pagemap_scan_check_page_written(struct pagemap_scan_private *p) +{
return ((p->required_mask | p->anyof_mask | p->excluded_mask) &PAGE_IS_WRITTEN) ? PM_SCAN_OP_WRITE : 0;+}
Please inline at the single callsite.
Sure.
For flags name: PM_REQUIRE_WRITE_ACCESS? Or Is it intended to be checked only if doing WP (as the current name suggests) and so it would be redundant as WP currently requires `p->required_mask = PAGE_IS_WRITTEN`?
This is intended to indicate that if userfaultfd is needed. If PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if userfaultfd has been initialized for this memory. I'll rename to PM_SCAN_REQUIRE_UFFD.
+static int pagemap_scan_args_valid(struct pm_scan_arg *arg, unsigned long start,
struct page_region __user *vec)+{
/* Detect illegal size, flags, len and masks */if (arg->size != sizeof(struct pm_scan_arg))return -EINVAL;if (arg->flags & ~PM_SCAN_OPS)return -EINVAL;if (!arg->len)return -EINVAL;if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask |arg->return_mask) & ~PM_SCAN_BITS_ALL)return -EINVAL;if (!arg->required_mask && !arg->anyof_mask &&!arg->excluded_mask)return -EINVAL;if (!arg->return_mask)return -EINVAL;I just noticed that `!arg->return_mask == !IS_PM_SCAN_GET()`. So the OP_GET is redundant and can be removed from the `flags` if checking `return_mask` instead. That way there won't be a "flags-encoded ops" thing as it would be a 'scan' with optional 'write-protect'.
I know it makes flags checking easier if we remove PM_SCAN_OP_GET. But it'll created complications later if more OPs are added to the IOCTL. Lets not remove it.
Some history: I'd PM_SCAN_OP_GET. Then someone asked me to remove it. Then someone asked me to add it back.
/* Validate memory range */if (!IS_ALIGNED(start, PAGE_SIZE))return -EINVAL;if (!access_ok((void __user *)start, arg->len))return -EFAULT;if (IS_PM_SCAN_GET(arg->flags)) {if (!arg->vec)return -EINVAL;access_ok() -> -EFAULT (an early fail-check) (the vec_len should be checked first then, failing with -EINVAL if 0)
I'd access_ok() for vec. But I removed it after discussion with you. Now I only check that vec shouldn't be NULL: https://lore.kernel.org/all/CABb0KFGru9xFCxyVHBcE+dkXe58=5wiCbvZGSWhuTfr4gn=...
[...]
if (!access_ok((void __user *)vec,arg->vec_len * sizeof(struct page_region)))return false;Is there a provision that userspace threads are all blocked from manipulating mmaps during this ioctl()? If not, this is a TOCTOU bug and the writes should be checked each time as another userspace thread could remap the memory while the ioctl() is working.
mincore() syscall is doing in the same way. It checks the validity in the start only. What provision should I add? Isn't it obvious that the user should not remap such memory?
On the second look, I think the code already checks that while doing copy_to_user(), so this check is redundant and can be removed.
if (arg->vec_len == 0)return -EINVAL;}if (IS_PM_SCAN_WP(arg->flags)) {if (!IS_PM_SCAN_GET(arg->flags) && arg->max_pages)return -EINVAL;if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask) &~PAGE_IS_WRITTEN)Is `excluded_mask = PAGE_IS_WRITTEN` intended to be allowed? It would make WP do nothing, unless the required/anyof/excluded masks are not supposed to limit WP?
My intention is that no other flag than PAGE_IS_WRITTEN must be specified in all masks if WP op is specified.
If exluded_mask = PAGE_IS_WRITTEN, * the page would be rejected if page is written. No need to WP these. * Only those pages would be allowed which aren't written. Then would also not write protected.
So write protect operation wouldn't happen logically. It is on user for not comprehending the combined meaning of WP op and excluded mask.
Do you agree or do you want me to reject WP op if excluded = PAGE_IS_WRITTEN?
return -EINVAL;}If `flags == 0` (and `return_mask == 0` in case my earlier comment is applied) it should fail with -EINVAL.
I'll add flags==0 condition.
[...]
--- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h +/*
- struct page_region - Page region with bitmap flags
- @start: Start of the region
- @len: Length of the region in pages
- bitmap: Bits sets for the region
'@' is missing for the third field. BTW, maybe we can call it something like `flags` or `category` (something that hints at the meaning of the value instead of its data representation).
The deification of this struct says, "with bitmap flags". Bitmap was a different name. I'll update it to flags.
+/*
- struct pm_scan_arg - Pagemap ioctl argument
- @size: Size of the structure
- @flags: Flags for the IOCTL
- @start: Starting address of the region
- @len: Length of the region (All the pages in this length are included)
Maybe `scan_start`, `scan_len` - so that there is a better distinction from the structure's `size` field?
As start and len already communicate the meaning. We are making things more verbose.
- @vec: Address of page_region struct array for output
- @vec_len: Length of the page_region struct array
- @max_pages: Optional max return pages
- @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
- */
I skipped most of the page walk implementation as maybe the comments above could make it simpler. Reading this patch and the documentation I still feel confused about how the filtering/limiting parameters
I'm really sad to hear this. I've been working on making this series from so many revisions. I was hopping that it would make complete sense to reviewers and later to users.
What do you think is missing which is restricting these patches getting accepted to upstream?
should affect GET, WP and WP+GET. Should they limit the pages walked (and WP-ed)? Or only the GET's output? How about GET+WP case?
The address range needs to be walked until max pages pages are found, user buffer is full or whole range is walked. If the page will be added to user buffer or not depends on the selection criteria (*masks). There is no difference in case of walk for GET, WP and GET+WP. Only that WP doesn't take any user buffer and just WPs the whole region.
Best Regards
Michał Mirosław
On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 3:36 AM, Michał Mirosław wrote:
On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum usama.anjum@collabora.com 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).
- Find pages which have been written-to and/or write protect the pages (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP)
This IOCTL can be extended to get information about more PTE bits.
[...]
--- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c
[...]
+static int pagemap_scan_output(bool wt, bool file, bool pres, bool swap,
struct pagemap_scan_private *p,unsigned long addr, unsigned int n_pages)+{
unsigned long bitmap = PM_SCAN_BITMAP(wt, file, pres, swap);struct page_region *cur_buf = &p->cur_buf;Maybe we can go a step further and say here `cur_buf = &p->vec_buf[p->vec_buf_index];` and remove `p->cur_buf` entirely?
No, this cannot be done. vec_buf_index represents overall index in vec_buf which is a user buffer. We cannot access it conveniently. This is why I'd added cur_buf in the fisrt place.
Wasn't this the case only before the intermediate p->vec_buf array was added? It is now a kmalloc()ed temporary and it isn't copied to userspace until the walk iteration finishes.
[...]
if (cur_buf->bitmap == bitmap &&cur_buf->start + cur_buf->len * PAGE_SIZE == addr) {cur_buf->len += n_pages;p->found_pages += n_pages;} else {if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len)return -ENOMEM;Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel ran out of memory when allocating, not that there is no space in a user-provided buffer.
There are 3 kinds of return values here:
- PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the
page walk from next entry
- 0 ---> continue the page walk
- -ENOMEM --> Abort the page walk from current entry, user buffer is full
which is not error, but only a stop signal. This -ENOMEM is just differentiater from (1). This -ENOMEM is for internal use and isn't returned to user.
But why ENOSPC is not good here? I was used before, I think.
if (cur_buf->len) {memcpy(&p->vec_buf[p->vec_buf_index], cur_buf,sizeof(*p->vec_buf));p->vec_buf_index++;}cur_buf->start = addr;cur_buf->len = n_pages;cur_buf->bitmap = bitmap;p->found_pages += n_pages;}if (p->max_pages && (p->found_pages == p->max_pages))Since `p->found_pages > 0` holds here, the first check is redundant.
I didn't get it. As the max_pages is optional, its validity needs to be checked first.
I mean that `p->max_pages`, if 0, will never compare equal to `p->found_pages` here.
For flags name: PM_REQUIRE_WRITE_ACCESS? Or Is it intended to be checked only if doing WP (as the current name suggests) and so it would be redundant as WP currently requires `p->required_mask = PAGE_IS_WRITTEN`?
This is intended to indicate that if userfaultfd is needed. If PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if userfaultfd has been initialized for this memory. I'll rename to PM_SCAN_REQUIRE_UFFD.
Why do we need that check? Wouldn't `is_written = false` work for vmas not registered via uffd?
+static int pagemap_scan_args_valid(struct pm_scan_arg *arg, unsigned long start,
struct page_region __user *vec)+{
/* Detect illegal size, flags, len and masks */if (arg->size != sizeof(struct pm_scan_arg))return -EINVAL;if (arg->flags & ~PM_SCAN_OPS)return -EINVAL;if (!arg->len)return -EINVAL;if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask |arg->return_mask) & ~PM_SCAN_BITS_ALL)return -EINVAL;if (!arg->required_mask && !arg->anyof_mask &&!arg->excluded_mask)return -EINVAL;if (!arg->return_mask)return -EINVAL;I just noticed that `!arg->return_mask == !IS_PM_SCAN_GET()`. So the OP_GET is redundant and can be removed from the `flags` if checking `return_mask` instead. That way there won't be a "flags-encoded ops" thing as it would be a 'scan' with optional 'write-protect'.
I know it makes flags checking easier if we remove PM_SCAN_OP_GET. But it'll created complications later if more OPs are added to the IOCTL. Lets not remove it.
Some history: I'd PM_SCAN_OP_GET. Then someone asked me to remove it. Then someone asked me to add it back.
/* Validate memory range */if (!IS_ALIGNED(start, PAGE_SIZE))return -EINVAL;if (!access_ok((void __user *)start, arg->len))return -EFAULT;if (IS_PM_SCAN_GET(arg->flags)) {if (!arg->vec)return -EINVAL;access_ok() -> -EFAULT (an early fail-check) (the vec_len should be checked first then, failing with -EINVAL if 0)
I'd access_ok() for vec. But I removed it after discussion with you. Now I only check that vec shouldn't be NULL: https://lore.kernel.org/all/CABb0KFGru9xFCxyVHBcE+dkXe58=5wiCbvZGSWhuTfr4gn=...
Oh, indeed. It seems I then misunderstood what `access_ok()` does. It is only doing a sanity check on the pointer (range) and not actually verifying memory mapping validity. So as a early-fail check it's ok.
[...]
if (arg->vec_len == 0)return -EINVAL;}if (IS_PM_SCAN_WP(arg->flags)) {if (!IS_PM_SCAN_GET(arg->flags) && arg->max_pages)return -EINVAL;if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask) &~PAGE_IS_WRITTEN)Is `excluded_mask = PAGE_IS_WRITTEN` intended to be allowed? It would make WP do nothing, unless the required/anyof/excluded masks are not supposed to limit WP?
My intention is that no other flag than PAGE_IS_WRITTEN must be specified in all masks if WP op is specified.
If exluded_mask = PAGE_IS_WRITTEN,
- the page would be rejected if page is written. No need to WP these.
- Only those pages would be allowed which aren't written. Then would also
not write protected.
So write protect operation wouldn't happen logically. It is on user for not comprehending the combined meaning of WP op and excluded mask.
Do you agree or do you want me to reject WP op if excluded = PAGE_IS_WRITTEN?
I see a bit of inconsistency here in that in some parts of the API we disallow nonsensical (no-op) parameters (like output_mask == 0 or required_mask == excluded_mask), but in others do not. I'd prefer to have it one way and if I had to choose I'd allow trivial no-ops to reduce the kernel-side code overhead.
The other topic is whether we should limit the page selection to PAGE_IS_WRITTEN for WP op. I don't see any benefit from that.
Maybe we should go back to the drawing board for a quick review: Can we decouple the responsibilities of the page selection from WP operation? I'd think yes - are there downsides to having the required/any/excluded masks limit the pages presented to GET and/or WP and have GET use returned_mask, max_pages, vec_buf+len and stop the scan after the limit is hit? WP would just act on whatever pages were accepted by the walk filter and (if enabled) GET output buffer.
Nit: I'd prefer the excluded_mask be replaced by inverted_mask, but we already discussed that so feel free to ignore. This would remove a bit of logic from the filtering code, and userspace could always wrap the ioctl in a function that converts from required+excluded into required+inverted.
While here, I wonder if we really need to fail the call if there are unknown bits in those masks set: if this bit set is expanded with another category flags, a newer userspace run on older kernel would get EINVAL even if the "treat unknown as 0" be what it requires. There is no simple way in the API to discover what bits the kernel supports. We could allow a no-op (no WP nor GET) call to help with that and then rejecting unknown bits would make sense.
[...]
--- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h +/*
- struct page_region - Page region with bitmap flags
- @start: Start of the region
- @len: Length of the region in pages
- bitmap: Bits sets for the region
'@' is missing for the third field. BTW, maybe we can call it something like `flags` or `category` (something that hints at the meaning of the value instead of its data representation).
The deification of this struct says, "with bitmap flags". Bitmap was a different name. I'll update it to flags.
From the implementation and our discussions I guess the `bitmap`/`flags` field is holding a set of matching categories: a bit value 1 = pages are in this category, value 0 = pages are not in this category.
+/*
- struct pm_scan_arg - Pagemap ioctl argument
- @size: Size of the structure
- @flags: Flags for the IOCTL
- @start: Starting address of the region
- @len: Length of the region (All the pages in this length are included)
Maybe `scan_start`, `scan_len` - so that there is a better distinction from the structure's `size` field?
As start and len already communicate the meaning. We are making things more verbose.
We are describing (in the name) only that it is a range, but not of what or what purpose. That information is only in the docstring, but it is harder to get by someone just reading the code.
- @vec: Address of page_region struct array for output
- @vec_len: Length of the page_region struct array
- @max_pages: Optional max return pages
- @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
- */
I skipped most of the page walk implementation as maybe the comments above could make it simpler. Reading this patch and the documentation I still feel confused about how the filtering/limiting parameters
I'm really sad to hear this. I've been working on making this series from so many revisions. I was hopping that it would make complete sense to reviewers and later to users.
What do you think is missing which is restricting these patches getting accepted to upstream?
should affect GET, WP and WP+GET. Should they limit the pages walked (and WP-ed)? Or only the GET's output? How about GET+WP case?
The address range needs to be walked until max pages pages are found, user buffer is full or whole range is walked. If the page will be added to user buffer or not depends on the selection criteria (*masks). There is no difference in case of walk for GET, WP and GET+WP. Only that WP doesn't take any user buffer and just WPs the whole region.
Ok, then this intent (if I understand correctly) does not entirely match the implementation. Let's split up the conditions:
1. The address range needs to be walked until max pages pages are found
current implementation: the address range is walked until max pages matching masks (incl. return_mask) are reported by GET (or until end of range if GET is not requested). Maybe we need to describe what "found" means here?
2. user buffer is full
Matches implementation except in GET+WP edge cases.
3. or whole range is walked
Matches implementation.
4. If the page will be added to user buffer or not depends on the selection criteria (*masks).
Is `return_mask` a selection criterion? The implementation does skip (counting and outputting) pages where (category_mask & return_mask) == 0.
5. There is no difference in case of walk for GET, WP and GET+WP.
I'm not sure I understand this condition: Is it that the sequence of pages walked does not depend on the operations? If so, then I think the implementation matches (not entirely sure, though).
6. Only that WP doesn't take any user buffer
Matches implementation (trivial).
7. ... and just WPs the whole region.
Follows from current implementation for #1.
But there is an additional restriction: - if using WP, masks can only contain IS_WRITTEN.
Summing this up: I think the number of interactions between various parameters can be reduced: a. required/anyof/excluded masks apply to the operation as a whole (selecting pages to act on); and b. returned_mask, max_pages, vec_buf+len apply to GET and stop the walk early if limits are hit (checked before WP is done)
That way we can conceptually decouple WP from the selection of pages and don't need to restrict the page masks to IS_WRITTEN. This makes the operation basically a GET (SCAN as the ioctl name suggests) with an auxiliary and optional WP (IOW: pages walked don't depend on WP operation being requested or not). This also matches the current implementation, where plain WP can be done via UFFD-specific call. (BTW, why the UFFD one has to be slower?)
(Note: my intent in this discussion is to minimise the effort needed to understand how to use and what are the restrictions of the API - and doing this by minimizing the number and size of rules that one has to apply. Nb. this is proportional to number of rules in ...args_validate().)
Best Regards Michał Mirosław
On 6/14/23 8:14 PM, Michał Mirosław wrote:
On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 3:36 AM, Michał Mirosław wrote:
On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum usama.anjum@collabora.com 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).
- Find pages which have been written-to and/or write protect the pages (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP)
This IOCTL can be extended to get information about more PTE bits.
[...]
--- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c
[...]
+static int pagemap_scan_output(bool wt, bool file, bool pres, bool swap,
struct pagemap_scan_private *p,unsigned long addr, unsigned int n_pages)+{
unsigned long bitmap = PM_SCAN_BITMAP(wt, file, pres, swap);struct page_region *cur_buf = &p->cur_buf;Maybe we can go a step further and say here `cur_buf = &p->vec_buf[p->vec_buf_index];` and remove `p->cur_buf` entirely?
No, this cannot be done. vec_buf_index represents overall index in vec_buf which is a user buffer. We cannot access it conveniently. This is why I'd added cur_buf in the fisrt place.
Sorry, vec_buf_index starts from 0 in each smaller page walk. Whatever we put in vec_buf is copied to user buffer and cur_buf isn't copied until we walk over next pages. This seems straight forward to me. But in case as you are saying that we start putting cur_buf inside vec_buf itself, we'll have to be careful that we don't copy the last element to the user as we'll be readjusting it and may be doing addition in next small page walk.
Wasn't this the case only before the intermediate p->vec_buf array was added? It is now a kmalloc()ed temporary and it isn't copied to userspace until the walk iteration finishes.
[...]
if (cur_buf->bitmap == bitmap &&cur_buf->start + cur_buf->len * PAGE_SIZE == addr) {cur_buf->len += n_pages;p->found_pages += n_pages;} else {if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len)return -ENOMEM;Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel ran out of memory when allocating, not that there is no space in a user-provided buffer.
There are 3 kinds of return values here:
- PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the
page walk from next entry
- 0 ---> continue the page walk
- -ENOMEM --> Abort the page walk from current entry, user buffer is full
which is not error, but only a stop signal. This -ENOMEM is just differentiater from (1). This -ENOMEM is for internal use and isn't returned to user.
But why ENOSPC is not good here? I was used before, I think.
-ENOSPC is being returned in form of true error from pagemap_scan_hugetlb_entry(). So I'd to remove -ENOSPC from here as it wasn't true error here, it was only a way to abort the walk immediately. I'm liking the following erturn code from here now:
#define PM_SCAN_BUFFER_FULL (-256)
if (cur_buf->len) {memcpy(&p->vec_buf[p->vec_buf_index], cur_buf,sizeof(*p->vec_buf));p->vec_buf_index++;}cur_buf->start = addr;cur_buf->len = n_pages;cur_buf->bitmap = bitmap;p->found_pages += n_pages;}if (p->max_pages && (p->found_pages == p->max_pages))Since `p->found_pages > 0` holds here, the first check is redundant.
I didn't get it. As the max_pages is optional, its validity needs to be checked first.
I mean that `p->max_pages`, if 0, will never compare equal to `p->found_pages` here.
Okay, I'll update.
For flags name: PM_REQUIRE_WRITE_ACCESS? Or Is it intended to be checked only if doing WP (as the current name suggests) and so it would be redundant as WP currently requires `p->required_mask = PAGE_IS_WRITTEN`?
This is intended to indicate that if userfaultfd is needed. If PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if userfaultfd has been initialized for this memory. I'll rename to PM_SCAN_REQUIRE_UFFD.
Why do we need that check? Wouldn't `is_written = false` work for vmas not registered via uffd?
UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region for it to report correct written values on the memory region. Without UFFD WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state undefined. If user hasn't initialized memory with UFFD, he has no right to set is_written = false.
+static int pagemap_scan_args_valid(struct pm_scan_arg *arg, unsigned long start,
struct page_region __user *vec)+{
/* Detect illegal size, flags, len and masks */if (arg->size != sizeof(struct pm_scan_arg))return -EINVAL;if (arg->flags & ~PM_SCAN_OPS)return -EINVAL;if (!arg->len)return -EINVAL;if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask |arg->return_mask) & ~PM_SCAN_BITS_ALL)return -EINVAL;if (!arg->required_mask && !arg->anyof_mask &&!arg->excluded_mask)return -EINVAL;if (!arg->return_mask)return -EINVAL;I just noticed that `!arg->return_mask == !IS_PM_SCAN_GET()`. So the OP_GET is redundant and can be removed from the `flags` if checking `return_mask` instead. That way there won't be a "flags-encoded ops" thing as it would be a 'scan' with optional 'write-protect'.
I know it makes flags checking easier if we remove PM_SCAN_OP_GET. But it'll created complications later if more OPs are added to the IOCTL. Lets not remove it.
Some history: I'd PM_SCAN_OP_GET. Then someone asked me to remove it. Then someone asked me to add it back.
/* Validate memory range */if (!IS_ALIGNED(start, PAGE_SIZE))return -EINVAL;if (!access_ok((void __user *)start, arg->len))return -EFAULT;if (IS_PM_SCAN_GET(arg->flags)) {if (!arg->vec)return -EINVAL;access_ok() -> -EFAULT (an early fail-check) (the vec_len should be checked first then, failing with -EINVAL if 0)
I'd access_ok() for vec. But I removed it after discussion with you. Now I only check that vec shouldn't be NULL: https://lore.kernel.org/all/CABb0KFGru9xFCxyVHBcE+dkXe58=5wiCbvZGSWhuTfr4gn=...
Oh, indeed. It seems I then misunderstood what `access_ok()` does. It is only doing a sanity check on the pointer (range) and not actually verifying memory mapping validity. So as a early-fail check it's ok.
I'll re-add.
[...]
if (arg->vec_len == 0)return -EINVAL;}if (IS_PM_SCAN_WP(arg->flags)) {if (!IS_PM_SCAN_GET(arg->flags) && arg->max_pages)return -EINVAL;if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask) &~PAGE_IS_WRITTEN)Is `excluded_mask = PAGE_IS_WRITTEN` intended to be allowed? It would make WP do nothing, unless the required/anyof/excluded masks are not supposed to limit WP?
My intention is that no other flag than PAGE_IS_WRITTEN must be specified in all masks if WP op is specified.
If exluded_mask = PAGE_IS_WRITTEN,
- the page would be rejected if page is written. No need to WP these.
- Only those pages would be allowed which aren't written. Then would also
not write protected.
So write protect operation wouldn't happen logically. It is on user for not comprehending the combined meaning of WP op and excluded mask.
Do you agree or do you want me to reject WP op if excluded = PAGE_IS_WRITTEN?
I see a bit of inconsistency here in that in some parts of the API we disallow nonsensical (no-op) parameters (like output_mask == 0 or required_mask == excluded_mask), but in others do not. I'd prefer to have it one way and if I had to choose I'd allow trivial no-ops to reduce the kernel-side code overhead.
The other topic is whether we should limit the page selection to PAGE_IS_WRITTEN for WP op. I don't see any benefit from that.
Maybe we should go back to the drawing board for a quick review: Can we decouple the responsibilities of the page selection from WP operation? I'd think yes - are there downsides to having the required/any/excluded masks limit the pages presented to GET and/or WP and have GET use returned_mask, max_pages, vec_buf+len and stop the scan after the limit is hit? WP would just act on whatever pages were accepted by the walk filter and (if enabled) GET output buffer.
No, we only take read mm lock. We need to WP only those pages which have been noted to be written, the page passed the filtering with all the masks and page successfully got added to user buffer. This would break if we try to decouple.
Nit: I'd prefer the excluded_mask be replaced by inverted_mask, but we already discussed that so feel free to ignore. This would remove a bit of logic from the filtering code, and userspace could always wrap the ioctl in a function that converts from required+excluded into required+inverted.
While here, I wonder if we really need to fail the call if there are unknown bits in those masks set: if this bit set is expanded with another category flags, a newer userspace run on older kernel would get EINVAL even if the "treat unknown as 0" be what it requires. There is no simple way in the API to discover what bits the kernel supports. We could allow a no-op (no WP nor GET) call to help with that and then rejecting unknown bits would make sense.
I've not seen any examples of this. But I've seen examples of returning error if kernel doesn't support a feature. Each new feature comes with a kernel version, greater than this version support this feature. If user is trying to use advanced feature which isn't present in a kernel, we should return error and not proceed to confuse the user/kernel. In fact if we look at userfaultfd_api(), we return error immediately if feature has some bit set which kernel doesn't support.
[...]
--- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h +/*
- struct page_region - Page region with bitmap flags
- @start: Start of the region
- @len: Length of the region in pages
- bitmap: Bits sets for the region
'@' is missing for the third field. BTW, maybe we can call it something like `flags` or `category` (something that hints at the meaning of the value instead of its data representation).
The deification of this struct says, "with bitmap flags". Bitmap was a different name. I'll update it to flags.
From the implementation and our discussions I guess the `bitmap`/`flags` field is holding a set of matching categories: a bit value 1 = pages are in this category, value 0 = pages are not in this category.
+/*
- struct pm_scan_arg - Pagemap ioctl argument
- @size: Size of the structure
- @flags: Flags for the IOCTL
- @start: Starting address of the region
- @len: Length of the region (All the pages in this length are included)
Maybe `scan_start`, `scan_len` - so that there is a better distinction from the structure's `size` field?
As start and len already communicate the meaning. We are making things more verbose.
We are describing (in the name) only that it is a range, but not of what or what purpose. That information is only in the docstring, but it is harder to get by someone just reading the code.
Agreed. But I'm using same names, start and len which mincore (a historic syscall) is using. I've followed mincore here.
- @vec: Address of page_region struct array for output
- @vec_len: Length of the page_region struct array
- @max_pages: Optional max return pages
- @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
- */
I skipped most of the page walk implementation as maybe the comments above could make it simpler. Reading this patch and the documentation I still feel confused about how the filtering/limiting parameters
I'm really sad to hear this. I've been working on making this series from so many revisions. I was hopping that it would make complete sense to reviewers and later to users.
What do you think is missing which is restricting these patches getting accepted to upstream?
should affect GET, WP and WP+GET. Should they limit the pages walked (and WP-ed)? Or only the GET's output? How about GET+WP case?
The address range needs to be walked until max pages pages are found, user buffer is full or whole range is walked. If the page will be added to user buffer or not depends on the selection criteria (*masks). There is no difference in case of walk for GET, WP and GET+WP. Only that WP doesn't take any user buffer and just WPs the whole region.
Ok, then this intent (if I understand correctly) does not entirely match the implementation. Let's split up the conditions:
- The address range needs to be walked until max pages pages are found
current implementation: the address range is walked until max pages matching masks (incl. return_mask) are reported by GET (or until end of range if GET is not requested). Maybe we need to describe what "found" means here?
Found means all the pages which are found to be fulfilling the masks and we have added it to the user buffer. I can add the comment on top of pagemap_scan_private struct? But I don't think that it is difficult to understand the meaning of found_pages and also we compare it with max_pages which makes things very easy to understand.
- user buffer is full
Matches implementation except in GET+WP edge cases.
I'm not sure which edge case you are referring to? Probably for hugetlb error return case?
- or whole range is walked
Matches implementation.
- If the page will be added to user buffer or not depends on the
selection criteria (*masks).
Is `return_mask` a selection criterion? The implementation does skip (counting and outputting) pages where (category_mask & return_mask) == 0.
No, it shouldn't be a selection criterion. I guess, right now I've wrong implementation of this mask. Return mask should just mask the flags:
@@ -1894,8 +1894,8 @@ static int pagemap_scan_output(bool wt, bool file, bool pres, bool swap, return 0;
flags &= p->return_mask; - if (!flags) - return 0;
If user want to find the write status of all PRESENT pages, required_mask = PRESENT and RETURN_MASK = WRITTEN. Thoughts?
- There is no difference in case of walk for GET, WP and GET+WP.
I'm not sure I understand this condition: Is it that the sequence of pages walked does not depend on the operations? If so, then I think the implementation matches (not entirely sure, though).
Yeah, correct.
- Only that WP doesn't take any user buffer
Matches implementation (trivial).
- ... and just WPs the whole region.
Follows from current implementation for #1.
But there is an additional restriction:
- if using WP, masks can only contain IS_WRITTEN.
Summing this up: I think the number of interactions between various parameters can be reduced: a. required/anyof/excluded masks apply to the operation as a whole (selecting pages to act on); and b. returned_mask, max_pages, vec_buf+len apply to GET and stop the walk early if limits are hit (checked before WP is done)
That way we can conceptually decouple WP from the selection of pages and don't need to restrict the page masks to IS_WRITTEN. This makes the operation basically a GET (SCAN as the ioctl name suggests) with an auxiliary and optional WP (IOW: pages walked don't depend on WP operation being requested or not). This also matches the current implementation, where plain WP can be done via UFFD-specific call.
Your explanation is very thoughtful. Decoupling will surely help in making things simpler and very easy to understand. I'll make the change to decouple WP op from masks.
@@ -2141,9 +2141,9 @@ static int pagemap_scan_args_valid(struct pm_scan_arg *arg, unsigned long start, if (!IS_PM_SCAN_GET(arg->flags) && arg->max_pages) return -EINVAL;
- if ((arg->required_mask | arg->anyof_mask | arg->excluded_mask) & - ~PAGE_IS_WRITTEN) - return -EINVAL;
(BTW, why the UFFD one has to be slower?)
Probably the worse case of 6ce64428d6 is hitting and making writes slower: https://lore.kernel.org/all/ZG4Xb3rYK0p8BoB9@x1n
(Note: my intent in this discussion is to minimise the effort needed to understand how to use and what are the restrictions of the API - and doing this by minimizing the number and size of rules that one has to apply. Nb. this is proportional to number of rules in ...args_validate().)
Best Regards Michał Mirosław
(A quick reply to answer open questions in case they help the next version.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 8:14 PM, Michał Mirosław wrote:
On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 3:36 AM, Michał Mirosław wrote:
On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
[...]
if (cur_buf->bitmap == bitmap &&cur_buf->start + cur_buf->len * PAGE_SIZE == addr) {cur_buf->len += n_pages;p->found_pages += n_pages;} else {if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len)return -ENOMEM;Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel ran out of memory when allocating, not that there is no space in a user-provided buffer.
There are 3 kinds of return values here:
- PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the
page walk from next entry
- 0 ---> continue the page walk
- -ENOMEM --> Abort the page walk from current entry, user buffer is full
which is not error, but only a stop signal. This -ENOMEM is just differentiater from (1). This -ENOMEM is for internal use and isn't returned to user.
But why ENOSPC is not good here? I was used before, I think.
-ENOSPC is being returned in form of true error from pagemap_scan_hugetlb_entry(). So I'd to remove -ENOSPC from here as it wasn't true error here, it was only a way to abort the walk immediately. I'm liking the following erturn code from here now:
#define PM_SCAN_BUFFER_FULL (-256)
I guess this will be reworked anyway, but I'd prefer this didn't need custom errors etc. If we agree to decoupling the selection and GET output, it could be:
bool is_interesting_page(p, flags); // this one does the required/anyof/excluded match size_t output_range(p, start, len, flags); // this one fills the output vector and returns how many pages were fit
In this setup, `is_interesting_page() && (n_out = output_range()) < n_pages` means this is the final range, no more will fit. And if `n_out == 0` then no pages fit and no WP is needed (no other special cases).
For flags name: PM_REQUIRE_WRITE_ACCESS? Or Is it intended to be checked only if doing WP (as the current name suggests) and so it would be redundant as WP currently requires `p->required_mask = PAGE_IS_WRITTEN`?
This is intended to indicate that if userfaultfd is needed. If PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if userfaultfd has been initialized for this memory. I'll rename to PM_SCAN_REQUIRE_UFFD.
Why do we need that check? Wouldn't `is_written = false` work for vmas not registered via uffd?
UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region for it to report correct written values on the memory region. Without UFFD WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state undefined. If user hasn't initialized memory with UFFD, he has no right to set is_written = false.
How about calculating `is_written = is_uffd_registered() && is_uffd_wp()`? This would enable a user to apply GET+WP for the whole address space of a process regardless of whether all of it is registered.
While here, I wonder if we really need to fail the call if there are unknown bits in those masks set: if this bit set is expanded with another category flags, a newer userspace run on older kernel would get EINVAL even if the "treat unknown as 0" be what it requires. There is no simple way in the API to discover what bits the kernel supports. We could allow a no-op (no WP nor GET) call to help with that and then rejecting unknown bits would make sense.
I've not seen any examples of this. But I've seen examples of returning error if kernel doesn't support a feature. Each new feature comes with a kernel version, greater than this version support this feature. If user is trying to use advanced feature which isn't present in a kernel, we should return error and not proceed to confuse the user/kernel. In fact if we look at userfaultfd_api(), we return error immediately if feature has some bit set which kernel doesn't support.
I think we should have a way of detecting the supported flags if we don't want a forward compatibility policy for flags here. Maybe it would be enough to allow all the no-op combinations for this purpose?
[...]
--- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h +/*
- struct page_region - Page region with bitmap flags
- @start: Start of the region
- @len: Length of the region in pages
- bitmap: Bits sets for the region
'@' is missing for the third field. BTW, maybe we can call it something like `flags` or `category` (something that hints at the meaning of the value instead of its data representation).
The deification of this struct says, "with bitmap flags". Bitmap was a different name. I'll update it to flags.
From the implementation and our discussions I guess the `bitmap`/`flags` field is holding a set of matching categories: a bit value 1 = pages are in this category, value 0 = pages are not in this category.
+/*
- struct pm_scan_arg - Pagemap ioctl argument
- @size: Size of the structure
- @flags: Flags for the IOCTL
- @start: Starting address of the region
- @len: Length of the region (All the pages in this length are included)
Maybe `scan_start`, `scan_len` - so that there is a better distinction from the structure's `size` field?
As start and len already communicate the meaning. We are making things more verbose.
We are describing (in the name) only that it is a range, but not of what or what purpose. That information is only in the docstring, but it is harder to get by someone just reading the code.
Agreed. But I'm using same names, start and len which mincore (a historic syscall) is using. I've followed mincore here.
mincore() doesn't take parameters as a struct, but as three positional arguments (whose names don't matter nor appear at call point) - I wouldn't take it as a precedent for structure field naming.
- @vec: Address of page_region struct array for output
- @vec_len: Length of the page_region struct array
- @max_pages: Optional max return pages
- @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
- */
I skipped most of the page walk implementation as maybe the comments above could make it simpler. Reading this patch and the documentation I still feel confused about how the filtering/limiting parameters
I'm really sad to hear this. I've been working on making this series from so many revisions. I was hopping that it would make complete sense to reviewers and later to users.
What do you think is missing which is restricting these patches getting accepted to upstream?
should affect GET, WP and WP+GET. Should they limit the pages walked (and WP-ed)? Or only the GET's output? How about GET+WP case?
The address range needs to be walked until max pages pages are found, user buffer is full or whole range is walked. If the page will be added to user buffer or not depends on the selection criteria (*masks). There is no difference in case of walk for GET, WP and GET+WP. Only that WP doesn't take any user buffer and just WPs the whole region.
Ok, then this intent (if I understand correctly) does not entirely match the implementation. Let's split up the conditions:
- The address range needs to be walked until max pages pages are found
current implementation: the address range is walked until max pages matching masks (incl. return_mask) are reported by GET (or until end of range if GET is not requested). Maybe we need to describe what "found" means here?
Found means all the pages which are found to be fulfilling the masks and we have added it to the user buffer. I can add the comment on top of pagemap_scan_private struct? But I don't think that it is difficult to understand the meaning of found_pages and also we compare it with max_pages which makes things very easy to understand.
After fixing `return_mask` and the selection/action split I think "pages found" might work - as now the count will be exactly what pages match the required/anyof/excluded criteria.
- user buffer is full
Matches implementation except in GET+WP edge cases.
I'm not sure which edge case you are referring to? Probably for hugetlb error return case?
Yes, that one.
Best Regards Michał Mirosław
I'll send next revision now.
On 6/14/23 11:00 PM, Michał Mirosław wrote:
(A quick reply to answer open questions in case they help the next version.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 8:14 PM, Michał Mirosław wrote:
On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 3:36 AM, Michał Mirosław wrote:
On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
[...]
if (cur_buf->bitmap == bitmap &&cur_buf->start + cur_buf->len * PAGE_SIZE == addr) {cur_buf->len += n_pages;p->found_pages += n_pages;} else {if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len)return -ENOMEM;Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel ran out of memory when allocating, not that there is no space in a user-provided buffer.
There are 3 kinds of return values here:
- PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the
page walk from next entry
- 0 ---> continue the page walk
- -ENOMEM --> Abort the page walk from current entry, user buffer is full
which is not error, but only a stop signal. This -ENOMEM is just differentiater from (1). This -ENOMEM is for internal use and isn't returned to user.
But why ENOSPC is not good here? I was used before, I think.
-ENOSPC is being returned in form of true error from pagemap_scan_hugetlb_entry(). So I'd to remove -ENOSPC from here as it wasn't true error here, it was only a way to abort the walk immediately. I'm liking the following erturn code from here now:
#define PM_SCAN_BUFFER_FULL (-256)
I guess this will be reworked anyway, but I'd prefer this didn't need custom errors etc. If we agree to decoupling the selection and GET output, it could be:
bool is_interesting_page(p, flags); // this one does the required/anyof/excluded match size_t output_range(p, start, len, flags); // this one fills the output vector and returns how many pages were fit
In this setup, `is_interesting_page() && (n_out = output_range()) < n_pages` means this is the final range, no more will fit. And if `n_out == 0` then no pages fit and no WP is needed (no other special cases).
Right now, pagemap_scan_output() performs the work of both of these two functions. The part can be broken into is_interesting_pages() and we can leave the remaining part as it is.
Saying that n_out < n_pages tells us the buffer is full covers one case. But there is case of maximum pages have been found and walk needs to be aborted.
I'll just add is_interesting_page() in next version.
For flags name: PM_REQUIRE_WRITE_ACCESS? Or Is it intended to be checked only if doing WP (as the current name suggests) and so it would be redundant as WP currently requires `p->required_mask = PAGE_IS_WRITTEN`?
This is intended to indicate that if userfaultfd is needed. If PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if userfaultfd has been initialized for this memory. I'll rename to PM_SCAN_REQUIRE_UFFD.
Why do we need that check? Wouldn't `is_written = false` work for vmas not registered via uffd?
UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region for it to report correct written values on the memory region. Without UFFD WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state undefined. If user hasn't initialized memory with UFFD, he has no right to set is_written = false.
How about calculating `is_written = is_uffd_registered() && is_uffd_wp()`? This would enable a user to apply GET+WP for the whole address space of a process regardless of whether all of it is registered.
I wouldn't want to check if uffd is registered again and again. This is why we are doing it only once every walk in pagemap_scan_test_walk().
While here, I wonder if we really need to fail the call if there are unknown bits in those masks set: if this bit set is expanded with another category flags, a newer userspace run on older kernel would get EINVAL even if the "treat unknown as 0" be what it requires. There is no simple way in the API to discover what bits the kernel supports. We could allow a no-op (no WP nor GET) call to help with that and then rejecting unknown bits would make sense.
I've not seen any examples of this. But I've seen examples of returning error if kernel doesn't support a feature. Each new feature comes with a kernel version, greater than this version support this feature. If user is trying to use advanced feature which isn't present in a kernel, we should return error and not proceed to confuse the user/kernel. In fact if we look at userfaultfd_api(), we return error immediately if feature has some bit set which kernel doesn't support.
I think we should have a way of detecting the supported flags if we don't want a forward compatibility policy for flags here. Maybe it would be enough to allow all the no-op combinations for this purpose?
Again I don't think UFFD is doing anything like this.
[...]
--- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h +/*
- struct page_region - Page region with bitmap flags
- @start: Start of the region
- @len: Length of the region in pages
- bitmap: Bits sets for the region
'@' is missing for the third field. BTW, maybe we can call it something like `flags` or `category` (something that hints at the meaning of the value instead of its data representation).
The deification of this struct says, "with bitmap flags". Bitmap was a different name. I'll update it to flags.
From the implementation and our discussions I guess the `bitmap`/`flags` field is holding a set of matching categories: a bit value 1 = pages are in this category, value 0 = pages are not in this category.
+/*
- struct pm_scan_arg - Pagemap ioctl argument
- @size: Size of the structure
- @flags: Flags for the IOCTL
- @start: Starting address of the region
- @len: Length of the region (All the pages in this length are included)
Maybe `scan_start`, `scan_len` - so that there is a better distinction from the structure's `size` field?
As start and len already communicate the meaning. We are making things more verbose.
We are describing (in the name) only that it is a range, but not of what or what purpose. That information is only in the docstring, but it is harder to get by someone just reading the code.
Agreed. But I'm using same names, start and len which mincore (a historic syscall) is using. I've followed mincore here.
mincore() doesn't take parameters as a struct, but as three positional arguments (whose names don't matter nor appear at call point) - I wouldn't take it as a precedent for structure field naming.
- @vec: Address of page_region struct array for output
- @vec_len: Length of the page_region struct array
- @max_pages: Optional max return pages
- @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
- */
I skipped most of the page walk implementation as maybe the comments above could make it simpler. Reading this patch and the documentation I still feel confused about how the filtering/limiting parameters
I'm really sad to hear this. I've been working on making this series from so many revisions. I was hopping that it would make complete sense to reviewers and later to users.
What do you think is missing which is restricting these patches getting accepted to upstream?
should affect GET, WP and WP+GET. Should they limit the pages walked (and WP-ed)? Or only the GET's output? How about GET+WP case?
The address range needs to be walked until max pages pages are found, user buffer is full or whole range is walked. If the page will be added to user buffer or not depends on the selection criteria (*masks). There is no difference in case of walk for GET, WP and GET+WP. Only that WP doesn't take any user buffer and just WPs the whole region.
Ok, then this intent (if I understand correctly) does not entirely match the implementation. Let's split up the conditions:
- The address range needs to be walked until max pages pages are found
current implementation: the address range is walked until max pages matching masks (incl. return_mask) are reported by GET (or until end of range if GET is not requested). Maybe we need to describe what "found" means here?
Found means all the pages which are found to be fulfilling the masks and we have added it to the user buffer. I can add the comment on top of pagemap_scan_private struct? But I don't think that it is difficult to understand the meaning of found_pages and also we compare it with max_pages which makes things very easy to understand.
After fixing `return_mask` and the selection/action split I think "pages found" might work - as now the count will be exactly what pages match the required/anyof/excluded criteria.
- user buffer is full
Matches implementation except in GET+WP edge cases.
I'm not sure which edge case you are referring to? Probably for hugetlb error return case?
Yes, that one.
Best Regards Michał Mirosław
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote:
(A quick reply to answer open questions in case they help the next version.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 8:14 PM, Michał Mirosław wrote:
On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 3:36 AM, Michał Mirosław wrote:
On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
[...]
> + if (cur_buf->bitmap == bitmap && > + cur_buf->start + cur_buf->len * PAGE_SIZE == addr) { > + cur_buf->len += n_pages; > + p->found_pages += n_pages; > + } else { > + if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len) > + return -ENOMEM;
Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel ran out of memory when allocating, not that there is no space in a user-provided buffer.
There are 3 kinds of return values here:
- PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the
page walk from next entry
- 0 ---> continue the page walk
- -ENOMEM --> Abort the page walk from current entry, user buffer is full
which is not error, but only a stop signal. This -ENOMEM is just differentiater from (1). This -ENOMEM is for internal use and isn't returned to user.
But why ENOSPC is not good here? I was used before, I think.
-ENOSPC is being returned in form of true error from pagemap_scan_hugetlb_entry(). So I'd to remove -ENOSPC from here as it wasn't true error here, it was only a way to abort the walk immediately. I'm liking the following erturn code from here now:
#define PM_SCAN_BUFFER_FULL (-256)
I guess this will be reworked anyway, but I'd prefer this didn't need custom errors etc. If we agree to decoupling the selection and GET output, it could be:
bool is_interesting_page(p, flags); // this one does the required/anyof/excluded match size_t output_range(p, start, len, flags); // this one fills the output vector and returns how many pages were fit
In this setup, `is_interesting_page() && (n_out = output_range()) < n_pages` means this is the final range, no more will fit. And if `n_out == 0` then no pages fit and no WP is needed (no other special cases).
Right now, pagemap_scan_output() performs the work of both of these two functions. The part can be broken into is_interesting_pages() and we can leave the remaining part as it is.
Saying that n_out < n_pages tells us the buffer is full covers one case. But there is case of maximum pages have been found and walk needs to be aborted.
This case is exactly what `n_out < n_pages` will cover (if scan_output uses max_pages properly to limit n_out). Isn't it that when the buffer is full we want to abort the scan always (with WP if `n_out > 0`)?
For flags name: PM_REQUIRE_WRITE_ACCESS? Or Is it intended to be checked only if doing WP (as the current name suggests) and so it would be redundant as WP currently requires `p->required_mask = PAGE_IS_WRITTEN`?
This is intended to indicate that if userfaultfd is needed. If PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if userfaultfd has been initialized for this memory. I'll rename to PM_SCAN_REQUIRE_UFFD.
Why do we need that check? Wouldn't `is_written = false` work for vmas not registered via uffd?
UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region for it to report correct written values on the memory region. Without UFFD WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state undefined. If user hasn't initialized memory with UFFD, he has no right to set is_written = false.
How about calculating `is_written = is_uffd_registered() && is_uffd_wp()`? This would enable a user to apply GET+WP for the whole address space of a process regardless of whether all of it is registered.
I wouldn't want to check if uffd is registered again and again. This is why we are doing it only once every walk in pagemap_scan_test_walk().
There is no need to do the checks repeatedly. If I understand the code correctly, uffd registration is per-vma, so it can be communicated from test_walk to entry/hole callbacks via a field in pagemap_scan_private.
While here, I wonder if we really need to fail the call if there are unknown bits in those masks set: if this bit set is expanded with another category flags, a newer userspace run on older kernel would get EINVAL even if the "treat unknown as 0" be what it requires. There is no simple way in the API to discover what bits the kernel supports. We could allow a no-op (no WP nor GET) call to help with that and then rejecting unknown bits would make sense.
I've not seen any examples of this. But I've seen examples of returning error if kernel doesn't support a feature. Each new feature comes with a kernel version, greater than this version support this feature. If user is trying to use advanced feature which isn't present in a kernel, we should return error and not proceed to confuse the user/kernel. In fact if we look at userfaultfd_api(), we return error immediately if feature has some bit set which kernel doesn't support.
I think we should have a way of detecting the supported flags if we don't want a forward compatibility policy for flags here. Maybe it would be enough to allow all the no-op combinations for this purpose?
Again I don't think UFFD is doing anything like this.
If it's cheap and easy to provide a user with a way to detect the supported features - why not do it?
Best Regards Michał Mirosław
On Thu, 15 Jun 2023 at 16:52, Michał Mirosław emmir@google.com wrote:
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote:
(A quick reply to answer open questions in case they help the next version.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 8:14 PM, Michał Mirosław wrote:
On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 3:36 AM, Michał Mirosław wrote: > On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum > usama.anjum@collabora.com wrote: > For flags name: PM_REQUIRE_WRITE_ACCESS? > Or Is it intended to be checked only if doing WP (as the current name > suggests) and so it would be redundant as WP currently requires > `p->required_mask = PAGE_IS_WRITTEN`? This is intended to indicate that if userfaultfd is needed. If PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if userfaultfd has been initialized for this memory. I'll rename to PM_SCAN_REQUIRE_UFFD.
Why do we need that check? Wouldn't `is_written = false` work for vmas not registered via uffd?
UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region for it to report correct written values on the memory region. Without UFFD WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state undefined. If user hasn't initialized memory with UFFD, he has no right to set is_written = false.
How about calculating `is_written = is_uffd_registered() && is_uffd_wp()`? This would enable a user to apply GET+WP for the whole address space of a process regardless of whether all of it is registered.
I wouldn't want to check if uffd is registered again and again. This is why we are doing it only once every walk in pagemap_scan_test_walk().
There is no need to do the checks repeatedly. If I understand the code correctly, uffd registration is per-vma, so it can be communicated from test_walk to entry/hole callbacks via a field in pagemap_scan_private.
Actually... this could be exposed as a page category for the filter (e.g. PAGE_USES_UFFD_WP) and then you could just make the ioctl() to work for your usecase without tracking the ranges at the userspace side.
Best Regards Michał Mirosław
Please review the v19. I hope to get your reviewed by tag soon.
On 6/15/23 7:58 PM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 16:52, Michał Mirosław emmir@google.com wrote:
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote:
(A quick reply to answer open questions in case they help the next version.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 8:14 PM, Michał Mirosław wrote:
On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum usama.anjum@collabora.com wrote: > > On 6/14/23 3:36 AM, Michał Mirosław wrote: >> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum >> usama.anjum@collabora.com wrote: >> For flags name: PM_REQUIRE_WRITE_ACCESS? >> Or Is it intended to be checked only if doing WP (as the current name >> suggests) and so it would be redundant as WP currently requires >> `p->required_mask = PAGE_IS_WRITTEN`? > This is intended to indicate that if userfaultfd is needed. If > PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if > userfaultfd has been initialized for this memory. I'll rename to > PM_SCAN_REQUIRE_UFFD.
Why do we need that check? Wouldn't `is_written = false` work for vmas not registered via uffd?
UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region for it to report correct written values on the memory region. Without UFFD WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state undefined. If user hasn't initialized memory with UFFD, he has no right to set is_written = false.
How about calculating `is_written = is_uffd_registered() && is_uffd_wp()`? This would enable a user to apply GET+WP for the whole address space of a process regardless of whether all of it is registered.
I wouldn't want to check if uffd is registered again and again. This is why we are doing it only once every walk in pagemap_scan_test_walk().
There is no need to do the checks repeatedly. If I understand the code correctly, uffd registration is per-vma, so it can be communicated from test_walk to entry/hole callbacks via a field in pagemap_scan_private.
Actually... this could be exposed as a page category for the filter (e.g. PAGE_USES_UFFD_WP) and then you could just make the ioctl() to work for your usecase without tracking the ranges at the userspace side.
I'm not sure about page category. ASAIK the current check isn't bad when we already mention in documentation that memory must be registered with UFFD WP before using write feature of the IOCTL.
Just like mincore mentions in documentation that user buffer will be filled with values based on the length of the region. Kernel doesn't care if user had provided smaller buffer and kernel overwrites because of user's own issue. I want to follow the same path. If user doesn't read documentation and follow it, he should be punished with the error.
Best Regards Michał Mirosław
On Thu, 15 Jun 2023 at 17:16, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
Please review the v19. I hope to get your reviewed by tag soon.
On 6/15/23 7:58 PM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 16:52, Michał Mirosław emmir@google.com wrote:
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote:
(A quick reply to answer open questions in case they help the next version.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 8:14 PM, Michał Mirosław wrote: > On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum > usama.anjum@collabora.com wrote: >> >> On 6/14/23 3:36 AM, Michał Mirosław wrote: >>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum >>> usama.anjum@collabora.com wrote: >>> For flags name: PM_REQUIRE_WRITE_ACCESS? >>> Or Is it intended to be checked only if doing WP (as the current name >>> suggests) and so it would be redundant as WP currently requires >>> `p->required_mask = PAGE_IS_WRITTEN`? >> This is intended to indicate that if userfaultfd is needed. If >> PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if >> userfaultfd has been initialized for this memory. I'll rename to >> PM_SCAN_REQUIRE_UFFD. > > Why do we need that check? Wouldn't `is_written = false` work for vmas > not registered via uffd? UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region for it to report correct written values on the memory region. Without UFFD WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state undefined. If user hasn't initialized memory with UFFD, he has no right to set is_written = false.
How about calculating `is_written = is_uffd_registered() && is_uffd_wp()`? This would enable a user to apply GET+WP for the whole address space of a process regardless of whether all of it is registered.
I wouldn't want to check if uffd is registered again and again. This is why we are doing it only once every walk in pagemap_scan_test_walk().
There is no need to do the checks repeatedly. If I understand the code correctly, uffd registration is per-vma, so it can be communicated from test_walk to entry/hole callbacks via a field in pagemap_scan_private.
Actually... this could be exposed as a page category for the filter (e.g. PAGE_USES_UFFD_WP) and then you could just make the ioctl() to work for your usecase without tracking the ranges at the userspace side.
I'm not sure about page category. ASAIK the current check isn't bad when we already mention in documentation that memory must be registered with UFFD WP before using write feature of the IOCTL.
You could relax the (documentation) rule to be "WP works only on ranges registeder via UFFD for ASYNC_WP". That way you allow people, who don't read documentation to shoot their foot, but don't block people that know what they are doing from exploiting the nice feature that they don't need to track all the WP-registered ranges calling the ioctl() for each one and instead can just call it once for the whole address space.
Best Regards Michał Mirosław
On 6/16/23 1:00 AM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 17:16, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
Please review the v19. I hope to get your reviewed by tag soon.
On 6/15/23 7:58 PM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 16:52, Michał Mirosław emmir@google.com wrote:
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote:
(A quick reply to answer open questions in case they help the next version.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote: > On 6/14/23 8:14 PM, Michał Mirosław wrote: >> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum >> usama.anjum@collabora.com wrote: >>> >>> On 6/14/23 3:36 AM, Michał Mirosław wrote: >>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum >>>> usama.anjum@collabora.com wrote: >>>> For flags name: PM_REQUIRE_WRITE_ACCESS? >>>> Or Is it intended to be checked only if doing WP (as the current name >>>> suggests) and so it would be redundant as WP currently requires >>>> `p->required_mask = PAGE_IS_WRITTEN`? >>> This is intended to indicate that if userfaultfd is needed. If >>> PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if >>> userfaultfd has been initialized for this memory. I'll rename to >>> PM_SCAN_REQUIRE_UFFD. >> >> Why do we need that check? Wouldn't `is_written = false` work for vmas >> not registered via uffd? > UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region > for it to report correct written values on the memory region. Without UFFD > WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state > undefined. If user hasn't initialized memory with UFFD, he has no right to > set is_written = false.
How about calculating `is_written = is_uffd_registered() && is_uffd_wp()`? This would enable a user to apply GET+WP for the whole address space of a process regardless of whether all of it is registered.
I wouldn't want to check if uffd is registered again and again. This is why we are doing it only once every walk in pagemap_scan_test_walk().
There is no need to do the checks repeatedly. If I understand the code correctly, uffd registration is per-vma, so it can be communicated from test_walk to entry/hole callbacks via a field in pagemap_scan_private.
Actually... this could be exposed as a page category for the filter (e.g. PAGE_USES_UFFD_WP) and then you could just make the ioctl() to work for your usecase without tracking the ranges at the userspace side.
I'm not sure about page category. ASAIK the current check isn't bad when we already mention in documentation that memory must be registered with UFFD WP before using write feature of the IOCTL.
You could relax the (documentation) rule to be "WP works only on ranges registeder via UFFD for ASYNC_WP". That way you allow people, who don't read documentation to shoot their foot,
They'll shoot their foot and have no idea why they are getting false results. Isn't it better that they get error and they go read the documentation and then register with UFFD first? I think, returning error is way better than not returning anything.
but don't block people that know what they are doing from exploiting the nice feature that they don't need to track all the WP-registered ranges calling the ioctl() for each one and instead can just call it once for the whole address space.
Best Regards Michał Mirosław
On 6/15/23 7:52 PM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote:
(A quick reply to answer open questions in case they help the next version.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 8:14 PM, Michał Mirosław wrote:
On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 3:36 AM, Michał Mirosław wrote: > On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum > usama.anjum@collabora.com wrote:
[...]
>> + if (cur_buf->bitmap == bitmap && >> + cur_buf->start + cur_buf->len * PAGE_SIZE == addr) { >> + cur_buf->len += n_pages; >> + p->found_pages += n_pages; >> + } else { >> + if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len) >> + return -ENOMEM; > > Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel > ran out of memory when allocating, not that there is no space in a > user-provided buffer. There are 3 kinds of return values here:
- PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the
page walk from next entry
- 0 ---> continue the page walk
- -ENOMEM --> Abort the page walk from current entry, user buffer is full
which is not error, but only a stop signal. This -ENOMEM is just differentiater from (1). This -ENOMEM is for internal use and isn't returned to user.
But why ENOSPC is not good here? I was used before, I think.
-ENOSPC is being returned in form of true error from pagemap_scan_hugetlb_entry(). So I'd to remove -ENOSPC from here as it wasn't true error here, it was only a way to abort the walk immediately. I'm liking the following erturn code from here now:
#define PM_SCAN_BUFFER_FULL (-256)
I guess this will be reworked anyway, but I'd prefer this didn't need custom errors etc. If we agree to decoupling the selection and GET output, it could be:
bool is_interesting_page(p, flags); // this one does the required/anyof/excluded match size_t output_range(p, start, len, flags); // this one fills the output vector and returns how many pages were fit
In this setup, `is_interesting_page() && (n_out = output_range()) < n_pages` means this is the final range, no more will fit. And if `n_out == 0` then no pages fit and no WP is needed (no other special cases).
Right now, pagemap_scan_output() performs the work of both of these two functions. The part can be broken into is_interesting_pages() and we can leave the remaining part as it is.
Saying that n_out < n_pages tells us the buffer is full covers one case. But there is case of maximum pages have been found and walk needs to be aborted.
This case is exactly what `n_out < n_pages` will cover (if scan_output uses max_pages properly to limit n_out). Isn't it that when the buffer is full we want to abort the scan always (with WP if `n_out > 0`)?
Wouldn't it be duplication of condition if buffer is full inside pagemap_scan_output() and just outside it. Inside pagemap_scan_output() we check if we have space before putting data inside it. I'm using this same condition to indicate that buffer is full.
> For flags name: PM_REQUIRE_WRITE_ACCESS? > Or Is it intended to be checked only if doing WP (as the current name > suggests) and so it would be redundant as WP currently requires > `p->required_mask = PAGE_IS_WRITTEN`? This is intended to indicate that if userfaultfd is needed. If PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if userfaultfd has been initialized for this memory. I'll rename to PM_SCAN_REQUIRE_UFFD.
Why do we need that check? Wouldn't `is_written = false` work for vmas not registered via uffd?
UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region for it to report correct written values on the memory region. Without UFFD WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state undefined. If user hasn't initialized memory with UFFD, he has no right to set is_written = false.
How about calculating `is_written = is_uffd_registered() && is_uffd_wp()`? This would enable a user to apply GET+WP for the whole address space of a process regardless of whether all of it is registered.
I wouldn't want to check if uffd is registered again and again. This is why we are doing it only once every walk in pagemap_scan_test_walk().
There is no need to do the checks repeatedly. If I understand the code correctly, uffd registration is per-vma, so it can be communicated from test_walk to entry/hole callbacks via a field in pagemap_scan_private.
While here, I wonder if we really need to fail the call if there are unknown bits in those masks set: if this bit set is expanded with another category flags, a newer userspace run on older kernel would get EINVAL even if the "treat unknown as 0" be what it requires. There is no simple way in the API to discover what bits the kernel supports. We could allow a no-op (no WP nor GET) call to help with that and then rejecting unknown bits would make sense.
I've not seen any examples of this. But I've seen examples of returning error if kernel doesn't support a feature. Each new feature comes with a kernel version, greater than this version support this feature. If user is trying to use advanced feature which isn't present in a kernel, we should return error and not proceed to confuse the user/kernel. In fact if we look at userfaultfd_api(), we return error immediately if feature has some bit set which kernel doesn't support.
I think we should have a way of detecting the supported flags if we don't want a forward compatibility policy for flags here. Maybe it would be enough to allow all the no-op combinations for this purpose?
Again I don't think UFFD is doing anything like this.
If it's cheap and easy to provide a user with a way to detect the supported features - why not do it?
I'm sorry. But it would bring up something new and iterations will be needed to just play around. I like the UFFD way.
Best Regards Michał Mirosław
On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/15/23 7:52 PM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote:
(A quick reply to answer open questions in case they help the next version.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 8:14 PM, Michał Mirosław wrote:
On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum usama.anjum@collabora.com wrote: > > On 6/14/23 3:36 AM, Michał Mirosław wrote: >> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum >> usama.anjum@collabora.com wrote:
[...]
>>> + if (cur_buf->bitmap == bitmap && >>> + cur_buf->start + cur_buf->len * PAGE_SIZE == addr) { >>> + cur_buf->len += n_pages; >>> + p->found_pages += n_pages; >>> + } else { >>> + if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len) >>> + return -ENOMEM; >> >> Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel >> ran out of memory when allocating, not that there is no space in a >> user-provided buffer. > There are 3 kinds of return values here: > * PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the > page walk from next entry > * 0 ---> continue the page walk > * -ENOMEM --> Abort the page walk from current entry, user buffer is full > which is not error, but only a stop signal. This -ENOMEM is just > differentiater from (1). This -ENOMEM is for internal use and isn't > returned to user.
But why ENOSPC is not good here? I was used before, I think.
-ENOSPC is being returned in form of true error from pagemap_scan_hugetlb_entry(). So I'd to remove -ENOSPC from here as it wasn't true error here, it was only a way to abort the walk immediately. I'm liking the following erturn code from here now:
#define PM_SCAN_BUFFER_FULL (-256)
I guess this will be reworked anyway, but I'd prefer this didn't need custom errors etc. If we agree to decoupling the selection and GET output, it could be:
bool is_interesting_page(p, flags); // this one does the required/anyof/excluded match size_t output_range(p, start, len, flags); // this one fills the output vector and returns how many pages were fit
In this setup, `is_interesting_page() && (n_out = output_range()) < n_pages` means this is the final range, no more will fit. And if `n_out == 0` then no pages fit and no WP is needed (no other special cases).
Right now, pagemap_scan_output() performs the work of both of these two functions. The part can be broken into is_interesting_pages() and we can leave the remaining part as it is.
Saying that n_out < n_pages tells us the buffer is full covers one case. But there is case of maximum pages have been found and walk needs to be aborted.
This case is exactly what `n_out < n_pages` will cover (if scan_output uses max_pages properly to limit n_out). Isn't it that when the buffer is full we want to abort the scan always (with WP if `n_out > 0`)?
Wouldn't it be duplication of condition if buffer is full inside pagemap_scan_output() and just outside it. Inside pagemap_scan_output() we check if we have space before putting data inside it. I'm using this same condition to indicate that buffer is full.
I'm not sure what do you mean? The buffer-full conditions would be checked in ..scan_output() and communicated to the caller by returning N less than `n_pages` passed in. This is exactly how e.g. read() works: if you get less than requested you've hit the end of the file. If the file happens to have size that is equal to the provided buffer length, the next read() will return 0.
While here, I wonder if we really need to fail the call if there are unknown bits in those masks set: if this bit set is expanded with another category flags, a newer userspace run on older kernel would get EINVAL even if the "treat unknown as 0" be what it requires. There is no simple way in the API to discover what bits the kernel supports. We could allow a no-op (no WP nor GET) call to help with that and then rejecting unknown bits would make sense.
I've not seen any examples of this. But I've seen examples of returning error if kernel doesn't support a feature. Each new feature comes with a kernel version, greater than this version support this feature. If user is trying to use advanced feature which isn't present in a kernel, we should return error and not proceed to confuse the user/kernel. In fact if we look at userfaultfd_api(), we return error immediately if feature has some bit set which kernel doesn't support.
I think we should have a way of detecting the supported flags if we don't want a forward compatibility policy for flags here. Maybe it would be enough to allow all the no-op combinations for this purpose?
Again I don't think UFFD is doing anything like this.
If it's cheap and easy to provide a user with a way to detect the supported features - why not do it?
I'm sorry. But it would bring up something new and iterations will be needed to just play around. I like the UFFD way.
Let's then first agree on what would have to be changed. I guess we could leverage that `scan_len = 0` doesn't make much sense otherwise and let it be used to check the other fields for support.
Best Regards Michał Mirosław
On 6/16/23 1:07 AM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/15/23 7:52 PM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote:
(A quick reply to answer open questions in case they help the next version.)
On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/14/23 8:14 PM, Michał Mirosław wrote: > On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum > usama.anjum@collabora.com wrote: >> >> On 6/14/23 3:36 AM, Michał Mirosław wrote: >>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum >>> usama.anjum@collabora.com wrote:
[...]
>>>> + if (cur_buf->bitmap == bitmap && >>>> + cur_buf->start + cur_buf->len * PAGE_SIZE == addr) { >>>> + cur_buf->len += n_pages; >>>> + p->found_pages += n_pages; >>>> + } else { >>>> + if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len) >>>> + return -ENOMEM; >>> >>> Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel >>> ran out of memory when allocating, not that there is no space in a >>> user-provided buffer. >> There are 3 kinds of return values here: >> * PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the >> page walk from next entry >> * 0 ---> continue the page walk >> * -ENOMEM --> Abort the page walk from current entry, user buffer is full >> which is not error, but only a stop signal. This -ENOMEM is just >> differentiater from (1). This -ENOMEM is for internal use and isn't >> returned to user. > > But why ENOSPC is not good here? I was used before, I think. -ENOSPC is being returned in form of true error from pagemap_scan_hugetlb_entry(). So I'd to remove -ENOSPC from here as it wasn't true error here, it was only a way to abort the walk immediately. I'm liking the following erturn code from here now:
#define PM_SCAN_BUFFER_FULL (-256)
I guess this will be reworked anyway, but I'd prefer this didn't need custom errors etc. If we agree to decoupling the selection and GET output, it could be:
bool is_interesting_page(p, flags); // this one does the required/anyof/excluded match size_t output_range(p, start, len, flags); // this one fills the output vector and returns how many pages were fit
In this setup, `is_interesting_page() && (n_out = output_range()) < n_pages` means this is the final range, no more will fit. And if `n_out == 0` then no pages fit and no WP is needed (no other special cases).
Right now, pagemap_scan_output() performs the work of both of these two functions. The part can be broken into is_interesting_pages() and we can leave the remaining part as it is.
Saying that n_out < n_pages tells us the buffer is full covers one case. But there is case of maximum pages have been found and walk needs to be aborted.
This case is exactly what `n_out < n_pages` will cover (if scan_output uses max_pages properly to limit n_out). Isn't it that when the buffer is full we want to abort the scan always (with WP if `n_out > 0`)?
Wouldn't it be duplication of condition if buffer is full inside pagemap_scan_output() and just outside it. Inside pagemap_scan_output() we check if we have space before putting data inside it. I'm using this same condition to indicate that buffer is full.
I'm not sure what do you mean? The buffer-full conditions would be checked in ..scan_output() and communicated to the caller by returning N less than `n_pages` passed in. This is exactly how e.g. read() works: if you get less than requested you've hit the end of the file. If the file happens to have size that is equal to the provided buffer length, the next read() will return 0.
Right now we have:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return PM_SCAN_BUFFER_FULL; if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;
pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret >= 0) // success make_UFFD_WP and flush else buffer_error
You are asking me to do:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return 0; if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;
pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret > 0) // success make_UFFD_WP and flush else if (ret == 0) // buffer full return PM_SCAN_BUFFER_FULL; else //other errors buffer_error
So you are asking me to go from consie code to write more lines of code. I would write more lines without any issue if it improves readability and logical sense. But I don't see here any benefit.
> While here, I wonder if we really need to fail the call if there are > unknown bits in those masks set: if this bit set is expanded with > another category flags, a newer userspace run on older kernel would > get EINVAL even if the "treat unknown as 0" be what it requires. > There is no simple way in the API to discover what bits the kernel > supports. We could allow a no-op (no WP nor GET) call to help with > that and then rejecting unknown bits would make sense. I've not seen any examples of this. But I've seen examples of returning error if kernel doesn't support a feature. Each new feature comes with a kernel version, greater than this version support this feature. If user is trying to use advanced feature which isn't present in a kernel, we should return error and not proceed to confuse the user/kernel. In fact if we look at userfaultfd_api(), we return error immediately if feature has some bit set which kernel doesn't support.
I think we should have a way of detecting the supported flags if we don't want a forward compatibility policy for flags here. Maybe it would be enough to allow all the no-op combinations for this purpose?
Again I don't think UFFD is doing anything like this.
If it's cheap and easy to provide a user with a way to detect the supported features - why not do it?
I'm sorry. But it would bring up something new and iterations will be needed to just play around. I like the UFFD way.
Let's then first agree on what would have to be changed. I guess we could leverage that `scan_len = 0` doesn't make much sense otherwise and let it be used to check the other fields for support.
We are making things more and more complex. I don't like multi-plexing variables. Can you give examples where multi-plexing has been done on variables inside linux kernel? Muti-plexing means user gives input and takes output from same variable. It makes variable double meaning.
Best Regards Michał Mirosław
On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/16/23 1:07 AM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/15/23 7:52 PM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote:
(A quick reply to answer open questions in case they help the next version.)
[...]
I guess this will be reworked anyway, but I'd prefer this didn't need custom errors etc. If we agree to decoupling the selection and GET output, it could be:
bool is_interesting_page(p, flags); // this one does the required/anyof/excluded match size_t output_range(p, start, len, flags); // this one fills the output vector and returns how many pages were fit
In this setup, `is_interesting_page() && (n_out = output_range()) < n_pages` means this is the final range, no more will fit. And if `n_out == 0` then no pages fit and no WP is needed (no other special cases).
Right now, pagemap_scan_output() performs the work of both of these two functions. The part can be broken into is_interesting_pages() and we can leave the remaining part as it is.
Saying that n_out < n_pages tells us the buffer is full covers one case. But there is case of maximum pages have been found and walk needs to be aborted.
This case is exactly what `n_out < n_pages` will cover (if scan_output uses max_pages properly to limit n_out). Isn't it that when the buffer is full we want to abort the scan always (with WP if `n_out > 0`)?
Wouldn't it be duplication of condition if buffer is full inside pagemap_scan_output() and just outside it. Inside pagemap_scan_output() we check if we have space before putting data inside it. I'm using this same condition to indicate that buffer is full.
I'm not sure what do you mean? The buffer-full conditions would be checked in ..scan_output() and communicated to the caller by returning N less than `n_pages` passed in. This is exactly how e.g. read() works: if you get less than requested you've hit the end of the file. If the file happens to have size that is equal to the provided buffer length, the next read() will return 0.
Right now we have:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return PM_SCAN_BUFFER_FULL; if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;
Why do you need to differentiate between those cases?
pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret >= 0) // success make_UFFD_WP and flush else buffer_error
You are asking me to do:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return 0;
if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;
This should be instead:
n_pages = min(p->max_pags - p_found_pages, n_pages) ... return n_pages;
pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret > 0) // success make_UFFD_WP and flush else if (ret == 0) // buffer full return PM_SCAN_BUFFER_FULL; else //other errors buffer_error
And this would be:
if (ret > 0 && WP) WP + flush
if (ret < n_pages) return -ENOSPC;
So you are asking me to go from consie code to write more lines of code. I would write more lines without any issue if it improves readability and logical sense. But I don't see here any benefit.
Please see the clarifications above.
Best Regards Michał Mirosław
On 6/19/23 1:16 PM, Michał Mirosław wrote:
On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/16/23 1:07 AM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/15/23 7:52 PM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
I'll send next revision now. On 6/14/23 11:00 PM, Michał Mirosław wrote: > (A quick reply to answer open questions in case they help the next version.)
[...]
> I guess this will be reworked anyway, but I'd prefer this didn't need > custom errors etc. If we agree to decoupling the selection and GET > output, it could be: > > bool is_interesting_page(p, flags); // this one does the > required/anyof/excluded match > size_t output_range(p, start, len, flags); // this one fills the > output vector and returns how many pages were fit > > In this setup, `is_interesting_page() && (n_out = output_range()) < > n_pages` means this is the final range, no more will fit. And if > `n_out == 0` then no pages fit and no WP is needed (no other special > cases). Right now, pagemap_scan_output() performs the work of both of these two functions. The part can be broken into is_interesting_pages() and we can leave the remaining part as it is.
Saying that n_out < n_pages tells us the buffer is full covers one case. But there is case of maximum pages have been found and walk needs to be aborted.
This case is exactly what `n_out < n_pages` will cover (if scan_output uses max_pages properly to limit n_out). Isn't it that when the buffer is full we want to abort the scan always (with WP if `n_out > 0`)?
Wouldn't it be duplication of condition if buffer is full inside pagemap_scan_output() and just outside it. Inside pagemap_scan_output() we check if we have space before putting data inside it. I'm using this same condition to indicate that buffer is full.
I'm not sure what do you mean? The buffer-full conditions would be checked in ..scan_output() and communicated to the caller by returning N less than `n_pages` passed in. This is exactly how e.g. read() works: if you get less than requested you've hit the end of the file. If the file happens to have size that is equal to the provided buffer length, the next read() will return 0.
Right now we have:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return PM_SCAN_BUFFER_FULL; if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;
Why do you need to differentiate between those cases?
pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret >= 0) // success make_UFFD_WP and flush else buffer_error
You are asking me to do:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return 0;
if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;This should be instead:
n_pages = min(p->max_pags - p_found_pages, n_pages) ... return n_pages;
You are missing the optimization here that we check for full buffer every time adding to user buffer. This was added to remove extra iteration of page walk if buffer is full already. The way you are suggesting will remove it.
So you are returning remaining pages to be found now. This doesn't seem right. If max_pages is 520, found_pages is 0 and n_pages is 512 before calling pagemap_scan_output(). found_pages would become 512 after adding 512 pages to output buffer. But n_pages would return 8 instead of 512. You were saying we should return the number of pages added to the output buffer.
pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret > 0) // success make_UFFD_WP and flush else if (ret == 0) // buffer full return PM_SCAN_BUFFER_FULL; else //other errors buffer_error
And this would be:
if (ret > 0 && WP) WP + flush
if (ret < n_pages) return -ENOSPC;
So you are asking me to go from consie code to write more lines of code. I would write more lines without any issue if it improves readability and logical sense. But I don't see here any benefit.
Please see the clarifications above.
Best Regards Michał Mirosław
On Tue, 20 Jun 2023 at 13:16, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/19/23 1:16 PM, Michał Mirosław wrote:
On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/16/23 1:07 AM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/15/23 7:52 PM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum usama.anjum@collabora.com wrote: > I'll send next revision now. > On 6/14/23 11:00 PM, Michał Mirosław wrote: >> (A quick reply to answer open questions in case they help the next version.)
[...]
>> I guess this will be reworked anyway, but I'd prefer this didn't need >> custom errors etc. If we agree to decoupling the selection and GET >> output, it could be: >> >> bool is_interesting_page(p, flags); // this one does the >> required/anyof/excluded match >> size_t output_range(p, start, len, flags); // this one fills the >> output vector and returns how many pages were fit >> >> In this setup, `is_interesting_page() && (n_out = output_range()) < >> n_pages` means this is the final range, no more will fit. And if >> `n_out == 0` then no pages fit and no WP is needed (no other special >> cases). > Right now, pagemap_scan_output() performs the work of both of these two > functions. The part can be broken into is_interesting_pages() and we can > leave the remaining part as it is. > > Saying that n_out < n_pages tells us the buffer is full covers one case. > But there is case of maximum pages have been found and walk needs to be > aborted.
This case is exactly what `n_out < n_pages` will cover (if scan_output uses max_pages properly to limit n_out). Isn't it that when the buffer is full we want to abort the scan always (with WP if `n_out > 0`)?
Wouldn't it be duplication of condition if buffer is full inside pagemap_scan_output() and just outside it. Inside pagemap_scan_output() we check if we have space before putting data inside it. I'm using this same condition to indicate that buffer is full.
I'm not sure what do you mean? The buffer-full conditions would be checked in ..scan_output() and communicated to the caller by returning N less than `n_pages` passed in. This is exactly how e.g. read() works: if you get less than requested you've hit the end of the file. If the file happens to have size that is equal to the provided buffer length, the next read() will return 0.
Right now we have:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return PM_SCAN_BUFFER_FULL; if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;
Why do you need to differentiate between those cases?
pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret >= 0) // success make_UFFD_WP and flush else buffer_error
You are asking me to do:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return 0;
if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;This should be instead:
n_pages = min(p->max_pags - p_found_pages, n_pages) ... return n_pages;
You are missing the optimization here that we check for full buffer every time adding to user buffer. This was added to remove extra iteration of page walk if buffer is full already. The way you are suggesting will remove it.
So you are returning remaining pages to be found now. This doesn't seem right. If max_pages is 520, found_pages is 0 and n_pages is 512 before calling pagemap_scan_output(). found_pages would become 512 after adding 512 pages to output buffer. But n_pages would return 8 instead of 512. You were saying we should return the number of pages added to the output buffer.
Ok, if we want this optimization, then i'd rework it so that we have:
bool pagemap_scan_output(..., int *n_pages) { limit n_pages; ... return have_more_room_in_output; }
The compiler should remove the pointer and memory storage for `n_pages` when inlining the function.
Best Regards Michał Mirosław
On 6/21/23 3:05 AM, Michał Mirosław wrote:
On Tue, 20 Jun 2023 at 13:16, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/19/23 1:16 PM, Michał Mirosław wrote:
On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/16/23 1:07 AM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/15/23 7:52 PM, Michał Mirosław wrote: > On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum > usama.anjum@collabora.com wrote: >> I'll send next revision now. >> On 6/14/23 11:00 PM, Michał Mirosław wrote: >>> (A quick reply to answer open questions in case they help the next version.)
[...]
>>> I guess this will be reworked anyway, but I'd prefer this didn't need >>> custom errors etc. If we agree to decoupling the selection and GET >>> output, it could be: >>> >>> bool is_interesting_page(p, flags); // this one does the >>> required/anyof/excluded match >>> size_t output_range(p, start, len, flags); // this one fills the >>> output vector and returns how many pages were fit >>> >>> In this setup, `is_interesting_page() && (n_out = output_range()) < >>> n_pages` means this is the final range, no more will fit. And if >>> `n_out == 0` then no pages fit and no WP is needed (no other special >>> cases). >> Right now, pagemap_scan_output() performs the work of both of these two >> functions. The part can be broken into is_interesting_pages() and we can >> leave the remaining part as it is. >> >> Saying that n_out < n_pages tells us the buffer is full covers one case. >> But there is case of maximum pages have been found and walk needs to be >> aborted. > > This case is exactly what `n_out < n_pages` will cover (if scan_output > uses max_pages properly to limit n_out). > Isn't it that when the buffer is full we want to abort the scan always > (with WP if `n_out > 0`)? Wouldn't it be duplication of condition if buffer is full inside pagemap_scan_output() and just outside it. Inside pagemap_scan_output() we check if we have space before putting data inside it. I'm using this same condition to indicate that buffer is full.
I'm not sure what do you mean? The buffer-full conditions would be checked in ..scan_output() and communicated to the caller by returning N less than `n_pages` passed in. This is exactly how e.g. read() works: if you get less than requested you've hit the end of the file. If the file happens to have size that is equal to the provided buffer length, the next read() will return 0.
Right now we have:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return PM_SCAN_BUFFER_FULL; if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;
Why do you need to differentiate between those cases?
pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret >= 0) // success make_UFFD_WP and flush else buffer_error
You are asking me to do:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return 0;
if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;This should be instead:
n_pages = min(p->max_pags - p_found_pages, n_pages) ... return n_pages;
You are missing the optimization here that we check for full buffer every time adding to user buffer. This was added to remove extra iteration of page walk if buffer is full already. The way you are suggesting will remove it.
So you are returning remaining pages to be found now. This doesn't seem right. If max_pages is 520, found_pages is 0 and n_pages is 512 before calling pagemap_scan_output(). found_pages would become 512 after adding 512 pages to output buffer. But n_pages would return 8 instead of 512. You were saying we should return the number of pages added to the output buffer.
Ok, if we want this optimization, then i'd rework it so that we have:
bool pagemap_scan_output(..., int *n_pages) { limit n_pages; ... return have_more_room_in_output; }
This is becoming more and more closer to what I have in the code. The only difference now is that you are asking me to not return the buffer full status from inside this function and instead there should be a input+output pointer to n_pages and the caller would return the buffer full status. As compared to the suggestion, the current form looks simpler. My earlier point ( https://lore.kernel.org/all/2e1b80f1-0385-0674-ae5f-9703a6ef975d@collabora.c... ) is valid again. I don't want to bring logic out of pagemap_scan_output(). This is internal function. There could be thousand ways how internal code can be written. I've really liked so many optimizations which you have advised. This isn't something worth doing. It would increase lines of code with no added readability benefit.
On Wed, 21 Jun 2023 at 06:44, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/21/23 3:05 AM, Michał Mirosław wrote:
On Tue, 20 Jun 2023 at 13:16, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/19/23 1:16 PM, Michał Mirosław wrote:
On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 6/16/23 1:07 AM, Michał Mirosław wrote:
On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum usama.anjum@collabora.com wrote: > On 6/15/23 7:52 PM, Michał Mirosław wrote: >> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum >> usama.anjum@collabora.com wrote: >>> I'll send next revision now. >>> On 6/14/23 11:00 PM, Michał Mirosław wrote: >>>> (A quick reply to answer open questions in case they help the next version.)
[...]
>>>> I guess this will be reworked anyway, but I'd prefer this didn't need >>>> custom errors etc. If we agree to decoupling the selection and GET >>>> output, it could be: >>>> >>>> bool is_interesting_page(p, flags); // this one does the >>>> required/anyof/excluded match >>>> size_t output_range(p, start, len, flags); // this one fills the >>>> output vector and returns how many pages were fit >>>> >>>> In this setup, `is_interesting_page() && (n_out = output_range()) < >>>> n_pages` means this is the final range, no more will fit. And if >>>> `n_out == 0` then no pages fit and no WP is needed (no other special >>>> cases). >>> Right now, pagemap_scan_output() performs the work of both of these two >>> functions. The part can be broken into is_interesting_pages() and we can >>> leave the remaining part as it is. >>> >>> Saying that n_out < n_pages tells us the buffer is full covers one case. >>> But there is case of maximum pages have been found and walk needs to be >>> aborted. >> >> This case is exactly what `n_out < n_pages` will cover (if scan_output >> uses max_pages properly to limit n_out). >> Isn't it that when the buffer is full we want to abort the scan always >> (with WP if `n_out > 0`)? > Wouldn't it be duplication of condition if buffer is full inside > pagemap_scan_output() and just outside it. Inside pagemap_scan_output() we > check if we have space before putting data inside it. I'm using this same > condition to indicate that buffer is full.
I'm not sure what do you mean? The buffer-full conditions would be checked in ..scan_output() and communicated to the caller by returning N less than `n_pages` passed in. This is exactly how e.g. read() works: if you get less than requested you've hit the end of the file. If the file happens to have size that is equal to the provided buffer length, the next read() will return 0.
Right now we have:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return PM_SCAN_BUFFER_FULL; if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;
Why do you need to differentiate between those cases?
pagemap_scan_pmd_entry(): ret = pagemap_scan_output(bitmap, p, start, n_pages); if (ret >= 0) // success make_UFFD_WP and flush else buffer_error
You are asking me to do:
pagemap_scan_output(): if (p->vec_buf_index >= p->vec_buf_len) return 0;
if (p->found_pages == p->max_pages) return PM_SCAN_FOUND_MAX_PAGES;This should be instead:
n_pages = min(p->max_pags - p_found_pages, n_pages) ... return n_pages;
You are missing the optimization here that we check for full buffer every time adding to user buffer. This was added to remove extra iteration of page walk if buffer is full already. The way you are suggesting will remove it.
So you are returning remaining pages to be found now. This doesn't seem right. If max_pages is 520, found_pages is 0 and n_pages is 512 before calling pagemap_scan_output(). found_pages would become 512 after adding 512 pages to output buffer. But n_pages would return 8 instead of 512. You were saying we should return the number of pages added to the output buffer.
Ok, if we want this optimization, then i'd rework it so that we have:
bool pagemap_scan_output(..., int *n_pages) { limit n_pages; ... return have_more_room_in_output; }
This is becoming more and more closer to what I have in the code. The only difference now is that you are asking me to not return the buffer full status from inside this function and instead there should be a input+output pointer to n_pages and the caller would return the buffer full status. As compared to the suggestion, the current form looks simpler. My earlier point ( https://lore.kernel.org/all/2e1b80f1-0385-0674-ae5f-9703a6ef975d@collabora.c... ) is valid again. I don't want to bring logic out of pagemap_scan_output(). This is internal function. There could be thousand ways how internal code can be written. I've really liked so many optimizations which you have advised. This isn't something worth doing. It would increase lines of code with no added readability benefit.
Yes, I try to suggest a minimal change. The benefit is that you don't need special error values anymore and so the cognitive load to understand the code flow is less. The idea is not to strictly save on lines typed, but on localising the information needed as much as possible. Also the distinction between BUFFER_FULL and FOUND_MAX_PAGES is only in which criteria was detected, but otherwise the code should behave the same way.
Best Regards Michał Mirosław
On 6/13/23 03:29, Muhammad Usama Anjum wrote:
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index b7b56871029c..47879c38ce2f 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -305,4 +305,57 @@ 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 pm_scan_arg)
Please update Documentation/userspace-api/ioctl/ioctl-number.rst also.
thanks.
On 6/14/23 9:09 AM, Randy Dunlap wrote:
On 6/13/23 03:29, Muhammad Usama Anjum wrote:
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h index b7b56871029c..47879c38ce2f 100644 --- a/include/uapi/linux/fs.h +++ b/include/uapi/linux/fs.h @@ -305,4 +305,57 @@ 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 pm_scan_arg)
Please update Documentation/userspace-api/ioctl/ioctl-number.rst also.
I've just checked this file and found out that the range is already correctly mentioned:
'f' 00-0F linux/fs.h conflict!
thanks.
New IOCTL and macros has been added in the kernel sources. Update the tools header file as well.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- tools/include/uapi/linux/fs.h | 53 +++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)
diff --git a/tools/include/uapi/linux/fs.h b/tools/include/uapi/linux/fs.h index b7b56871029c..47879c38ce2f 100644 --- a/tools/include/uapi/linux/fs.h +++ b/tools/include/uapi/linux/fs.h @@ -305,4 +305,57 @@ 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 pm_scan_arg) + +/* Bits are set in the bitmap of the page_region and masks in pm_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 in pages + * bitmap: Bits sets for the region + */ +struct page_region { + __u64 start; + __u64 len; + __u64 bitmap; +}; + +/* + * struct pm_scan_arg - Pagemap ioctl argument + * @size: Size of the structure + * @flags: Flags for the IOCTL + * @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 + * @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 pm_scan_arg { + __u64 size; + __u64 flags; + __u64 start; + __u64 len; + __u64 vec; + __u64 vec_len; + __u64 max_pages; + __u64 required_mask; + __u64 anyof_mask; + __u64 excluded_mask; + __u64 return_mask; +}; + +/* Supported flags */ +#define PM_SCAN_OP_GET (1 << 0) +#define PM_SCAN_OP_WP (1 << 1) + #endif /* _UAPI_LINUX_FS_H */
Add some explanation and method to use write-protection and written-to on memory range.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- Changes in v16: - Update the documentation
Changes in v11: - Add more documentation --- Documentation/admin-guide/mm/pagemap.rst | 58 ++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst index c8f380271cad..3b977526d4b3 100644 --- a/Documentation/admin-guide/mm/pagemap.rst +++ b/Documentation/admin-guide/mm/pagemap.rst @@ -227,3 +227,61 @@ Before Linux 3.11 pagemap bits 55-60 were used for "page-shift" (which is always 12 at most architectures). Since Linux 3.11 their meaning changes after first clear of soft-dirty bits. Since Linux 4.2 they are used for flags unconditionally. + +Pagemap Scan IOCTL +================== + +The ``PAGEMAP_SCAN`` IOCTL on the pagemap file can be used to get or optionally +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``). +- Find pages which have been written-to and/or write protect the pages atomically + (atomic ``PM_SCAN_OP_GET + PM_SCAN_OP_WP``) + +The ``struct pm_scan_arg`` is used as the argument of the IOCTL. + 1. The size of the ``struct pm_scan_arg`` must be specified in the ``size`` + field. This field will be helpful in recognizing the structure if extensions + are done later. + 2. The flags can be specified in the ``flags`` field. The ``PM_SCAN_OP_GET`` + and ``PM_SCAN_OP_WP`` are the only added flags at this time. + 3. The range is specified through ``start`` and ``len``. + 4. The output buffer of ``struct page_region`` array and size is specified in + ``vec`` and ``vec_len``. + 5. The optional maximum requested pages are specified in the ``max_pages``. + 6. The masks are specified in ``required_mask``, ``anyof_mask``, + ``excluded_ mask`` and ``return_mask``. + 1. To find if ``PAGE_IS_WRITTEN`` flag is set for pages which have + ``PAGE_IS_FILE`` set and ``PAGE_IS_SWAPPED`` un-set, ``required_mask`` + is set to ``PAGE_IS_FILE``, ``exclude_mask`` is set to + ``PAGE_IS_SWAPPED`` and ``return_mask`` is set to ``PAGE_IS_WRITTEN``. + The output buffer in ``vec`` and length must be specified in ``vec_len``. + 2. To find pages which have either ``PAGE_IS_FILE`` or ``PAGE_IS_SWAPPED`` + set, ``anyof_masks`` is set to ``PAGE_IS_FILE | PAGE_IS_SWAPPED``. + 3. To find written pages and engage write protect, ``PAGE_IS_WRITTEN`` is + specified in ``required_mask`` and ``return_mask``. In addition to + specifying the output buffer in ``vec`` and length in ``vec_len``, the + ``PM_SCAN_OP_WP`` is specified in ``flags`` to perform write protect + on the range as well. + +The ``PAGE_IS_WRITTEN`` flag can be considered as the better and correct +alternative of soft-dirty flag. It doesn't get affected by household chores (VMA +merging) of the kernel and hence the user can find the true soft-dirty pages +only. This IOCTL adds the atomic way to find which pages have been written and +write protect those pages again. This kind of operation is needed to efficiently +find out which pages have changed in the memory. + +To get information about which pages have been written-to or optionally write +protect the pages, following must be performed first in order: + 1. The userfaultfd file descriptor is created with ``userfaultfd`` syscall. + 2. The ``UFFD_FEATURE_WP_UNPOPULATED`` and ``UFFD_FEATURE_WP_ASYNC`` features + are set by ``UFFDIO_API`` IOCTL. + 3. The memory range is registered with ``UFFDIO_REGISTER_MODE_WP`` mode + through ``UFFDIO_REGISTER`` IOCTL. + 4. Then the any part of the registered memory or the whole memory region must + be write protected using ``PAGEMAP_SCAN`` IOCTL with flag ``PM_SCAN_OP_WP`` + or the ``UFFDIO_WRITEPROTECT`` IOCTL can be used. Both of these perform the + same operation. The former is better in terms of performance. + 5. Now the ``PAGEMAP_SCAN`` IOCTL can be used to either just find pages which + have been written-to and/or optionally write protect the pages as well.
Hi--
On 6/13/23 03:29, Muhammad Usama Anjum wrote:
Add some explanation and method to use write-protection and written-to on memory range.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
Changes in v16:
- Update the documentation
Changes in v11:
- Add more documentation
Documentation/admin-guide/mm/pagemap.rst | 58 ++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst index c8f380271cad..3b977526d4b3 100644 --- a/Documentation/admin-guide/mm/pagemap.rst +++ b/Documentation/admin-guide/mm/pagemap.rst @@ -227,3 +227,61 @@ Before Linux 3.11 pagemap bits 55-60 were used for "page-shift" (which is always 12 at most architectures). Since Linux 3.11 their meaning changes after first clear of soft-dirty bits. Since Linux 4.2 they are used for flags unconditionally.
+Pagemap Scan IOCTL +==================
+The ``PAGEMAP_SCAN`` IOCTL on the pagemap file can be used to get or optionally +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``),
written to
- file mapped (``PAGE_IS_FILE``), present (``PAGE_IS_PRESENT``) or swapped
- (``PAGE_IS_SWAPPED``).
+- Find pages which have been written-to and/or write protect the pages atomically
written to
- (atomic ``PM_SCAN_OP_GET + PM_SCAN_OP_WP``)
+The ``struct pm_scan_arg`` is used as the argument of the IOCTL.
- The size of the ``struct pm_scan_arg`` must be specified in the ``size``
- field. This field will be helpful in recognizing the structure if extensions
- are done later.
- The flags can be specified in the ``flags`` field. The ``PM_SCAN_OP_GET``
- and ``PM_SCAN_OP_WP`` are the only added flags at this time.
- The range is specified through ``start`` and ``len``.
- The output buffer of ``struct page_region`` array and size is specified in
- ``vec`` and ``vec_len``.
- The optional maximum requested pages are specified in the ``max_pages``.
- The masks are specified in ``required_mask``, ``anyof_mask``,
- ``excluded_ mask`` and ``return_mask``.
- To find if ``PAGE_IS_WRITTEN`` flag is set for pages which have
``PAGE_IS_FILE`` set and ``PAGE_IS_SWAPPED`` un-set, ``required_mask``
unset,
is set to ``PAGE_IS_FILE``, ``exclude_mask`` is set to``PAGE_IS_SWAPPED`` and ``return_mask`` is set to ``PAGE_IS_WRITTEN``.The output buffer in ``vec`` and length must be specified in ``vec_len``.
- To find pages which have either ``PAGE_IS_FILE`` or ``PAGE_IS_SWAPPED``
set, ``anyof_masks`` is set to ``PAGE_IS_FILE | PAGE_IS_SWAPPED``.
- To find written pages and engage write protect, ``PAGE_IS_WRITTEN`` is
specified in ``required_mask`` and ``return_mask``. In addition tospecifying the output buffer in ``vec`` and length in ``vec_len``, the``PM_SCAN_OP_WP`` is specified in ``flags`` to perform write protecton the range as well.+The ``PAGE_IS_WRITTEN`` flag can be considered as the better and correct +alternative of soft-dirty flag. It doesn't get affected by household chores (VMA
housekeeping ?
+merging) of the kernel and hence the user can find the true soft-dirty pages +only. This IOCTL adds the atomic way to find which pages have been written and +write protect those pages again. This kind of operation is needed to efficiently +find out which pages have changed in the memory.
+To get information about which pages have been written-to or optionally write
written to
+protect the pages, following must be performed first in order:
- The userfaultfd file descriptor is created with ``userfaultfd`` syscall.
- The ``UFFD_FEATURE_WP_UNPOPULATED`` and ``UFFD_FEATURE_WP_ASYNC`` features
- are 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 must
"the" or "any". Probably "any". Not both.
- be write protected using ``PAGEMAP_SCAN`` IOCTL with flag ``PM_SCAN_OP_WP``
- or the ``UFFDIO_WRITEPROTECT`` IOCTL can be used. Both of these perform the
- same operation. The former is better in terms of performance.
- Now the ``PAGEMAP_SCAN`` IOCTL can be used to either just find pages which
- have been written-to and/or optionally write protect the pages as well.
written to
Hi Randy,
Thank you for your review.
On 6/14/23 9:20 AM, Randy Dunlap wrote:
Hi--
On 6/13/23 03:29, Muhammad Usama Anjum wrote:
Add some explanation and method to use write-protection and written-to on memory range.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com
Changes in v16:
- Update the documentation
Changes in v11:
- Add more documentation
Documentation/admin-guide/mm/pagemap.rst | 58 ++++++++++++++++++++++++ 1 file changed, 58 insertions(+)
diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst index c8f380271cad..3b977526d4b3 100644 --- a/Documentation/admin-guide/mm/pagemap.rst +++ b/Documentation/admin-guide/mm/pagemap.rst @@ -227,3 +227,61 @@ Before Linux 3.11 pagemap bits 55-60 were used for "page-shift" (which is always 12 at most architectures). Since Linux 3.11 their meaning changes after first clear of soft-dirty bits. Since Linux 4.2 they are used for flags unconditionally.
+Pagemap Scan IOCTL +==================
+The ``PAGEMAP_SCAN`` IOCTL on the pagemap file can be used to get or optionally +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``),
written to
I'll make updates on all the places you have suggested.
- file mapped (``PAGE_IS_FILE``), present (``PAGE_IS_PRESENT``) or swapped
- (``PAGE_IS_SWAPPED``).
+- Find pages which have been written-to and/or write protect the pages atomically
written to
- (atomic ``PM_SCAN_OP_GET + PM_SCAN_OP_WP``)
+The ``struct pm_scan_arg`` is used as the argument of the IOCTL.
- The size of the ``struct pm_scan_arg`` must be specified in the ``size``
- field. This field will be helpful in recognizing the structure if extensions
- are done later.
- The flags can be specified in the ``flags`` field. The ``PM_SCAN_OP_GET``
- and ``PM_SCAN_OP_WP`` are the only added flags at this time.
- The range is specified through ``start`` and ``len``.
- The output buffer of ``struct page_region`` array and size is specified in
- ``vec`` and ``vec_len``.
- The optional maximum requested pages are specified in the ``max_pages``.
- The masks are specified in ``required_mask``, ``anyof_mask``,
- ``excluded_ mask`` and ``return_mask``.
- To find if ``PAGE_IS_WRITTEN`` flag is set for pages which have
``PAGE_IS_FILE`` set and ``PAGE_IS_SWAPPED`` un-set, ``required_mask``unset,
is set to ``PAGE_IS_FILE``, ``exclude_mask`` is set to``PAGE_IS_SWAPPED`` and ``return_mask`` is set to ``PAGE_IS_WRITTEN``.The output buffer in ``vec`` and length must be specified in ``vec_len``.
- To find pages which have either ``PAGE_IS_FILE`` or ``PAGE_IS_SWAPPED``
set, ``anyof_masks`` is set to ``PAGE_IS_FILE | PAGE_IS_SWAPPED``.
- To find written pages and engage write protect, ``PAGE_IS_WRITTEN`` is
specified in ``required_mask`` and ``return_mask``. In addition tospecifying the output buffer in ``vec`` and length in ``vec_len``, the``PM_SCAN_OP_WP`` is specified in ``flags`` to perform write protecton the range as well.+The ``PAGE_IS_WRITTEN`` flag can be considered as the better and correct +alternative of soft-dirty flag. It doesn't get affected by household chores (VMA
housekeeping ?+merging) of the kernel and hence the user can find the true soft-dirty pages +only. This IOCTL adds the atomic way to find which pages have been written and +write protect those pages again. This kind of operation is needed to efficiently +find out which pages have changed in the memory.
+To get information about which pages have been written-to or optionally write
written to+protect the pages, following must be performed first in order:
- The userfaultfd file descriptor is created with ``userfaultfd`` syscall.
- The ``UFFD_FEATURE_WP_UNPOPULATED`` and ``UFFD_FEATURE_WP_ASYNC`` features
- are 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 must
"the" or "any". Probably "any". Not both.
- be write protected using ``PAGEMAP_SCAN`` IOCTL with flag ``PM_SCAN_OP_WP``
- or the ``UFFDIO_WRITEPROTECT`` IOCTL can be used. Both of these perform the
- same operation. The former is better in terms of performance.
- Now the ``PAGEMAP_SCAN`` IOCTL can be used to either just find pages which
- have been written-to and/or optionally write protect the pages as well.
written to
Add pagemap ioctl tests. Add several different types of tests to judge the correction of the interface.
Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com --- Changes in v18: - Rebase on top of 20230613 (Resolve conflict in Makefile) - Add temp files to .gitignore
Changes in v17: - Rebase on top of next-20230525
Changes in v16: - Added yet more tests which is a randomization test case to catch the corner cases - Add reset by exclusive PM_SCAN_OP_WP as well
Changes in v13: - Update tests and rebase Makefile
Changes in v12: - Updates and add more memory type tests
Changes in v11: - Rebase on top of next-20230216 and update tests
Chages in v7: - Add and update all test cases
Changes in v6: - Rename variables
Changes in v4: - Updated all the tests to conform to new IOCTL
Changes in v3: - Add another test to do sanity of flags
Changes in v2: - Update the tests to use the ioctl interface instead of syscall --- TAP version 13 1..92 ok 1 sanity_tests_sd memory size must be valid ok 2 sanity_tests_sd output buffer must be specified ok 3 sanity_tests_sd output buffer size must be valid ok 4 sanity_tests_sd wrong flag specified ok 5 sanity_tests_sd flag has extra bits specified ok 6 sanity_tests_sd no selection mask is specified ok 7 sanity_tests_sd no return mask is specified ok 8 sanity_tests_sd wrong return mask specified ok 9 sanity_tests_sd mixture of correct and wrong flag ok 10 sanity_tests_sd PAGEMAP_BITS_ALL cannot be specified with PM_SCAN_OP_WP ok 11 sanity_tests_sd Clear area with larger vec size ok 12 sanity_tests_sd Repeated pattern of written and non-written pages ok 13 sanity_tests_sd Repeated pattern of written and non-written pages in parts ok 14 sanity_tests_sd Repeated pattern of written and non-written pages max_pages ok 15 sanity_tests_sd only get 2 written pages and clear them as well ok 16 sanity_tests_sd Two regions ok 17 sanity_tests_sd Smaller max_pages ok 18 Smaller vec 46 50 ok 19 Page testing: all new pages must not be written (dirty) ok 20 Page testing: all pages must be written (dirty) ok 21 Page testing: all pages dirty other than first and the last one ok 22 Page testing: PM_SCAN_OP_WP ok 23 Page testing: only middle page dirty ok 24 Page testing: only two middle pages dirty ok 25 Large Page testing: all new pages must not be written (dirty) ok 26 Large Page testing: all pages must be written (dirty) ok 27 Large Page testing: all pages dirty other than first and the last one ok 28 Large Page testing: PM_SCAN_OP_WP ok 29 Large Page testing: only middle page dirty ok 30 Large Page testing: only two middle pages dirty ok 31 Huge page testing: all new pages must not be written (dirty) ok 32 Huge page testing: all pages must be written (dirty) ok 33 Huge page testing: all pages dirty other than first and the last one ok 34 Huge page testing: PM_SCAN_OP_WP ok 35 Huge page testing: only middle page dirty ok 36 Huge page testing: only two middle pages dirty ok 37 Hugetlb shmem testing: all new pages must not be written (dirty) ok 38 Hugetlb shmem testing: all pages must be written (dirty) ok 39 Hugetlb shmem testing: all pages dirty other than first and the last one ok 40 Hugetlb shmem testing: PM_SCAN_OP_WP ok 41 Hugetlb shmem testing: only middle page dirty ok 42 Hugetlb shmem testing: only two middle pages dirty ok 43 Hugetlb mem testing: all new pages must not be written (dirty) ok 44 Hugetlb mem testing: all pages must be written (dirty) ok 45 Hugetlb mem testing: all pages dirty other than first and the last one ok 46 Hugetlb mem testing: PM_SCAN_OP_WP ok 47 Hugetlb mem testing: only middle page dirty ok 48 Hugetlb mem testing: only two middle pages dirty ok 49 File memory testing: all new pages must not be written (dirty) ok 50 File memory testing: all pages must be written (dirty) ok 51 File memory testing: all pages dirty other than first and the last one ok 52 File memory testing: PM_SCAN_OP_WP ok 53 File memory testing: only middle page dirty ok 54 File memory testing: only two middle pages dirty ok 55 File anonymous memory testing: all new pages must not be written (dirty) ok 56 File anonymous memory testing: all pages must be written (dirty) ok 57 File anonymous memory testing: all pages dirty other than first and the last one ok 58 File anonymous memory testing: PM_SCAN_OP_WP ok 59 File anonymous memory testing: only middle page dirty ok 60 File anonymous memory testing: only two middle pages dirty ok 61 hpage_unit_tests all new huge page must not be written (dirty) ok 62 hpage_unit_tests all the huge page must not be written ok 63 hpage_unit_tests all the huge page must be written and clear ok 64 hpage_unit_tests only middle page written ok 65 hpage_unit_tests clear first half of huge page ok 66 hpage_unit_tests clear first half of huge page with limited buffer ok 67 hpage_unit_tests clear second half huge page ok 68 hpage_unit_tests get half huge page ok 69 hpage_unit_tests get half huge page ok 70 Test test_simple ok 71 mprotect_tests Both pages written ok 72 mprotect_tests Both pages are not written (dirty) ok 73 mprotect_tests Both pages written after remap and mprotect ok 74 mprotect_tests Clear and make the pages written ok 75 transact_test count 192 ok 76 transact_test count 0 ok 77 transact_test Extra pages 0 (0.0%), extra thread faults 0. ok 78 sanity_tests clear op can only be specified with PAGE_IS_WRITTEN ok 79 sanity_tests required_mask specified ok 80 sanity_tests anyof_mask specified ok 81 sanity_tests excluded_mask specified ok 82 sanity_tests required_mask and anyof_mask specified ok 83 sanity_tests Get sd and present pages with anyof_mask ok 84 sanity_tests Get all the pages with required_mask ok 85 sanity_tests Get sd and present pages with required_mask and anyof_mask ok 86 sanity_tests Don't get sd pages ok 87 sanity_tests Don't get present pages ok 88 sanity_tests Find written present pages with return mask ok 89 sanity_tests Memory mapped file ok 90 sanity_tests Read/write to private memory mapped file ok 91 unmapped_region_tests Get status of pages ok 92 userfaultfd_tests all new pages must not be written (dirty) # Totals: pass:92 fail:0 xfail:0 xpass:0 skip:0 error:0 --- tools/testing/selftests/mm/.gitignore | 2 + tools/testing/selftests/mm/Makefile | 3 +- tools/testing/selftests/mm/config | 1 + tools/testing/selftests/mm/pagemap_ioctl.c | 1459 ++++++++++++++++++++ tools/testing/selftests/mm/run_vmtests.sh | 4 + 5 files changed, 1468 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/mm/pagemap_ioctl.c mode change 100644 => 100755 tools/testing/selftests/mm/run_vmtests.sh
diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore index 7e2a982383c0..3bc799592895 100644 --- a/tools/testing/selftests/mm/.gitignore +++ b/tools/testing/selftests/mm/.gitignore @@ -17,6 +17,8 @@ mremap_dontunmap mremap_test on-fault-limit transhuge-stress +pagemap_ioctl +*.tmp* protection_keys protection_keys_32 protection_keys_64 diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index e6cd60ca9e48..08d3676c091b 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -30,7 +30,7 @@ MACHINE ?= $(shell echo $(uname_M) | sed -e 's/aarch64.*/arm64/' -e 's/ppc64.*/p MAKEFLAGS += --no-builtin-rules
CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -LDLIBS = -lrt -lpthread +LDLIBS = -lrt -lpthread -lm
TEST_GEN_PROGS = cow TEST_GEN_PROGS += compaction_test @@ -56,6 +56,7 @@ TEST_GEN_PROGS += mrelease_test TEST_GEN_PROGS += mremap_dontunmap TEST_GEN_PROGS += mremap_test TEST_GEN_PROGS += on-fault-limit +TEST_GEN_PROGS += pagemap_ioctl TEST_GEN_PROGS += thuge-gen TEST_GEN_PROGS += transhuge-stress TEST_GEN_PROGS += uffd-stress diff --git a/tools/testing/selftests/mm/config b/tools/testing/selftests/mm/config index be087c4bc396..4309916f629e 100644 --- a/tools/testing/selftests/mm/config +++ b/tools/testing/selftests/mm/config @@ -1,5 +1,6 @@ CONFIG_SYSVIPC=y CONFIG_USERFAULTFD=y +CONFIG_PTE_MARKER_UFFD_WP=y CONFIG_TEST_VMALLOC=m CONFIG_DEVICE_PRIVATE=y CONFIG_TEST_HMM=m diff --git a/tools/testing/selftests/mm/pagemap_ioctl.c b/tools/testing/selftests/mm/pagemap_ioctl.c new file mode 100644 index 000000000000..1c2d0034c0dc --- /dev/null +++ b/tools/testing/selftests/mm/pagemap_ioctl.c @@ -0,0 +1,1459 @@ +// SPDX-License-Identifier: GPL-2.0 +#define _GNU_SOURCE +#include <stdio.h> +#include <fcntl.h> +#include <string.h> +#include <sys/mman.h> +#include <errno.h> +#include <malloc.h> +#include "vm_util.h" +#include "../kselftest.h" +#include <linux/types.h> +#include <linux/memfd.h> +#include <linux/userfaultfd.h> +#include <linux/fs.h> +#include <sys/ioctl.h> +#include <sys/stat.h> +#include <math.h> +#include <asm/unistd.h> +#include <pthread.h> +#include <sys/resource.h> +#include <assert.h> +#include <sys/ipc.h> +#include <sys/shm.h> + +#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 TEST_ITERATIONS 100 +#define PAGEMAP "/proc/self/pagemap" +int pagemap_fd; +int uffd; +int page_size; +int hpage_size; + +static long pagemap_ioctl(void *start, int len, void *vec, int vec_len, int flag, + int max_pages, long required_mask, long anyof_mask, long excluded_mask, + long return_mask) +{ + struct pm_scan_arg arg; + + arg.start = (uintptr_t)start; + arg.len = len; + arg.vec = (uintptr_t)vec; + arg.vec_len = vec_len; + arg.flags = flag; + arg.size = sizeof(struct pm_scan_arg); + arg.max_pages = max_pages; + arg.required_mask = required_mask; + arg.anyof_mask = anyof_mask; + arg.excluded_mask = excluded_mask; + arg.return_mask = return_mask; + + return ioctl(pagemap_fd, PAGEMAP_SCAN, &arg); +} + +int init_uffd(void) +{ + struct uffdio_api uffdio_api; + + uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK); + if (uffd == -1) + ksft_exit_fail_msg("uffd syscall failed\n"); + + uffdio_api.api = UFFD_API; + uffdio_api.features = UFFD_FEATURE_WP_UNPOPULATED | UFFD_FEATURE_WP_ASYNC | + UFFD_FEATURE_WP_HUGETLBFS_SHMEM; + if (ioctl(uffd, UFFDIO_API, &uffdio_api)) + ksft_exit_fail_msg("UFFDIO_API\n"); + + if (!(uffdio_api.api & UFFDIO_REGISTER_MODE_WP) || + !(uffdio_api.features & UFFD_FEATURE_WP_UNPOPULATED) || + !(uffdio_api.features & UFFD_FEATURE_WP_ASYNC) || + !(uffdio_api.features & UFFD_FEATURE_WP_HUGETLBFS_SHMEM)) + ksft_exit_fail_msg("UFFDIO_API error %llu\n", uffdio_api.api); + + return 0; +} + +int wp_init(void *lpBaseAddress, int dwRegionSize) +{ + struct uffdio_register uffdio_register; + struct uffdio_writeprotect wp; + + uffdio_register.range.start = (unsigned long)lpBaseAddress; + uffdio_register.range.len = dwRegionSize; + uffdio_register.mode = UFFDIO_REGISTER_MODE_WP; + if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register)) + ksft_exit_fail_msg("ioctl(UFFDIO_REGISTER) %d %s\n", errno, strerror(errno)); + + if (!(uffdio_register.ioctls & UFFDIO_WRITEPROTECT)) + ksft_exit_fail_msg("ioctl set is incorrect\n"); + + wp.range.start = (unsigned long)lpBaseAddress; + wp.range.len = dwRegionSize; + wp.mode = UFFDIO_WRITEPROTECT_MODE_WP; + + if (ioctl(uffd, UFFDIO_WRITEPROTECT, &wp)) + ksft_exit_fail_msg("ioctl(UFFDIO_WRITEPROTECT)\n"); + + return 0; +} + +int wp_free(void *lpBaseAddress, int dwRegionSize) +{ + struct uffdio_register uffdio_register; + + uffdio_register.range.start = (unsigned long)lpBaseAddress; + uffdio_register.range.len = dwRegionSize; + uffdio_register.mode = UFFDIO_REGISTER_MODE_WP; + if (ioctl(uffd, UFFDIO_UNREGISTER, &uffdio_register.range)) + ksft_exit_fail_msg("ioctl unregister failure\n"); + return 0; +} + +int wp_addr_range(void *lpBaseAddress, int dwRegionSize) +{ + struct uffdio_writeprotect wp; + + if (rand() % 2) { + wp.range.start = (unsigned long)lpBaseAddress; + wp.range.len = dwRegionSize; + wp.mode = UFFDIO_WRITEPROTECT_MODE_WP; + + if (ioctl(uffd, UFFDIO_WRITEPROTECT, &wp)) + ksft_exit_fail_msg("ioctl(UFFDIO_WRITEPROTECT)\n"); + } else { + if (pagemap_ioctl(lpBaseAddress, dwRegionSize, NULL, 0, PM_SCAN_OP_WP, + 0, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN) < 0) + ksft_exit_fail_msg("error %d %d %s\n", 1, errno, strerror(errno)); + } + + return 0; +} + +void *gethugetlb_mem(int size, int *shmid) +{ + char *mem; + + if (shmid) { + *shmid = shmget(2, size, SHM_HUGETLB | IPC_CREAT | SHM_R | SHM_W); + if (*shmid < 0) + return NULL; + + mem = shmat(*shmid, 0, 0); + if (mem == (char *)-1) { + shmctl(*shmid, IPC_RMID, NULL); + ksft_exit_fail_msg("Shared memory attach failure\n"); + } + } else { + mem = mmap(NULL, size, PROT_READ | PROT_WRITE, + MAP_ANONYMOUS | MAP_HUGETLB | MAP_PRIVATE, -1, 0); + if (mem == MAP_FAILED) + return NULL; + } + + return mem; +} + +int userfaultfd_tests(void) +{ + int mem_size, vec_size, written, num_pages = 16; + char *mem, *vec; + + mem_size = num_pages * page_size; + mem = mmap(NULL, mem_size, PROT_NONE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + + wp_init(mem, mem_size); + + /* Change protection of pages differently */ + mprotect(mem, mem_size/8, PROT_READ|PROT_WRITE); + mprotect(mem + 1 * mem_size/8, mem_size/8, PROT_READ); + mprotect(mem + 2 * mem_size/8, mem_size/8, PROT_READ|PROT_WRITE); + mprotect(mem + 3 * mem_size/8, mem_size/8, PROT_READ); + mprotect(mem + 4 * mem_size/8, mem_size/8, PROT_READ|PROT_WRITE); + mprotect(mem + 5 * mem_size/8, mem_size/8, PROT_NONE); + mprotect(mem + 6 * mem_size/8, mem_size/8, PROT_READ|PROT_WRITE); + mprotect(mem + 7 * mem_size/8, mem_size/8, PROT_READ); + + wp_addr_range(mem + (mem_size/16), mem_size - 2 * (mem_size/8)); + wp_addr_range(mem, mem_size); + + vec_size = mem_size/page_size; + vec = malloc(sizeof(struct page_region) * vec_size); + + written = pagemap_ioctl(mem, mem_size, vec, 1, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + vec_size - 2, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + ksft_test_result(written == 0, "%s all new pages must not be written (dirty)\n", __func__); + + wp_free(mem, mem_size); + munmap(mem, mem_size); + free(vec); + return 0; +} + +int get_reads(struct page_region *vec, int vec_size) +{ + int i, sum = 0; + + for (i = 0; i < vec_size; i++) + sum += vec[i].len; + + return sum; +} + +int sanity_tests_sd(void) +{ + int mem_size, vec_size, ret, ret2, ret3, i, num_pages = 10, total_pages = 0; + int total_writes, total_reads, reads, count; + struct page_region *vec, *vec2; + char *mem, *m[2]; + + vec_size = 100; + mem_size = num_pages * page_size; + + vec = malloc(sizeof(struct page_region) * vec_size); + if (!vec) + ksft_exit_fail_msg("error nomem\n"); + + vec2 = malloc(sizeof(struct page_region) * vec_size); + if (!vec2) + ksft_exit_fail_msg("error nomem\n"); + + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + + wp_init(mem, mem_size); + wp_addr_range(mem, mem_size); + + /* 1. wrong operation */ + ksft_test_result(pagemap_ioctl(mem, 0, vec, vec_size, PM_SCAN_OP_GET, + 0, PAGEMAP_BITS_ALL, 0, 0, PAGEMAP_BITS_ALL) < 0, + "%s memory size must be valid\n", __func__); + + ksft_test_result(pagemap_ioctl(mem, mem_size, NULL, vec_size, PM_SCAN_OP_GET, + 0, PAGEMAP_BITS_ALL, 0, 0, PAGEMAP_BITS_ALL) < 0, + "%s output buffer must be specified\n", __func__); + + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, 0, PM_SCAN_OP_GET, + 0, PAGEMAP_BITS_ALL, 0, 0, PAGEMAP_BITS_ALL) < 0, + "%s output buffer size must be valid\n", __func__); + + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, -1, + 0, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN) < 0, + "%s wrong flag specified\n", __func__); + + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, + PM_SCAN_OP_GET | PM_SCAN_OP_WP | 0xFF, + 0, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN) < 0, + "%s flag has extra bits specified\n", __func__); + + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, + 0, 0, 0, 0, PAGE_IS_WRITTEN) < 0, + "%s no selection mask is specified\n", __func__); + + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, + 0, PAGE_IS_WRITTEN, PAGE_IS_WRITTEN, 0, 0) < 0, + "%s no return mask is specified\n", __func__); + + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, + 0, PAGE_IS_WRITTEN, 0, 0, 0x1000) < 0, + "%s wrong return mask specified\n", __func__); + + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + 0, 0xFFF, PAGE_IS_WRITTEN, 0, PAGE_IS_WRITTEN) < 0, + "%s mixture of correct and wrong flag\n", __func__); + + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + 0, 0, 0, PAGEMAP_BITS_ALL, PAGE_IS_WRITTEN) < 0, + "%s PAGEMAP_BITS_ALL cannot be specified with PM_SCAN_OP_WP\n", __func__); + + /* 2. Clear area with larger vec size */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + ksft_test_result(ret >= 0, "%s Clear area with larger vec size\n", __func__); + + /* 3. Repeated pattern of written and non-written pages */ + for (i = 0; i < mem_size; i += 2 * page_size) + mem[i]++; + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, PAGE_IS_WRITTEN, 0, + 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == mem_size/(page_size * 2), + "%s Repeated pattern of written and non-written pages\n", __func__); + + /* 4. Repeated pattern of written and non-written pages in parts */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + num_pages/2 - 2, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ret2 = pagemap_ioctl(mem, mem_size, vec, 2, PM_SCAN_OP_GET, 0, PAGE_IS_WRITTEN, 0, 0, + PAGE_IS_WRITTEN); + if (ret2 < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret2, errno, strerror(errno)); + + ret3 = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + 0, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret3 < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret3, errno, strerror(errno)); + + ksft_test_result((ret + ret3) == num_pages/2 && ret2 == 2, + "%s Repeated pattern of written and non-written pages in parts\n", + __func__); + + /* 5. Repeated pattern of written and non-written pages max_pages */ + for (i = 0; i < mem_size; i += 2 * page_size) + mem[i]++; + mem[(mem_size/page_size - 1) * page_size]++; + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + num_pages/2, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ret2 = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + 0, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret2 < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret2, errno, strerror(errno)); + + ksft_test_result(ret == num_pages/2 && ret2 == 1, + "%s Repeated pattern of written and non-written pages max_pages\n", + __func__); + + /* 6. only get 2 dirty pages and clear them as well */ + vec_size = mem_size/page_size; + memset(mem, -1, mem_size); + + /* get and clear second and third pages */ + ret = pagemap_ioctl(mem + page_size, 2 * page_size, vec, 1, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + 2, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ret2 = pagemap_ioctl(mem, mem_size, vec2, vec_size, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret2 < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret2, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == 2 && + vec[0].start == (uintptr_t)(mem + page_size) && + ret2 == 2 && vec2[0].len == 1 && vec2[0].start == (uintptr_t)mem && + vec2[1].len == vec_size - 3 && + vec2[1].start == (uintptr_t)(mem + 3 * page_size), + "%s only get 2 written pages and clear them as well\n", __func__); + + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 7. Two regions */ + m[0] = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (m[0] == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + m[1] = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (m[1] == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + + wp_init(m[0], mem_size); + wp_init(m[1], mem_size); + wp_addr_range(m[0], mem_size); + wp_addr_range(m[1], mem_size); + + memset(m[0], 'a', mem_size); + memset(m[1], 'b', mem_size); + + wp_addr_range(m[0], mem_size); + + ret = pagemap_ioctl(m[1], mem_size, vec, 1, PM_SCAN_OP_GET, 0, PAGE_IS_WRITTEN, 0, 0, + PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == mem_size/page_size, + "%s Two regions\n", __func__); + + wp_free(m[0], mem_size); + wp_free(m[1], mem_size); + munmap(m[0], mem_size); + munmap(m[1], mem_size); + + free(vec); + free(vec2); + + /* 8. Smaller vec */ + mem_size = 1050 * page_size; + vec_size = mem_size/(page_size*2); + + vec = malloc(sizeof(struct page_region) * vec_size); + if (!vec) + ksft_exit_fail_msg("error nomem\n"); + + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + + wp_init(mem, mem_size); + wp_addr_range(mem, mem_size); + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + for (i = 0; i < mem_size/page_size; i += 2) + mem[i * page_size]++; + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + mem_size/(page_size*5), PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + total_pages += ret; + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + mem_size/(page_size*5), PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + total_pages += ret; + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + mem_size/(page_size*5), PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + total_pages += ret; + + ksft_test_result(total_pages == mem_size/(page_size*2), "%s Smaller max_pages\n", __func__); + + free(vec); + wp_free(mem, mem_size); + munmap(mem, mem_size); + total_pages = 0; + + /* 9. Smaller vec */ + mem_size = 10000 * page_size; + vec_size = 50;//mem_size/(page_size * 10); + + vec = malloc(sizeof(struct page_region) * vec_size); + if (!vec) + ksft_exit_fail_msg("error nomem\n"); + + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + + wp_init(mem, mem_size); + wp_addr_range(mem, mem_size); + + for (count = 0; count < TEST_ITERATIONS; count++) { + total_writes = total_reads = 0; + + for (i = 0; i < mem_size; i += page_size) { + if (rand() % 2) { + mem[i]++; + total_writes++; + } + } + + while (total_reads < total_writes) { + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, + PM_SCAN_OP_GET | PM_SCAN_OP_WP, + 0, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + if (ret > vec_size) + break; + + reads = get_reads(vec, ret); + total_reads += reads; + } + + if (total_reads != total_writes) + break; + } + + ksft_test_result(count == TEST_ITERATIONS, "Smaller vec %d %d\n", ret, vec_size); + + free(vec); + wp_free(mem, mem_size); + munmap(mem, mem_size); + + return 0; +} + +int base_tests(char *prefix, char *mem, int mem_size, int skip) +{ + int vec_size, written; + struct page_region *vec, *vec2; + + if (skip) { + ksft_test_result_skip("%s all new pages must not be written (dirty)\n", prefix); + ksft_test_result_skip("%s all pages must be written (dirty)\n", prefix); + ksft_test_result_skip("%s all pages dirty other than first and the last one\n", + prefix); + ksft_test_result_skip("%s PM_SCAN_OP_WP\n", prefix); + ksft_test_result_skip("%s only middle page dirty\n", prefix); + ksft_test_result_skip("%s only two middle pages dirty\n", prefix); + return 0; + } + + vec_size = mem_size/page_size; + vec = malloc(sizeof(struct page_region) * vec_size); + vec2 = malloc(sizeof(struct page_region) * vec_size); + + /* 1. all new pages must be not be written (dirty) */ + written = pagemap_ioctl(mem, mem_size, vec, 1, PM_SCAN_OP_GET | PM_SCAN_OP_WP, vec_size - 2, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + ksft_test_result(written == 0, "%s all new pages must not be written (dirty)\n", prefix); + + /* 2. all pages must be written */ + memset(mem, -1, mem_size); + + written = pagemap_ioctl(mem, mem_size, vec, 1, PM_SCAN_OP_GET, 0, PAGE_IS_WRITTEN, 0, 0, + PAGE_IS_WRITTEN); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + ksft_test_result(written == 1 && vec[0].len == mem_size/page_size, + "%s all pages must be written (dirty)\n", prefix); + + /* 3. all pages dirty other than first and the last one */ + written = pagemap_ioctl(mem, mem_size, vec, 1, PM_SCAN_OP_GET | PM_SCAN_OP_WP, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + memset(mem + page_size, 0, mem_size - (2 * page_size)); + + written = pagemap_ioctl(mem, mem_size, vec, 1, PM_SCAN_OP_GET | PM_SCAN_OP_WP, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + ksft_test_result(written == 1 && vec[0].len >= vec_size - 2 && vec[0].len <= vec_size, + "%s all pages dirty other than first and the last one\n", prefix); + + written = pagemap_ioctl(mem, mem_size, vec, 1, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + ksft_test_result(written == 0, + "%s PM_SCAN_OP_WP\n", prefix); + + /* 4. only middle page dirty */ + written = pagemap_ioctl(mem, mem_size, vec, 1, PM_SCAN_OP_GET | PM_SCAN_OP_WP, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + mem[vec_size/2 * page_size]++; + + written = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, PAGE_IS_WRITTEN, + 0, 0, PAGE_IS_WRITTEN); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + ksft_test_result(written == 1 && vec[0].len >= 1, + "%s only middle page dirty\n", prefix); + + /* 5. only two middle pages dirty and walk over only middle pages */ + written = pagemap_ioctl(mem, mem_size, vec, 1, PM_SCAN_OP_GET | PM_SCAN_OP_WP, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + mem[vec_size/2 * page_size]++; + mem[(vec_size/2 + 1) * page_size]++; + + written = pagemap_ioctl(&mem[vec_size/2 * page_size], 2 * page_size, vec, 1, PM_SCAN_OP_GET, + 0, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + ksft_test_result(written == 1 && vec[0].start == (uintptr_t)(&mem[vec_size/2 * page_size]) + && vec[0].len == 2, + "%s only two middle pages dirty\n", prefix); + + free(vec); + free(vec2); + return 0; +} + +void *gethugepage(int map_size) +{ + int ret; + char *map; + + map = memalign(hpage_size, map_size); + if (!map) + ksft_exit_fail_msg("memalign failed %d %s\n", errno, strerror(errno)); + + ret = madvise(map, map_size, MADV_HUGEPAGE); + if (ret) + return NULL; + + memset(map, 0, map_size); + + return map; +} + +int hpage_unit_tests(void) +{ + char *map; + int ret, ret2; + size_t num_pages = 10; + int map_size = hpage_size * num_pages; + int vec_size = map_size/page_size; + struct page_region *vec, *vec2; + + vec = malloc(sizeof(struct page_region) * vec_size); + vec2 = malloc(sizeof(struct page_region) * vec_size); + if (!vec || !vec2) + ksft_exit_fail_msg("malloc failed\n"); + + map = gethugepage(map_size); + if (map) { + wp_init(map, map_size); + wp_addr_range(map, map_size); + + /* 1. all new huge page must not be written (dirty) */ + ret = pagemap_ioctl(map, map_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 0, "%s all new huge page must not be written (dirty)\n", + __func__); + + /* 2. all the huge page must not be written */ + ret = pagemap_ioctl(map, map_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 0, "%s all the huge page must not be written\n", __func__); + + /* 3. all the huge page must be written and clear dirty as well */ + memset(map, -1, map_size); + ret = pagemap_ioctl(map, map_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + 0, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].start == (uintptr_t)map && + vec[0].len == vec_size && vec[0].bitmap == PAGE_IS_WRITTEN, + "%s all the huge page must be written and clear\n", __func__); + + /* 4. only middle page written */ + wp_free(map, map_size); + free(map); + map = gethugepage(map_size); + wp_init(map, map_size); + wp_addr_range(map, map_size); + map[vec_size/2 * page_size]++; + + ret = pagemap_ioctl(map, map_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len > 0, + "%s only middle page written\n", __func__); + + wp_free(map, map_size); + free(map); + } else { + ksft_test_result_skip("%s all new huge page must be written\n", __func__); + ksft_test_result_skip("%s all the huge page must not be written\n", __func__); + ksft_test_result_skip("%s all the huge page must be written and clear\n", __func__); + ksft_test_result_skip("%s only middle page written\n", __func__); + } + + /* 5. clear first half of huge page */ + map = gethugepage(map_size); + if (map) { + wp_init(map, map_size); + wp_addr_range(map, map_size); + + memset(map, 0, map_size); + + wp_addr_range(map, map_size/2); + + ret = pagemap_ioctl(map, map_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == vec_size/2 && + vec[0].start == (uintptr_t)(map + map_size/2), + "%s clear first half of huge page\n", __func__); + wp_free(map, map_size); + free(map); + } else { + ksft_test_result_skip("%s clear first half of huge page\n", __func__); + } + + /* 6. clear first half of huge page with limited buffer */ + map = gethugepage(map_size); + if (map) { + wp_init(map, map_size); + wp_addr_range(map, map_size); + + memset(map, 0, map_size); + + ret = pagemap_ioctl(map, map_size, vec, vec_size, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + vec_size/2, PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ret = pagemap_ioctl(map, map_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == vec_size/2 && + vec[0].start == (uintptr_t)(map + map_size/2), + "%s clear first half of huge page with limited buffer\n", + __func__); + wp_free(map, map_size); + free(map); + } else { + ksft_test_result_skip("%s clear first half of huge page with limited buffer\n", + __func__); + } + + /* 7. clear second half of huge page */ + map = gethugepage(map_size); + if (map) { + wp_init(map, map_size); + wp_addr_range(map, map_size); + + memset(map, -1, map_size); + + ret = pagemap_ioctl(map + map_size/2, map_size/2, vec, vec_size, + PM_SCAN_OP_GET | PM_SCAN_OP_WP, vec_size/2, PAGE_IS_WRITTEN, 0, + 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ret = pagemap_ioctl(map, map_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == vec_size/2, + "%s clear second half huge page\n", __func__); + wp_free(map, map_size); + free(map); + } else { + ksft_test_result_skip("%s clear second half huge page\n", __func__); + } + + /* 8. get half huge page */ + map = gethugepage(map_size); + if (map) { + wp_init(map, map_size); + wp_addr_range(map, map_size); + + memset(map, -1, map_size); + usleep(100); + + ret = pagemap_ioctl(map, map_size, vec, 1, PM_SCAN_OP_GET | PM_SCAN_OP_WP, + hpage_size/(2*page_size), PAGE_IS_WRITTEN, 0, 0, + PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec[0].len == hpage_size/(2*page_size), + "%s get half huge page\n", __func__); + + ret2 = pagemap_ioctl(map, map_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN); + if (ret2 < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret2, errno, strerror(errno)); + + ksft_test_result(ret2 == 1 && vec[0].len == (map_size - hpage_size/2)/page_size, + "%s get half huge page\n", __func__); + + wp_free(map, map_size); + free(map); + } else { + ksft_test_result_skip("%s get half huge page\n", __func__); + ksft_test_result_skip("%s get half huge page\n", __func__); + } + + free(vec); + free(vec2); + return 0; +} + +int unmapped_region_tests(void) +{ + void *start = (void *)0x10000000; + int written, len = 0x00040000; + int vec_size = len / page_size; + struct page_region *vec = malloc(sizeof(struct page_region) * vec_size); + + /* 1. Get written pages */ + written = pagemap_ioctl(start, len, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGEMAP_NON_WRITTEN_BITS, 0, 0, PAGEMAP_NON_WRITTEN_BITS); + if (written < 0) + ksft_exit_fail_msg("error %d %d %s\n", written, errno, strerror(errno)); + + ksft_test_result(written >= 0, "%s Get status of pages\n", __func__); + + free(vec); + return 0; +} + +static void test_simple(void) +{ + int i; + char *map; + struct page_region vec; + + map = aligned_alloc(page_size, page_size); + if (!map) + ksft_exit_fail_msg("aligned_alloc failed\n"); + + wp_init(map, page_size); + wp_addr_range(map, page_size); + + for (i = 0 ; i < TEST_ITERATIONS; i++) { + if (pagemap_ioctl(map, page_size, &vec, 1, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN) == 1) { + ksft_print_msg("written bit was 1, but should be 0 (i=%d)\n", i); + break; + } + + wp_addr_range(map, page_size); + /* Write something to the page to get the written bit enabled on the page */ + map[0]++; + + if (pagemap_ioctl(map, page_size, &vec, 1, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN) == 0) { + ksft_print_msg("written bit was 0, but should be 1 (i=%d)\n", i); + break; + } + + wp_addr_range(map, page_size); + } + wp_free(map, page_size); + free(map); + + ksft_test_result(i == TEST_ITERATIONS, "Test %s\n", __func__); +} + +int sanity_tests(void) +{ + int mem_size, vec_size, ret, fd, i, buf_size; + struct page_region *vec; + char *mem, *fmem; + struct stat sbuf; + + /* 1. wrong operation */ + mem_size = 10 * page_size; + vec_size = mem_size / page_size; + + vec = malloc(sizeof(struct page_region) * vec_size); + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED || vec == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + + wp_init(mem, mem_size); + wp_addr_range(mem, mem_size); + + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, + PM_SCAN_OP_GET | PM_SCAN_OP_WP, 0, PAGEMAP_BITS_ALL, 0, 0, + PAGEMAP_BITS_ALL) < 0, + "%s clear op can only be specified with PAGE_IS_WRITTEN\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGEMAP_BITS_ALL, 0, 0, PAGEMAP_BITS_ALL) >= 0, + "%s required_mask specified\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, + 0, PAGEMAP_BITS_ALL, 0, PAGEMAP_BITS_ALL) >= 0, + "%s anyof_mask specified\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, + 0, 0, PAGEMAP_BITS_ALL, PAGEMAP_BITS_ALL) >= 0, + "%s excluded_mask specified\n", __func__); + ksft_test_result(pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGEMAP_BITS_ALL, PAGEMAP_BITS_ALL, 0, + PAGEMAP_BITS_ALL) >= 0, + "%s required_mask and anyof_mask specified\n", __func__); + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 2. Get sd and present pages with anyof_mask */ + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem, mem_size); + wp_addr_range(mem, mem_size); + + memset(mem, 0, mem_size); + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, + 0, PAGEMAP_BITS_ALL, 0, PAGEMAP_BITS_ALL); + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)mem && vec[0].len == vec_size && + vec[0].bitmap == (PAGE_IS_WRITTEN | PAGE_IS_PRESENT), + "%s Get sd and present pages with anyof_mask\n", __func__); + + /* 3. Get sd and present pages with required_mask */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGEMAP_BITS_ALL, 0, 0, PAGEMAP_BITS_ALL); + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)mem && vec[0].len == vec_size && + vec[0].bitmap == (PAGE_IS_WRITTEN | PAGE_IS_PRESENT), + "%s Get all the pages with required_mask\n", __func__); + + /* 4. Get sd and present pages with required_mask and anyof_mask */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, PAGE_IS_PRESENT, 0, PAGEMAP_BITS_ALL); + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)mem && vec[0].len == vec_size && + vec[0].bitmap == (PAGE_IS_WRITTEN | PAGE_IS_PRESENT), + "%s Get sd and present pages with required_mask and anyof_mask\n", + __func__); + + /* 5. Don't get sd pages */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, + 0, 0, PAGE_IS_WRITTEN, PAGEMAP_BITS_ALL); + ksft_test_result(ret == 0, "%s Don't get sd pages\n", __func__); + + /* 6. Don't get present pages */ + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, + 0, 0, PAGE_IS_PRESENT, PAGEMAP_BITS_ALL); + ksft_test_result(ret == 0, "%s Don't get present pages\n", __func__); + + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 8. Find written present pages with return mask */ + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem, mem_size); + wp_addr_range(mem, mem_size); + + memset(mem, 0, mem_size); + + ret = pagemap_ioctl(mem, mem_size, vec, vec_size, PM_SCAN_OP_GET, 0, + 0, PAGEMAP_BITS_ALL, 0, PAGE_IS_WRITTEN); + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)mem && vec[0].len == vec_size && + vec[0].bitmap == PAGE_IS_WRITTEN, + "%s Find written present pages with return mask\n", __func__); + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 9. Memory mapped file */ + fd = open(__FILE__, O_RDONLY); + if (fd < 0) + ksft_exit_fail_msg("%s Memory mapped file\n"); + + ret = stat(__FILE__, &sbuf); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + fmem = mmap(NULL, sbuf.st_size, PROT_READ, MAP_PRIVATE, fd, 0); + if (fmem == MAP_FAILED) + ksft_exit_fail_msg("error nomem %ld %s\n", errno, strerror(errno)); + + ret = pagemap_ioctl(fmem, sbuf.st_size, vec, vec_size, PM_SCAN_OP_GET, 0, + 0, PAGEMAP_NON_WRITTEN_BITS, 0, PAGEMAP_NON_WRITTEN_BITS); + + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)fmem && + vec[0].len == ceilf((float)sbuf.st_size/page_size) && + vec[0].bitmap == PAGE_IS_FILE, + "%s Memory mapped file\n", __func__); + + munmap(fmem, sbuf.st_size); + close(fd); + + /* 10. Create and read/write to a memory mapped file*/ + buf_size = page_size * 10; + + fd = open(__FILE__".tmp2", O_RDWR | O_CREAT, 0777); + if (fd < 0) + ksft_exit_fail_msg("Create and read/write to a memory mapped file: %s\n", + strerror(errno)); + + for (i = 0; i < buf_size; i++) + if (write(fd, "c", 1) < 0) + ksft_exit_fail_msg("Create and read/write to a memory mapped file\n"); + + fmem = mmap(NULL, buf_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0); + if (fmem == MAP_FAILED) + ksft_exit_fail_msg("error nomem %ld %s\n", errno, strerror(errno)); + + wp_init(fmem, buf_size); + wp_addr_range(fmem, buf_size); + + for (i = 0; i < buf_size; i++) + fmem[i] = i; + + ret = pagemap_ioctl(fmem, buf_size, vec, vec_size, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN | PAGE_IS_FILE, PAGE_IS_PRESENT | PAGE_IS_SWAPPED, 0, + PAGEMAP_BITS_ALL); + + ksft_test_result(ret >= 0 && vec[0].start == (uintptr_t)fmem && + vec[0].len == (buf_size/page_size) && + (vec[0].bitmap | PAGE_IS_WRITTEN) && (vec[0].bitmap | PAGE_IS_FILE), + "%s Read/write to private memory mapped file\n", __func__); + + wp_free(fmem, buf_size); + munmap(fmem, buf_size); + close(fd); + + free(vec); + return 0; +} + +int mprotect_tests(void) +{ + int ret; + char *mem, *mem2; + struct page_region vec; + int pagemap_fd = open("/proc/self/pagemap", O_RDONLY); + + if (pagemap_fd < 0) { + fprintf(stderr, "open() failed\n"); + exit(1); + } + + /* 1. Map two pages */ + mem = mmap(0, 2 * page_size, PROT_READ|PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem, 2 * page_size); + wp_addr_range(mem, 2 * page_size); + + /* Populate both pages. */ + memset(mem, 1, 2 * page_size); + + ret = pagemap_ioctl(mem, 2 * page_size, &vec, 1, PM_SCAN_OP_GET, 0, PAGE_IS_WRITTEN, + 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec.len == 2, "%s Both pages written\n", __func__); + + /* 2. Start tracking */ + wp_addr_range(mem, 2 * page_size); + + ksft_test_result(pagemap_ioctl(mem, 2 * page_size, &vec, 1, PM_SCAN_OP_GET, 0, + PAGE_IS_WRITTEN, 0, 0, PAGE_IS_WRITTEN) == 0, + "%s Both pages are not written (dirty)\n", __func__); + + /* 3. Remap the second page */ + mem2 = mmap(mem + page_size, page_size, PROT_READ|PROT_WRITE, + MAP_PRIVATE|MAP_ANON|MAP_FIXED, -1, 0); + if (mem2 == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem2, page_size); + wp_addr_range(mem2, page_size); + + /* Protect + unprotect. */ + mprotect(mem, page_size, PROT_NONE); + mprotect(mem, 2 * page_size, PROT_READ); + mprotect(mem, 2 * page_size, PROT_READ|PROT_WRITE); + + /* Modify both pages. */ + memset(mem, 2, 2 * page_size); + + /* Protect + unprotect. */ + mprotect(mem, page_size, PROT_NONE); + mprotect(mem, page_size, PROT_READ); + mprotect(mem, page_size, PROT_READ|PROT_WRITE); + + ret = pagemap_ioctl(mem, 2 * page_size, &vec, 1, PM_SCAN_OP_GET, 0, PAGE_IS_WRITTEN, + 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec.len == 2, + "%s Both pages written after remap and mprotect\n", __func__); + + /* 4. Clear and make the pages written */ + wp_addr_range(mem, 2 * page_size); + + memset(mem, 'A', 2 * page_size); + + ret = pagemap_ioctl(mem, 2 * page_size, &vec, 1, PM_SCAN_OP_GET, 0, PAGE_IS_WRITTEN, + 0, 0, PAGE_IS_WRITTEN); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + ksft_test_result(ret == 1 && vec.len == 2, + "%s Clear and make the pages written\n", __func__); + + wp_free(mem, 2 * page_size); + munmap(mem, 2 * page_size); + return 0; +} + +/* transact test */ +static const unsigned int nthreads = 6, pages_per_thread = 32, access_per_thread = 8; +static pthread_barrier_t start_barrier, end_barrier; +static unsigned int extra_thread_faults; +static unsigned int iter_count = 1000; +static volatile int finish; + +static ssize_t get_dirty_pages_reset(char *mem, unsigned int count, + int reset, int page_size) +{ + struct pm_scan_arg arg = {0}; + struct page_region rgns[256]; + int i, j, cnt, ret; + + arg.size = sizeof(struct pm_scan_arg); + arg.start = (uintptr_t)mem; + arg.max_pages = count; + arg.len = count * page_size; + arg.vec = (uintptr_t)rgns; + arg.vec_len = sizeof(rgns) / sizeof(*rgns); + arg.flags = PM_SCAN_OP_GET; + if (reset) + arg.flags |= PM_SCAN_OP_WP; + arg.required_mask = PAGE_IS_WRITTEN; + arg.return_mask = PAGE_IS_WRITTEN; + + ret = ioctl(pagemap_fd, PAGEMAP_SCAN, &arg); + if (ret < 0) + ksft_exit_fail_msg("ioctl failed\n"); + + cnt = 0; + for (i = 0; i < ret; ++i) { + if (rgns[i].bitmap != PAGE_IS_WRITTEN) + ksft_exit_fail_msg("wrong bitmap\n"); + + for (j = 0; j < rgns[i].len; ++j) + cnt++; + } + + return cnt; +} + +void *thread_proc(void *mem) +{ + int *m = mem; + long curr_faults, faults; + struct rusage r; + unsigned int i; + int ret; + + if (getrusage(RUSAGE_THREAD, &r)) + ksft_exit_fail_msg("getrusage\n"); + + curr_faults = r.ru_minflt; + + while (!finish) { + ret = pthread_barrier_wait(&start_barrier); + if (ret && ret != PTHREAD_BARRIER_SERIAL_THREAD) + ksft_exit_fail_msg("pthread_barrier_wait\n"); + + for (i = 0; i < access_per_thread; ++i) + __atomic_add_fetch(m + i * (0x1000 / sizeof(*m)), 1, __ATOMIC_SEQ_CST); + + ret = pthread_barrier_wait(&end_barrier); + if (ret && ret != PTHREAD_BARRIER_SERIAL_THREAD) + ksft_exit_fail_msg("pthread_barrier_wait\n"); + + if (getrusage(RUSAGE_THREAD, &r)) + ksft_exit_fail_msg("getrusage\n"); + + faults = r.ru_minflt - curr_faults; + if (faults < access_per_thread) + ksft_exit_fail_msg("faults < access_per_thread"); + + __atomic_add_fetch(&extra_thread_faults, faults - access_per_thread, + __ATOMIC_SEQ_CST); + curr_faults = r.ru_minflt; + } + + return NULL; +} + +static void transact_test(int page_size) +{ + unsigned int i, count, extra_pages; + pthread_t th; + char *mem; + int ret, c; + + if (pthread_barrier_init(&start_barrier, NULL, nthreads + 1)) + ksft_exit_fail_msg("pthread_barrier_init\n"); + + if (pthread_barrier_init(&end_barrier, NULL, nthreads + 1)) + ksft_exit_fail_msg("pthread_barrier_init\n"); + + mem = mmap(NULL, 0x1000 * nthreads * pages_per_thread, PROT_READ | PROT_WRITE, + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("Error mmap %s.\n", strerror(errno)); + + wp_init(mem, 0x1000 * nthreads * pages_per_thread); + wp_addr_range(mem, 0x1000 * nthreads * pages_per_thread); + + memset(mem, 0, 0x1000 * nthreads * pages_per_thread); + + count = get_dirty_pages_reset(mem, nthreads * pages_per_thread, 1, page_size); + ksft_test_result(count > 0, "%s count %d\n", __func__, count); + count = get_dirty_pages_reset(mem, nthreads * pages_per_thread, 1, page_size); + ksft_test_result(count == 0, "%s count %d\n", __func__, count); + + finish = 0; + for (i = 0; i < nthreads; ++i) + pthread_create(&th, NULL, thread_proc, mem + 0x1000 * i * pages_per_thread); + + extra_pages = 0; + for (i = 0; i < iter_count; ++i) { + count = 0; + + ret = pthread_barrier_wait(&start_barrier); + if (ret && ret != PTHREAD_BARRIER_SERIAL_THREAD) + ksft_exit_fail_msg("pthread_barrier_wait\n"); + + count = get_dirty_pages_reset(mem, nthreads * pages_per_thread, 1, + page_size); + + ret = pthread_barrier_wait(&end_barrier); + if (ret && ret != PTHREAD_BARRIER_SERIAL_THREAD) + ksft_exit_fail_msg("pthread_barrier_wait\n"); + + if (count > nthreads * access_per_thread) + ksft_exit_fail_msg("Too big count %d expected %d, iter %d\n", + count, nthreads * access_per_thread, i); + + c = get_dirty_pages_reset(mem, nthreads * pages_per_thread, 1, page_size); + count += c; + + if (c > nthreads * access_per_thread) { + ksft_test_result_fail(" %s count > nthreads\n", __func__); + return; + } + + if (count != nthreads * access_per_thread) { + /* + * The purpose of the test is to make sure that no page updates are lost + * when the page updates and read-resetting soft dirty flags are performed + * in parallel. However, it is possible that the application will get the + * soft dirty flags twice on the two consecutive read-resets. This seems + * unavoidable as soft dirty flag is handled in software through page faults + * in kernel. While the updating the flags is supposed to be synchronized + * between page fault handling and read-reset, it is possible that + * read-reset happens after page fault PTE update but before the application + * re-executes write instruction. So read-reset gets the flag, clears write + * access and application gets page fault again for the same write. + */ + if (count < nthreads * access_per_thread) { + ksft_test_result_fail("Lost update, iter %d, %d vs %d.\n", i, count, + nthreads * access_per_thread); + return; + } + + extra_pages += count - nthreads * access_per_thread; + } + } + + pthread_barrier_wait(&start_barrier); + finish = 1; + pthread_barrier_wait(&end_barrier); + + ksft_test_result_pass("%s Extra pages %u (%.1lf%%), extra thread faults %d.\n", __func__, + extra_pages, + 100.0 * extra_pages / (iter_count * nthreads * access_per_thread), + extra_thread_faults); +} + +int main(void) +{ + int mem_size, shmid, buf_size, fd, i, ret; + char *mem, *map, *fmem; + struct stat sbuf; + + ksft_print_header(); + ksft_set_plan(92); + + page_size = getpagesize(); + hpage_size = read_pmd_pagesize(); + + pagemap_fd = open(PAGEMAP, O_RDWR); + if (pagemap_fd < 0) + return -EINVAL; + + if (init_uffd()) + ksft_exit_fail_msg("uffd init failed\n"); + + /* + * Written (dirty) PTE bit tests + */ + + /* 1. Sanity testing */ + sanity_tests_sd(); + + /* 2. Normal page testing */ + mem_size = 10 * page_size; + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem, mem_size); + wp_addr_range(mem, mem_size); + + base_tests("Page testing:", mem, mem_size, 0); + + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 3. Large page testing */ + mem_size = 512 * 10 * page_size; + mem = mmap(NULL, mem_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); + if (mem == MAP_FAILED) + ksft_exit_fail_msg("error nomem\n"); + wp_init(mem, mem_size); + wp_addr_range(mem, mem_size); + + base_tests("Large Page testing:", mem, mem_size, 0); + + wp_free(mem, mem_size); + munmap(mem, mem_size); + + /* 4. Huge page testing */ + map = gethugepage(hpage_size); + if (map) { + wp_init(map, hpage_size); + wp_addr_range(map, hpage_size); + base_tests("Huge page testing:", map, hpage_size, 0); + wp_free(map, hpage_size); + free(map); + } else { + base_tests("Huge page testing:", NULL, 0, 1); + } + + /* 5. Hugetlb page testing */ + mem_size = 2*1024*1024; + mem = gethugetlb_mem(mem_size, &shmid); + if (mem) { + wp_init(mem, mem_size); + wp_addr_range(mem, mem_size); + + base_tests("Hugetlb shmem testing:", mem, mem_size, 0); + + wp_free(mem, mem_size); + shmctl(shmid, IPC_RMID, NULL); + } else { + base_tests("Hugetlb shmem testing:", NULL, 0, 1); + } + + /* 6. Hugetlb page testing */ + mem = gethugetlb_mem(mem_size, NULL); + if (mem) { + wp_init(mem, mem_size); + wp_addr_range(mem, mem_size); + + base_tests("Hugetlb mem testing:", mem, mem_size, 0); + + wp_free(mem, mem_size); + } else { + base_tests("Hugetlb mem testing:", NULL, 0, 1); + } + + /* 7. file memory testing */ + buf_size = page_size * 10; + + fd = open(__FILE__".tmp0", O_RDWR | O_CREAT, 0777); + if (fd < 0) + ksft_exit_fail_msg("Create and read/write to a memory mapped file: %s\n", + strerror(errno)); + + for (i = 0; i < buf_size; i++) + if (write(fd, "c", 1) < 0) + ksft_exit_fail_msg("Create and read/write to a memory mapped file\n"); + + ret = stat(__FILE__".tmp0", &sbuf); + if (ret < 0) + ksft_exit_fail_msg("error %d %d %s\n", ret, errno, strerror(errno)); + + fmem = mmap(NULL, sbuf.st_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0); + if (fmem == MAP_FAILED) + ksft_exit_fail_msg("error nomem %ld %s\n", errno, strerror(errno)); + + wp_init(fmem, sbuf.st_size); + wp_addr_range(fmem, sbuf.st_size); + + base_tests("File memory testing:", fmem, sbuf.st_size, 0); + + wp_free(fmem, sbuf.st_size); + munmap(fmem, sbuf.st_size); + close(fd); + + /* 8. file memory testing */ + buf_size = page_size * 10; + + fd = memfd_create(__FILE__".tmp00", MFD_NOEXEC_SEAL); + if (fd < 0) + ksft_exit_fail_msg("Create and read/write to a memory mapped file: %s\n", + strerror(errno)); + + if (ftruncate(fd, buf_size)) + ksft_exit_fail_msg("Error ftruncate\n"); + + for (i = 0; i < buf_size; i++) + if (write(fd, "c", 1) < 0) + ksft_exit_fail_msg("Create and read/write to a memory mapped file\n"); + + fmem = mmap(NULL, buf_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0); + if (fmem == MAP_FAILED) + ksft_exit_fail_msg("error nomem %ld %s\n", errno, strerror(errno)); + + wp_init(fmem, buf_size); + wp_addr_range(fmem, buf_size); + + base_tests("File anonymous memory testing:", fmem, buf_size, 0); + + wp_free(fmem, buf_size); + munmap(fmem, buf_size); + close(fd); + + /* 9. Huge page tests */ + hpage_unit_tests(); + + /* 10. Iterative test */ + test_simple(); + + /* 11. Mprotect test */ + mprotect_tests(); + + /* 12. Transact test */ + transact_test(page_size); + + /* + * Other PTE bit tests + */ + + /* 1. Sanity testing */ + sanity_tests(); + + /* 2. Unmapped address test */ + unmapped_region_tests(); + + /* 3. Userfaultfd tests */ + userfaultfd_tests(); + + close(pagemap_fd); + return ksft_exit_pass(); +} diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh old mode 100644 new mode 100755 index 3f26f6e15b2a..79028a7e855c --- a/tools/testing/selftests/mm/run_vmtests.sh +++ b/tools/testing/selftests/mm/run_vmtests.sh @@ -53,6 +53,8 @@ separated by spaces: memory protection key tests - soft_dirty test soft dirty page bit semantics +- pagemap + test pagemap_scan IOCTL - cow test copy-on-write semantics example: ./run_vmtests.sh -t "hmm mmap ksm" @@ -292,6 +294,8 @@ fi
CATEGORY="soft_dirty" run_test ./soft-dirty
+CATEGORY="pagemap" run_test ./pagemap_ioctl + # COW tests CATEGORY="cow" run_test ./cow
linux-kselftest-mirror@lists.linaro.org
-
kernel test robot -
Michał Mirosław -
Muhammad Usama Anjum -
Randy Dunlap