[v18,2/5] fs/proc/task_mmu: Implement IOCTL to get and optionally clear info about PTEs

Commit Message

Muhammad Usama Anjum June 13, 2023, 10:29 a.m. UTC

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 <[e-mail]>
---
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(-)

Comments

kernel test robot June 13, 2023, 4:11 p.m. UTC | #1

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/userfaultfd-UFFD_FEATURE_WP_ASYNC/20230613-183334
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.com
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/[e-mail]/config)
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/[e-mail]
        # 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 <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

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

Michał Mirosław June 13, 2023, 10:36 p.m. UTC | #2

On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
<[e-mail]> 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

Randy Dunlap June 14, 2023, 4:09 a.m. UTC | #3

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.

Muhammad Usama Anjum June 14, 2023, 11:36 a.m. UTC | #4

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.

Muhammad Usama Anjum June 14, 2023, 1:46 p.m. UTC | #5

On 6/14/23 3:36 AM, Michał Mirosław wrote:
> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
> <[e-mail]> 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=[e-mail]


[...]
>>> +       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

Michał Mirosław June 14, 2023, 3:14 p.m. UTC | #6

On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
<[e-mail]> wrote:
>
> On 6/14/23 3:36 AM, Michał Mirosław wrote:
> > On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
> > <[e-mail]> 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=[e-mail]

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

Muhammad Usama Anjum June 14, 2023, 5:09 p.m. UTC | #7

On 6/14/23 8:14 PM, Michał Mirosław wrote:
> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
> <[e-mail]> wrote:
>>
>> On 6/14/23 3:36 AM, Michał Mirosław wrote:
>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
>>> <[e-mail]> 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=[e-mail]
> 
> 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:
> 
> 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?
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.

> 
> 2. 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?

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

> 
> 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).
Yeah, correct.

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

Michał Mirosław June 14, 2023, 6 p.m. UTC | #8

(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
<[e-mail]> wrote:
> On 6/14/23 8:14 PM, Michał Mirosław wrote:
> > On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
> > <[e-mail]> wrote:
> >>
> >> On 6/14/23 3:36 AM, Michał Mirosław wrote:
> >>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
> >>> <[e-mail]> 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:
> >
> > 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?
> 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.

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

Muhammad Usama Anjum June 15, 2023, 1:58 p.m. UTC | #9

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
> <[e-mail]> wrote:
>> On 6/14/23 8:14 PM, Michał Mirosław wrote:
>>> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
>>> <[e-mail]> wrote:
>>>>
>>>> On 6/14/23 3:36 AM, Michał Mirosław wrote:
>>>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
>>>>> <[e-mail]> 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:
>>>
>>> 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?
>> 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.
> 
>>> 2. 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

Michał Mirosław June 15, 2023, 2:52 p.m. UTC | #10

On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
<[e-mail]> 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
> > <[e-mail]> wrote:
> >> On 6/14/23 8:14 PM, Michał Mirosław wrote:
> >>> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
> >>> <[e-mail]> wrote:
> >>>>
> >>>> On 6/14/23 3:36 AM, Michał Mirosław wrote:
> >>>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
> >>>>> <[e-mail]> 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

Michał Mirosław June 15, 2023, 2:58 p.m. UTC | #11

On Thu, 15 Jun 2023 at 16:52, Michał Mirosław <[e-mail]> wrote:
> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
> <[e-mail]> 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
> > > <[e-mail]> wrote:
> > >> On 6/14/23 8:14 PM, Michał Mirosław wrote:
> > >>> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
> > >>> <[e-mail]> wrote:
> > >>>>
> > >>>> On 6/14/23 3:36 AM, Michał Mirosław wrote:
> > >>>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
> > >>>>> <[e-mail]> 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

Muhammad Usama Anjum June 15, 2023, 3:11 p.m. UTC | #12

On 6/15/23 7:52 PM, Michał Mirosław wrote:
> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
> <[e-mail]> 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
>>> <[e-mail]> wrote:
>>>> On 6/14/23 8:14 PM, Michał Mirosław wrote:
>>>>> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
>>>>> <[e-mail]> wrote:
>>>>>>
>>>>>> On 6/14/23 3:36 AM, Michał Mirosław wrote:
>>>>>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
>>>>>>> <[e-mail]> 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

Muhammad Usama Anjum June 15, 2023, 3:16 p.m. UTC | #13

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 <[e-mail]> wrote:
>> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
>> <[e-mail]> 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
>>>> <[e-mail]> wrote:
>>>>> On 6/14/23 8:14 PM, Michał Mirosław wrote:
>>>>>> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
>>>>>> <[e-mail]> wrote:
>>>>>>>
>>>>>>> On 6/14/23 3:36 AM, Michał Mirosław wrote:
>>>>>>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
>>>>>>>> <[e-mail]> 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

Michał Mirosław June 15, 2023, 8 p.m. UTC | #14

On Thu, 15 Jun 2023 at 17:16, Muhammad Usama Anjum
<[e-mail]> 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 <[e-mail]> wrote:
> >> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
> >> <[e-mail]> 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
> >>>> <[e-mail]> wrote:
> >>>>> On 6/14/23 8:14 PM, Michał Mirosław wrote:
> >>>>>> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
> >>>>>> <[e-mail]> wrote:
> >>>>>>>
> >>>>>>> On 6/14/23 3:36 AM, Michał Mirosław wrote:
> >>>>>>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
> >>>>>>>> <[e-mail]> 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

Michał Mirosław June 15, 2023, 8:07 p.m. UTC | #15

On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum
<[e-mail]> wrote:
> On 6/15/23 7:52 PM, Michał Mirosław wrote:
> > On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
> > <[e-mail]> 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
> >>> <[e-mail]> wrote:
> >>>> On 6/14/23 8:14 PM, Michał Mirosław wrote:
> >>>>> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
> >>>>> <[e-mail]> wrote:
> >>>>>>
> >>>>>> On 6/14/23 3:36 AM, Michał Mirosław wrote:
> >>>>>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
> >>>>>>> <[e-mail]> 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

Muhammad Usama Anjum June 16, 2023, 6:37 a.m. UTC | #16

On 6/16/23 1:00 AM, Michał Mirosław wrote:
> On Thu, 15 Jun 2023 at 17:16, Muhammad Usama Anjum
> <[e-mail]> 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 <[e-mail]> wrote:
>>>> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
>>>> <[e-mail]> 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
>>>>>> <[e-mail]> wrote:
>>>>>>> On 6/14/23 8:14 PM, Michał Mirosław wrote:
>>>>>>>> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
>>>>>>>> <[e-mail]> wrote:
>>>>>>>>>
>>>>>>>>> On 6/14/23 3:36 AM, Michał Mirosław wrote:
>>>>>>>>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
>>>>>>>>>> <[e-mail]> 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

Muhammad Usama Anjum June 16, 2023, 6:57 a.m. UTC | #17

On 6/16/23 1:07 AM, Michał Mirosław wrote:
> On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum
> <[e-mail]> wrote:
>> On 6/15/23 7:52 PM, Michał Mirosław wrote:
>>> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
>>> <[e-mail]> 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
>>>>> <[e-mail]> wrote:
>>>>>> On 6/14/23 8:14 PM, Michał Mirosław wrote:
>>>>>>> On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
>>>>>>> <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On 6/14/23 3:36 AM, Michał Mirosław wrote:
>>>>>>>>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
>>>>>>>>> <[e-mail]> 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

Michał Mirosław June 19, 2023, 8:16 a.m. UTC | #18

On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum
<[e-mail]> wrote:
>
> On 6/16/23 1:07 AM, Michał Mirosław wrote:
> > On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum
> > <[e-mail]> wrote:
> >> On 6/15/23 7:52 PM, Michał Mirosław wrote:
> >>> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
> >>> <[e-mail]> 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

Muhammad Usama Anjum June 20, 2023, 11:15 a.m. UTC | #19

On 6/19/23 1:16 PM, Michał Mirosław wrote:
> On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum
> <[e-mail]> wrote:
>>
>> On 6/16/23 1:07 AM, Michał Mirosław wrote:
>>> On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum
>>> <[e-mail]> wrote:
>>>> On 6/15/23 7:52 PM, Michał Mirosław wrote:
>>>>> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
>>>>> <[e-mail]> 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

Michał Mirosław June 20, 2023, 10:05 p.m. UTC | #20

On Tue, 20 Jun 2023 at 13:16, Muhammad Usama Anjum
<[e-mail]> wrote:
> On 6/19/23 1:16 PM, Michał Mirosław wrote:
> > On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum
> > <[e-mail]> wrote:
> >>
> >> On 6/16/23 1:07 AM, Michał Mirosław wrote:
> >>> On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum
> >>> <[e-mail]> wrote:
> >>>> On 6/15/23 7:52 PM, Michał Mirosław wrote:
> >>>>> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
> >>>>> <[e-mail]> 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

Muhammad Usama Anjum June 21, 2023, 4:44 a.m. UTC | #21

On 6/21/23 3:05 AM, Michał Mirosław wrote:
> On Tue, 20 Jun 2023 at 13:16, Muhammad Usama Anjum
> <[e-mail]> wrote:
>> On 6/19/23 1:16 PM, Michał Mirosław wrote:
>>> On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum
>>> <[e-mail]> wrote:
>>>>
>>>> On 6/16/23 1:07 AM, Michał Mirosław wrote:
>>>>> On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum
>>>>> <[e-mail]> wrote:
>>>>>> On 6/15/23 7:52 PM, Michał Mirosław wrote:
>>>>>>> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
>>>>>>> <[e-mail]> 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/[e-mail]
) 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.

Michał Mirosław June 21, 2023, 1:24 p.m. UTC | #22

On Wed, 21 Jun 2023 at 06:44, Muhammad Usama Anjum
<[e-mail]> wrote:
> On 6/21/23 3:05 AM, Michał Mirosław wrote:
> > On Tue, 20 Jun 2023 at 13:16, Muhammad Usama Anjum
> > <[e-mail]> wrote:
> >> On 6/19/23 1:16 PM, Michał Mirosław wrote:
> >>> On Fri, 16 Jun 2023 at 08:57, Muhammad Usama Anjum
> >>> <[e-mail]> wrote:
> >>>>
> >>>> On 6/16/23 1:07 AM, Michał Mirosław wrote:
> >>>>> On Thu, 15 Jun 2023 at 17:11, Muhammad Usama Anjum
> >>>>> <[e-mail]> wrote:
> >>>>>> On 6/15/23 7:52 PM, Michał Mirosław wrote:
> >>>>>>> On Thu, 15 Jun 2023 at 15:58, Muhammad Usama Anjum
> >>>>>>> <[e-mail]> 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/[e-mail]
> ) 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

Patch

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;