| Message ID | 20230606060822.1065182-3-usama.anjum@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Implement IOCTL to get and optionally clear info about PTEs | expand |
Hi Muhammad,
kernel test robot noticed the following build errors:
[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on next-20230606]
[cannot apply to linus/master v6.4-rc5]
[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/20230606-141114
base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/r/20230606060822.1065182-3-usama.anjum%40collabora.com
patch subject: [PATCH v17 2/5] fs/proc/task_mmu: Implement IOCTL to get and optionally clear info about PTEs
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20230607/202306070414.XDn2ITuw-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 12.3.0
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/20230606060822.1065182-3-usama.anjum@collabora.com
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306070414.XDn2ITuw-lkp@intel.com/
All errors (new ones prefixed by >>):
fs/proc/task_mmu.c: In function 'pagemap_scan_pmd_entry':
>> fs/proc/task_mmu.c:1960:31: error: 'HPAGE_SIZE' undeclared (first use in this function); did you mean 'PAGE_SIZE'?
1960 | n_pages < HPAGE_SIZE/PAGE_SIZE) {
| ^~~~~~~~~~
| PAGE_SIZE
fs/proc/task_mmu.c:1960:31: note: each undeclared identifier is reported only once for each function it appears in
vim +1960 fs/proc/task_mmu.c
1931
1932 static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
1933 unsigned long end, struct mm_walk *walk)
1934 {
1935 struct pagemap_scan_private *p = walk->private;
1936 struct vm_area_struct *vma = walk->vma;
1937 unsigned long addr = end;
1938 pte_t *pte, *orig_pte;
1939 spinlock_t *ptl;
1940 bool is_written;
1941 int ret = 0;
1942
1943 arch_enter_lazy_mmu_mode();
1944
1945 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
1946 ptl = pmd_trans_huge_lock(pmd, vma);
1947 if (ptl) {
1948 unsigned long n_pages = (end - start)/PAGE_SIZE;
1949
1950 if (p->max_pages && n_pages > p->max_pages - p->found_pages)
1951 n_pages = p->max_pages - p->found_pages;
1952
1953 is_written = !is_pmd_uffd_wp(*pmd);
1954
1955 /*
1956 * Break huge page into small pages if the WP operation need to
1957 * be performed is on a portion of the huge page.
1958 */
1959 if (is_written && IS_PM_SCAN_WP(p->flags) &&
> 1960 n_pages < HPAGE_SIZE/PAGE_SIZE) {
1961 spin_unlock(ptl);
1962
1963 split_huge_pmd(vma, pmd, start);
1964 goto process_smaller_pages;
1965 }
1966
1967 if (IS_PM_SCAN_GET(p->flags))
1968 ret = pagemap_scan_output(is_written, vma->vm_file,
1969 pmd_present(*pmd),
1970 is_swap_pmd(*pmd),
1971 p, start, n_pages);
1972
1973 if (ret >= 0 && is_written && IS_PM_SCAN_WP(p->flags))
1974 make_uffd_wp_pmd(vma, addr, pmd);
1975
1976 if (IS_PM_SCAN_WP(p->flags))
1977 flush_tlb_range(vma, start, end);
1978
1979 spin_unlock(ptl);
1980
1981 arch_leave_lazy_mmu_mode();
1982 return ret;
1983 }
1984
1985 process_smaller_pages:
1986 if (pmd_trans_unstable(pmd)) {
1987 arch_leave_lazy_mmu_mode();
1988 walk->action = ACTION_AGAIN;
1989 return 0;
1990 }
1991 #endif
1992
1993 orig_pte = pte = pte_offset_map_lock(vma->vm_mm, pmd, start, &ptl);
1994 for (addr = start; addr < end && !ret; pte++, addr += PAGE_SIZE) {
1995 is_written = !is_pte_uffd_wp(*pte);
1996
1997 if (IS_PM_SCAN_GET(p->flags))
1998 ret = pagemap_scan_output(is_written, vma->vm_file,
1999 pte_present(*pte),
2000 is_swap_pte(*pte),
2001 p, addr, 1);
2002
2003 if (ret >= 0 && is_written && IS_PM_SCAN_WP(p->flags))
2004 make_uffd_wp_pte(vma, addr, pte);
2005 }
2006
2007 if (IS_PM_SCAN_WP(p->flags))
2008 flush_tlb_range(vma, start, addr);
2009
2010 pte_unmap_unlock(orig_pte, ptl);
2011 arch_leave_lazy_mmu_mode();
2012
2013 cond_resched();
2014 return ret;
2015 }
2016
On Tue, 6 Jun 2023 at 08:08, Muhammad Usama Anjum <usama.anjum@collabora.com> wrote: > This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear > the info about page table entries. The following operations are supported > in this ioctl: > - Get the information if the pages have been written-to (PAGE_IS_WRITTEN), > file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped > (PAGE_IS_SWAPPED). > - Find pages which have been written-to and/or write protect the pages > (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP) > > This IOCTL can be extended to get information about more PTE bits. [...] > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c [...] > +#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)); > +} [...] > +#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)); Nit: please remove the outer parentheses (it is already done for similar finctuons above). > +} > +static inline bool pagemap_scan_check_page_written(struct pagemap_scan_private *p) > +{ > + return (p->required_mask | p->anyof_mask | p->excluded_mask) & > + PAGE_IS_WRITTEN; > +} This could be precalculated and put as a flag into pagemap_scan_private - it is kernel-private structure and there are a few spare bits in `flags` if you'd prefer not to add an explicit boolean. [...] > +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 = &p->cur; > + > + 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->bitmap == bitmap && > + cur->start + cur->len * PAGE_SIZE == addr) { > + cur->len += n_pages; > + p->found_pages += n_pages; > + } else { > + /* > + * All data is copied to cur first. When more data is found, we > + * push cur to vec and copy new data to cur. The vec_index > + * represents the current index of vec array. We add 1 to the > + * vec_index while performing checks to account for data in cur. > + */ > + if (cur->len && (p->vec_index + 1) >= p->vec_len) > + return -ENOSPC; > + > + if (cur->len) { > + memcpy(&p->vec[p->vec_index], cur, sizeof(*p->vec)); > + p->vec_index++; > + } > + > + cur->start = addr; > + cur->len = n_pages; > + cur->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; > + unsigned long addr = end; > + pte_t *pte, *orig_pte; > + spinlock_t *ptl; > + bool is_written; > + 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; Since p->found_pages is only ever increased in `pagemap_scan_output()` and that function is only called for GET or GET+WP operations, maybe the logic could be folded to pagemap_scan_output() to avoid duplication? In this function the calculation is used only when WP op is done to split the HP if n_pages limit would be hit, but if using plain WP (without GET) it doesn't make sense to use the limit. (pagemap_scan_output() is trivial enough so I think it could be pulled inside the spinlocked region.) > + > + 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_PM_SCAN_WP(p->flags)) Why `is_written` is not checked? If is_written is false, then the WP op should be a no-op and so won't need TLB flushing, will it? [Same for the PTE case below.] > + flush_tlb_range(vma, start, end); > + [...] > + if (IS_PM_SCAN_WP(p->flags)) > + 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 pte; > + > + 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); > + } > + > + pte = huge_ptep_get(ptep); > + is_written = !is_huge_pte_uffd_wp(pte); > + > + /* > + * Partial hugetlb page clear isn't supported > + */ > + if (is_written && IS_PM_SCAN_WP(p->flags) && > + n_pages < HPAGE_SIZE/PAGE_SIZE) { > + ret = -EPERM; Shouldn't this be ENOSPC, conveying that the operation would overflow the n_pages limit? > + goto unlock_and_return; > + } > + > + if (IS_PM_SCAN_GET(p->flags)) { > + ret = pagemap_scan_output(is_written, vma->vm_file, > + pte_present(pte), is_swap_pte(pte), > + p, start, n_pages); > + if (ret < 0) > + goto unlock_and_return; > + } > + > + if (is_written && IS_PM_SCAN_WP(p->flags)) { Oh, this case does check `is_written` before flushing TLB, contrary to what the cases above do. > + make_uffd_wp_huge_pte(vma, start, ptep, pte); > + 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; Nit: If the page flags don't match (wouldn't be output), the limit would not be hit and the calculation is unnecessary. But if it was done in pagemap_scan_output() instead after all the flags checks... > + ret = pagemap_scan_output(false, vma->vm_file, false, false, p, addr, > + n_pages); > + > + return ret; > +} [...] > +static long do_pagemap_scan(struct mm_struct *mm, > + struct pm_scan_arg __user *uarg) > +{ > + 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.flags = arg.flags; > + p.required_mask = arg.required_mask; > + p.anyof_mask = arg.anyof_mask; > + p.excluded_mask = arg.excluded_mask; > + p.return_mask = arg.return_mask; > + p.cur.start = p.cur.len = p.cur.bitmap = 0; > + p.vec = NULL; > + p.vec_len = PAGEMAP_WALK_SIZE >> PAGE_SHIFT; If p.vec_len would not count the entry held in `cur` (IOW: vec_len = WALK_SIZE - 1), then pagemap_scan_output() wouldn't need the big comment about adding or subtracting 1 when checking for overflow. The output vector needs to have space for at least one entrry to make GET useful. Maybe `cur` could be renamed or annotated to express that it always holds the last entry? > + > + /* > + * 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 between different walks and > + * store the p.cur at the end of the walk to the user buffer. > + */ > + if (IS_PM_SCAN_GET(p.flags)) { > + p.vec = kmalloc_array(p.vec_len, sizeof(*p.vec), GFP_KERNEL); > + if (!p.vec) > + 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_index = 0; > + > + empty_slots = arg.vec_len - vec_index; Can `empty_slots` be zero here? I don't see anything prohibiting this case. > + p.vec_len = min(p.vec_len, empty_slots); ( If not counting `cur`, it would be min(p.vec_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 != -ENOSPC && ret != PM_SCAN_FOUND_MAX_PAGES) > + goto free_data; > + > + walk_start = walk_end; > + if (IS_PM_SCAN_GET(p.flags) && p.vec_index) { > + if (copy_to_user(&vec[vec_index], p.vec, > + p.vec_index * sizeof(*p.vec))) { > + /* > + * Return error even though the OP succeeded > + */ > + ret = -EFAULT; > + goto free_data; > + } > + vec_index += p.vec_index; > + } > + } > + > + if (IS_PM_SCAN_GET(p.flags) && p.cur.len) { Nit: p.cur.len can be non-zero only if we do a GET (or GET+WP) operation. > + if (copy_to_user(&vec[vec_index], &p.cur, sizeof(*p.vec))) { Nit: sizeof(*p.cur); (even though this is the same type) > + 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); > + return ret; > +} > + > +static long do_pagemap_cmd(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + struct pm_scan_arg __user *uarg = (struct pm_scan_arg __user *)arg; The cast should be in do_pagemap_scan() as if there comes another `cmd`, then it might use a different argument type. > + struct mm_struct *mm = file->private_data; > + > + switch (cmd) { > + case PAGEMAP_SCAN: > + return do_pagemap_scan(mm, uarg); > + > + default: > + return -EINVAL; > + } > +} Best Regards Michał Mirosław
Hi Michał, Thank you for taking time to review! On 6/7/23 7:52 PM, Michał Mirosław wrote: > On Tue, 6 Jun 2023 at 08:08, Muhammad Usama Anjum > <usama.anjum@collabora.com> wrote: >> This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear >> the info about page table entries. The following operations are supported >> in this ioctl: >> - Get the information if the pages have been written-to (PAGE_IS_WRITTEN), >> file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped >> (PAGE_IS_SWAPPED). >> - Find pages which have been written-to and/or write protect the pages >> (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP) >> >> This IOCTL can be extended to get information about more PTE bits. > [...] >> --- a/fs/proc/task_mmu.c >> +++ b/fs/proc/task_mmu.c > [...] >> +#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)); >> +} > [...] >> +#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)); > > Nit: please remove the outer parentheses (it is already done for > similar finctuons above). Will remove. > >> +} > >> +static inline bool pagemap_scan_check_page_written(struct pagemap_scan_private *p) >> +{ >> + return (p->required_mask | p->anyof_mask | p->excluded_mask) & >> + PAGE_IS_WRITTEN; >> +} > > This could be precalculated and put as a flag into > pagemap_scan_private - it is kernel-private structure and there are a > few spare bits in `flags` if you'd prefer not to add an explicit > boolean. This inline function is only being used at one spot. I can remove the function altogether. I don't like putting it in flags. It'll bring some complexity. > > [...] >> +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 = &p->cur; >> + >> + 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->bitmap == bitmap && >> + cur->start + cur->len * PAGE_SIZE == addr) { >> + cur->len += n_pages; >> + p->found_pages += n_pages; >> + } else { >> + /* >> + * All data is copied to cur first. When more data is found, we >> + * push cur to vec and copy new data to cur. The vec_index >> + * represents the current index of vec array. We add 1 to the >> + * vec_index while performing checks to account for data in cur. >> + */ >> + if (cur->len && (p->vec_index + 1) >= p->vec_len) >> + return -ENOSPC; >> + >> + if (cur->len) { >> + memcpy(&p->vec[p->vec_index], cur, sizeof(*p->vec)); >> + p->vec_index++; >> + } >> + >> + cur->start = addr; >> + cur->len = n_pages; >> + cur->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; >> + unsigned long addr = end; >> + pte_t *pte, *orig_pte; >> + spinlock_t *ptl; >> + bool is_written; >> + 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; > > Since p->found_pages is only ever increased in `pagemap_scan_output()` > and that function is only called for GET or GET+WP operations, maybe > the logic could be folded to pagemap_scan_output() to avoid > duplication? > In this function the calculation is used only when WP op is done to > split the HP if n_pages limit would be hit, but if using plain WP > (without GET) it doesn't make sense to use the limit. The n_pages is needed to decide if THP need to be broken down and it is used in pagemap_scan_output(). I've brought this condition out of pagemap_scan_output() to cater this former condition. If I move it to pagemap_scan_output(), I'll have to write same condition to find out if I need to breakt he THP. This seems like repetition, but we have same use case for tlbhuge page. > (pagemap_scan_output() is trivial enough so I think it could be pulled > inside the spinlocked region.) It is already in spinlocked region. Spin lock is being released after tlb flush. > >> + >> + 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_PM_SCAN_WP(p->flags)) > > Why `is_written` is not checked? If is_written is false, then the WP > op should be a no-op and so won't need TLB flushing, will it? [Same > for the PTE case below.] It can be done for THP. But for ptes we cannot trust is_written as is_written only represent last pte state. > >> + flush_tlb_range(vma, start, end); >> + > [...] >> + if (IS_PM_SCAN_WP(p->flags)) >> + 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 pte; >> + >> + 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); >> + } >> + >> + pte = huge_ptep_get(ptep); >> + is_written = !is_huge_pte_uffd_wp(pte); >> + >> + /* >> + * Partial hugetlb page clear isn't supported >> + */ >> + if (is_written && IS_PM_SCAN_WP(p->flags) && >> + n_pages < HPAGE_SIZE/PAGE_SIZE) { >> + ret = -EPERM; > > Shouldn't this be ENOSPC, conveying that the operation would overflow > the n_pages limit? We are testing here is user has asked us to engage WP on a part of the hugetlb or we can only perform WP on a part of the engage as user buffer is full. We cannot judge this has happened because of the former or later condition. So I'm assuming that user's parameters aren't solid enough and returning -EPERM. It seemed more suitable to me. But I can return -ENOSPC as well, if you say? > >> + goto unlock_and_return; >> + } >> + >> + if (IS_PM_SCAN_GET(p->flags)) { >> + ret = pagemap_scan_output(is_written, vma->vm_file, >> + pte_present(pte), is_swap_pte(pte), >> + p, start, n_pages); >> + if (ret < 0) >> + goto unlock_and_return; >> + } >> + >> + if (is_written && IS_PM_SCAN_WP(p->flags)) { > > Oh, this case does check `is_written` before flushing TLB, contrary to > what the cases above do. > >> + make_uffd_wp_huge_pte(vma, start, ptep, pte); >> + 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; > > Nit: If the page flags don't match (wouldn't be output), the limit > would not be hit and the calculation is unnecessary. But if it was > done in pagemap_scan_output() instead after all the flags checks... Correct for this use case. But moving to pagemap_scan_output() would make me do duplicate calculation for other 2 cases as explained above. > >> + ret = pagemap_scan_output(false, vma->vm_file, false, false, p, addr, >> + n_pages); >> + >> + return ret; >> +} > [...] >> +static long do_pagemap_scan(struct mm_struct *mm, >> + struct pm_scan_arg __user *uarg) >> +{ >> + 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.flags = arg.flags; >> + p.required_mask = arg.required_mask; >> + p.anyof_mask = arg.anyof_mask; >> + p.excluded_mask = arg.excluded_mask; >> + p.return_mask = arg.return_mask; >> + p.cur.start = p.cur.len = p.cur.bitmap = 0; >> + p.vec = NULL; >> + p.vec_len = PAGEMAP_WALK_SIZE >> PAGE_SHIFT; > > If p.vec_len would not count the entry held in `cur` (IOW: vec_len = > WALK_SIZE - 1), then pagemap_scan_output() wouldn't need the big > comment about adding or subtracting 1 when checking for overflow. The > output vector needs to have space for at least one entrry to make GET > useful. Maybe `cur` could be renamed or annotated to express that it > always holds the last entry? Ohhh.. This can be done by doing subtracting 1 from empty_slots. But I've explored the idea. I don't see any benefit. If we do this, then I'll have to put a comment why subtracting 1 is needed. Seems like same problem: --- a/fs/proc/task_mmu.c +++ b/fs/proc/task_mmu.c @@ -1909,7 +1909,7 @@ static int pagemap_scan_output(bool wt, bool file, bool pres, bool swap, * represents the current index of vec array. We add 1 to the * vec_index while performing checks to account for data in cur. */ - if (cur->len && (p->vec_index + 1) >= p->vec_len) + if (cur->len && p->vec_index >= p->vec_len) return -ENOSPC; if (cur->len) { @@ -2202,7 +2202,7 @@ static long do_pagemap_scan(struct mm_struct *mm, if (IS_PM_SCAN_GET(p.flags)) { p.vec_index = 0; - empty_slots = arg.vec_len - vec_index; + empty_slots = arg.vec_len - 1 - vec_index; p.vec_len = min(p.vec_len, empty_slots); } Lets leave it as it is. I can change `cur` to `last` or any other name. Please suggest. > >> + >> + /* >> + * 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 between different walks and >> + * store the p.cur at the end of the walk to the user buffer. >> + */ >> + if (IS_PM_SCAN_GET(p.flags)) { >> + p.vec = kmalloc_array(p.vec_len, sizeof(*p.vec), GFP_KERNEL); >> + if (!p.vec) >> + 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_index = 0; >> + >> + empty_slots = arg.vec_len - vec_index; > > Can `empty_slots` be zero here? I don't see anything prohibiting this case. I'll add a check here and abort the loop here instead of continuing the operation. > >> + p.vec_len = min(p.vec_len, empty_slots); > > ( If not counting `cur`, it would be min(p.vec_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 != -ENOSPC && ret != PM_SCAN_FOUND_MAX_PAGES) >> + goto free_data; >> + >> + walk_start = walk_end; >> + if (IS_PM_SCAN_GET(p.flags) && p.vec_index) { >> + if (copy_to_user(&vec[vec_index], p.vec, >> + p.vec_index * sizeof(*p.vec))) { >> + /* >> + * Return error even though the OP succeeded >> + */ >> + ret = -EFAULT; >> + goto free_data; >> + } >> + vec_index += p.vec_index; >> + } >> + } >> + >> + if (IS_PM_SCAN_GET(p.flags) && p.cur.len) { > > Nit: p.cur.len can be non-zero only if we do a GET (or GET+WP) operation. I'll remove the first half of condition. > >> + if (copy_to_user(&vec[vec_index], &p.cur, sizeof(*p.vec))) { > > Nit: sizeof(*p.cur); (even though this is the same type) Good point. > >> + 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); >> + return ret; >> +} >> + >> +static long do_pagemap_cmd(struct file *file, unsigned int cmd, >> + unsigned long arg) >> +{ >> + struct pm_scan_arg __user *uarg = (struct pm_scan_arg __user *)arg; > > The cast should be in do_pagemap_scan() as if there comes another > `cmd`, then it might use a different argument type. I'll update. > >> + struct mm_struct *mm = file->private_data; >> + >> + switch (cmd) { >> + case PAGEMAP_SCAN: >> + return do_pagemap_scan(mm, uarg); >> + >> + default: >> + return -EINVAL; >> + } >> +} > > Best Regards > Michał Mirosław
On Wed, 7 Jun 2023 at 18:13, Muhammad Usama Anjum <usama.anjum@collabora.com> wrote: > > Hi Michał, > > Thank you for taking time to review! > > On 6/7/23 7:52 PM, Michał Mirosław wrote: > > On Tue, 6 Jun 2023 at 08:08, Muhammad Usama Anjum > > <usama.anjum@collabora.com> wrote: > >> This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear > >> the info about page table entries. The following operations are supported > >> in this ioctl: > >> - Get the information if the pages have been written-to (PAGE_IS_WRITTEN), > >> file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped > >> (PAGE_IS_SWAPPED). > >> - Find pages which have been written-to and/or write protect the pages > >> (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP) > >> > >> This IOCTL can be extended to get information about more PTE bits. > > [...] > >> --- a/fs/proc/task_mmu.c > >> +++ b/fs/proc/task_mmu.c [...] > >> +static inline bool pagemap_scan_check_page_written(struct pagemap_scan_private *p) > >> +{ > >> + return (p->required_mask | p->anyof_mask | p->excluded_mask) & > >> + PAGE_IS_WRITTEN; > >> +} > > > > This could be precalculated and put as a flag into > > pagemap_scan_private - it is kernel-private structure and there are a > > few spare bits in `flags` if you'd prefer not to add an explicit > > boolean. > This inline function is only being used at one spot. I can remove the > function altogether. I don't like putting it in flags. It'll bring some > complexity. The difference at the call site will be function call vs field access. Do you mean that moving the function to where the struct is initialized would add complexity? Why is that? > > [...] > >> +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 = &p->cur; > >> + > >> + 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->bitmap == bitmap && > >> + cur->start + cur->len * PAGE_SIZE == addr) { > >> + cur->len += n_pages; > >> + p->found_pages += n_pages; > >> + } else { > >> + /* > >> + * All data is copied to cur first. When more data is found, we > >> + * push cur to vec and copy new data to cur. The vec_index > >> + * represents the current index of vec array. We add 1 to the > >> + * vec_index while performing checks to account for data in cur. > >> + */ > >> + if (cur->len && (p->vec_index + 1) >= p->vec_len) > >> + return -ENOSPC; > >> + > >> + if (cur->len) { > >> + memcpy(&p->vec[p->vec_index], cur, sizeof(*p->vec)); > >> + p->vec_index++; > >> + } > >> + > >> + cur->start = addr; > >> + cur->len = n_pages; > >> + cur->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; > >> + unsigned long addr = end; > >> + pte_t *pte, *orig_pte; > >> + spinlock_t *ptl; > >> + bool is_written; > >> + 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; > > > > Since p->found_pages is only ever increased in `pagemap_scan_output()` > > and that function is only called for GET or GET+WP operations, maybe > > the logic could be folded to pagemap_scan_output() to avoid > > duplication? > > In this function the calculation is used only when WP op is done to > > split the HP if n_pages limit would be hit, but if using plain WP > > (without GET) it doesn't make sense to use the limit. > The n_pages is needed to decide if THP need to be broken down and it is > used in pagemap_scan_output(). I've brought this condition out of > pagemap_scan_output() to cater this former condition. If I move it to > pagemap_scan_output(), I'll have to write same condition to find out if I > need to breakt he THP. This seems like repetition, but we have same use > case for tlbhuge page. My point is that you need to split the THP only if doing a GET+WP operation. If you only do GET, then the worst case would be for the process to report a spurious WRITTEN bit if an earlier-visited part of THP was modified and the scan restarted in the middle of a THP. >> > (pagemap_scan_output() is trivial enough so I think it could be pulled > > inside the spinlocked region.) > It is already in spinlocked region. Spin lock is being released after tlb > flush. Ah, I was thinking about calling pagemap_scan_output() before checking split_huge_pmd() case - and at that use the pagemap_scan_output()'s return value to do the check. > >> + 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_PM_SCAN_WP(p->flags)) > > > > Why `is_written` is not checked? If is_written is false, then the WP > > op should be a no-op and so won't need TLB flushing, will it? [Same > > for the PTE case below.] > It can be done for THP. But for ptes we cannot trust is_written as > is_written only represent last pte state. Ok, so the PTE case could use a flag recording whether any PTE had WP applied instead of `is_written`. > >> + flush_tlb_range(vma, start, end); > >> + > > [...] > >> + if (IS_PM_SCAN_WP(p->flags)) > >> + 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 pte; > >> + > >> + 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); > >> + } > >> + > >> + pte = huge_ptep_get(ptep); > >> + is_written = !is_huge_pte_uffd_wp(pte); > >> + > >> + /* > >> + * Partial hugetlb page clear isn't supported > >> + */ > >> + if (is_written && IS_PM_SCAN_WP(p->flags) && > >> + n_pages < HPAGE_SIZE/PAGE_SIZE) { > >> + ret = -EPERM; > > > > Shouldn't this be ENOSPC, conveying that the operation would overflow > > the n_pages limit? > We are testing here is user has asked us to engage WP on a part of the > hugetlb or we can only perform WP on a part of the engage as user buffer is > full. We cannot judge this has happened because of the former or later > condition. So I'm assuming that user's parameters aren't solid enough and > returning -EPERM. It seemed more suitable to me. But I can return -ENOSPC > as well, if you say? Those two cases can be differentiated when checked before truncating n_pages. If a user requests partial WP for a hugetlb page wouldn't EINVAL (or other error - as this can't ever work) be more appropriate (this check could happen only at the start of scan)? If the request is due to max_pages limit (with found_pages > 0), then I'd return ENOSPC and expect the user to restart the scan with a new buffer. Our discussion here makes me wonder: what is the expected return value for the ioctl WP (without GET) operation? If it would return e.g. the number of 4K-pages successfully scanned, then the caller would be able to detect the partial tail hugepage case and act accordingly. > >> +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; > > > > Nit: If the page flags don't match (wouldn't be output), the limit > > would not be hit and the calculation is unnecessary. But if it was > > done in pagemap_scan_output() instead after all the flags checks... > Correct for this use case. But moving to pagemap_scan_output() would make > me do duplicate calculation for other 2 cases as explained above. (responded below the cases above) > >> + ret = pagemap_scan_output(false, vma->vm_file, false, false, p, addr, > >> + n_pages); > >> + > >> + return ret; > >> +} > > [...] > >> +static long do_pagemap_scan(struct mm_struct *mm, > >> + struct pm_scan_arg __user *uarg) > >> +{ > >> + 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.flags = arg.flags; > >> + p.required_mask = arg.required_mask; > >> + p.anyof_mask = arg.anyof_mask; > >> + p.excluded_mask = arg.excluded_mask; > >> + p.return_mask = arg.return_mask; > >> + p.cur.start = p.cur.len = p.cur.bitmap = 0; > >> + p.vec = NULL; > >> + p.vec_len = PAGEMAP_WALK_SIZE >> PAGE_SHIFT; > > > > If p.vec_len would not count the entry held in `cur` (IOW: vec_len = > > WALK_SIZE - 1), then pagemap_scan_output() wouldn't need the big > > comment about adding or subtracting 1 when checking for overflow. The > > output vector needs to have space for at least one entrry to make GET > > useful. Maybe `cur` could be renamed or annotated to express that it > > always holds the last entry? > Ohhh.. This can be done by doing subtracting 1 from empty_slots. But I've > explored the idea. I don't see any benefit. If we do this, then I'll have > to put a comment why subtracting 1 is needed. Seems like same problem: > > --- a/fs/proc/task_mmu.c > +++ b/fs/proc/task_mmu.c > @@ -1909,7 +1909,7 @@ static int pagemap_scan_output(bool wt, bool file, > bool pres, bool swap, > * represents the current index of vec array. We add 1 to the > * vec_index while performing checks to account for data in > cur. > */ > - if (cur->len && (p->vec_index + 1) >= p->vec_len) > + if (cur->len && p->vec_index >= p->vec_len) > return -ENOSPC; > > if (cur->len) { > @@ -2202,7 +2202,7 @@ static long do_pagemap_scan(struct mm_struct *mm, > if (IS_PM_SCAN_GET(p.flags)) { > p.vec_index = 0; > > - empty_slots = arg.vec_len - vec_index; > + empty_slots = arg.vec_len - 1 - vec_index; > p.vec_len = min(p.vec_len, empty_slots); > } > > Lets leave it as it is. I can change `cur` to `last` or any other name. > Please suggest. The difference is that you have the subtraction only once per the outer page_walk loop iteration, but in the current version the addition has to happen every pagemap_scan_output() call after a hole. From the readability perspective, "if (next_index >= vec_len)" is short and self-documenting. Also I'd use "p.vec_len = min(p.vec_len, empty_slots - 1)" as it also conveys the intent better (in that `vec` is holding all but the last entry, but arg.vec_len holds the final output buffer length). If we're picking colors, then maybe make `arg.vec_len` have a different name than `p.vec_len` (same for `vec_index`) so that there is less confusion possible as to what they refer to. Maybe keep `arg.vec_len`, but have `p.vec_buf`, `p.vec_buf_len`, and `p.next_buf_index`? (Note: you'd also need to decrement p.vec_len where it is first assigned.) [...] Best Regards Michał Mirosław
On 6/7/23 9:52 PM, Michał Mirosław wrote: > On Wed, 7 Jun 2023 at 18:13, Muhammad Usama Anjum > <usama.anjum@collabora.com> wrote: >> >> Hi Michał, >> >> Thank you for taking time to review! >> >> On 6/7/23 7:52 PM, Michał Mirosław wrote: >>> On Tue, 6 Jun 2023 at 08:08, Muhammad Usama Anjum >>> <usama.anjum@collabora.com> wrote: >>>> This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear >>>> the info about page table entries. The following operations are supported >>>> in this ioctl: >>>> - Get the information if the pages have been written-to (PAGE_IS_WRITTEN), >>>> file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped >>>> (PAGE_IS_SWAPPED). >>>> - Find pages which have been written-to and/or write protect the pages >>>> (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP) >>>> >>>> This IOCTL can be extended to get information about more PTE bits. >>> [...] >>>> --- a/fs/proc/task_mmu.c >>>> +++ b/fs/proc/task_mmu.c > [...] >>>> +static inline bool pagemap_scan_check_page_written(struct pagemap_scan_private *p) >>>> +{ >>>> + return (p->required_mask | p->anyof_mask | p->excluded_mask) & >>>> + PAGE_IS_WRITTEN; >>>> +} >>> >>> This could be precalculated and put as a flag into >>> pagemap_scan_private - it is kernel-private structure and there are a >>> few spare bits in `flags` if you'd prefer not to add an explicit >>> boolean. >> This inline function is only being used at one spot. I can remove the >> function altogether. I don't like putting it in flags. It'll bring some >> complexity. > > The difference at the call site will be function call vs field access. > Do you mean that moving the function to where the struct is > initialized would add complexity? Why is that? In my view, adding this calculation to `flag` would make the `flag double meaning. 1) user flags 2) wp flag is turn on or off Okay, I've added the flag and removed this function call from frpagemap_scan_test_walk(). > >>> [...] >>>> +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 = &p->cur; >>>> + >>>> + 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->bitmap == bitmap && >>>> + cur->start + cur->len * PAGE_SIZE == addr) { >>>> + cur->len += n_pages; >>>> + p->found_pages += n_pages; >>>> + } else { >>>> + /* >>>> + * All data is copied to cur first. When more data is found, we >>>> + * push cur to vec and copy new data to cur. The vec_index >>>> + * represents the current index of vec array. We add 1 to the >>>> + * vec_index while performing checks to account for data in cur. >>>> + */ >>>> + if (cur->len && (p->vec_index + 1) >= p->vec_len) >>>> + return -ENOSPC; >>>> + >>>> + if (cur->len) { >>>> + memcpy(&p->vec[p->vec_index], cur, sizeof(*p->vec)); >>>> + p->vec_index++; >>>> + } >>>> + >>>> + cur->start = addr; >>>> + cur->len = n_pages; >>>> + cur->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; >>>> + unsigned long addr = end; >>>> + pte_t *pte, *orig_pte; >>>> + spinlock_t *ptl; >>>> + bool is_written; >>>> + 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; >>> >>> Since p->found_pages is only ever increased in `pagemap_scan_output()` >>> and that function is only called for GET or GET+WP operations, maybe >>> the logic could be folded to pagemap_scan_output() to avoid >>> duplication? >>> In this function the calculation is used only when WP op is done to >>> split the HP if n_pages limit would be hit, but if using plain WP >>> (without GET) it doesn't make sense to use the limit. >> The n_pages is needed to decide if THP need to be broken down and it is >> used in pagemap_scan_output(). I've brought this condition out of >> pagemap_scan_output() to cater this former condition. If I move it to >> pagemap_scan_output(), I'll have to write same condition to find out if I >> need to breakt he THP. This seems like repetition, but we have same use >> case for tlbhuge page. > > My point is that you need to split the THP only if doing a GET+WP > operation. If you only do GET, then the worst case would be for the > process to report a spurious WRITTEN bit if an earlier-visited part of > THP was modified and the scan restarted in the middle of a THP. We only need to split if doing GET+WP or WP only. In case of WP op, I need to check if we have less than HPAGE_SIZE/PAGE_SIZE pages and only then split. So moving this if condition would not be beneficial. > >>>> (pagemap_scan_output() is trivial enough so I think it could be pulled >>> inside the spinlocked region.) >> It is already in spinlocked region. Spin lock is being released after tlb >> flush. > > Ah, I was thinking about calling pagemap_scan_output() before checking > split_huge_pmd() case - and at that use the pagemap_scan_output()'s > return value to do the check. > >>>> + 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_PM_SCAN_WP(p->flags)) >>> >>> Why `is_written` is not checked? If is_written is false, then the WP >>> op should be a no-op and so won't need TLB flushing, will it? [Same >>> for the PTE case below.] >> It can be done for THP. But for ptes we cannot trust is_written as >> is_written only represent last pte state. > > Ok, so the PTE case could use a flag recording whether any PTE had WP > applied instead of `is_written`. Definately, but I'll have to add a extra variable. I'll add it as you have asked. > >>>> + flush_tlb_range(vma, start, end); >>>> + >>> [...] >>>> + if (IS_PM_SCAN_WP(p->flags)) >>>> + 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 pte; >>>> + >>>> + 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); >>>> + } >>>> + >>>> + pte = huge_ptep_get(ptep); >>>> + is_written = !is_huge_pte_uffd_wp(pte); >>>> + >>>> + /* >>>> + * Partial hugetlb page clear isn't supported >>>> + */ >>>> + if (is_written && IS_PM_SCAN_WP(p->flags) && >>>> + n_pages < HPAGE_SIZE/PAGE_SIZE) { >>>> + ret = -EPERM; >>> >>> Shouldn't this be ENOSPC, conveying that the operation would overflow >>> the n_pages limit? >> We are testing here is user has asked us to engage WP on a part of the >> hugetlb or we can only perform WP on a part of the engage as user buffer is >> full. We cannot judge this has happened because of the former or later >> condition. So I'm assuming that user's parameters aren't solid enough and >> returning -EPERM. It seemed more suitable to me. But I can return -ENOSPC >> as well, if you say? > > Those two cases can be differentiated when checked before truncating > n_pages. If a user requests partial WP for a hugetlb page wouldn't > EINVAL (or other error - as this can't ever work) be more appropriate > (this check could happen only at the start of scan)? If the request is > due to max_pages limit (with found_pages > 0), then I'd return ENOSPC > and expect the user to restart the scan with a new buffer. Okay. I'll update. > > Our discussion here makes me wonder: what is the expected return value > for the ioctl WP (without GET) operation? If it would return e.g. the > number of 4K-pages successfully scanned, then the caller would be able > to detect the partial tail hugepage case and act accordingly. Returning error in case of only WP means partial hugetlb is hit. User should expect the partial wp on the memory area already. If we really want to return number of pages cleared, possible option of return value: * Return 0 means whole region is wp-ed * Return number of pages wp-ed if some error occurred (we need to handle this only for hugetlb) OR return -1 * number of pages wp-ed? An another uniform way can be to sum all wp-ed pages every time and return them * -. -1 is needed to let the know user that wp operation got halted mid way. > >>>> +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; >>> >>> Nit: If the page flags don't match (wouldn't be output), the limit >>> would not be hit and the calculation is unnecessary. But if it was >>> done in pagemap_scan_output() instead after all the flags checks... >> Correct for this use case. But moving to pagemap_scan_output() would make >> me do duplicate calculation for other 2 cases as explained above. > > (responded below the cases above) > >>>> + ret = pagemap_scan_output(false, vma->vm_file, false, false, p, addr, >>>> + n_pages); >>>> + >>>> + return ret; >>>> +} >>> [...] >>>> +static long do_pagemap_scan(struct mm_struct *mm, >>>> + struct pm_scan_arg __user *uarg) >>>> +{ >>>> + 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.flags = arg.flags; >>>> + p.required_mask = arg.required_mask; >>>> + p.anyof_mask = arg.anyof_mask; >>>> + p.excluded_mask = arg.excluded_mask; >>>> + p.return_mask = arg.return_mask; >>>> + p.cur.start = p.cur.len = p.cur.bitmap = 0; >>>> + p.vec = NULL; >>>> + p.vec_len = PAGEMAP_WALK_SIZE >> PAGE_SHIFT; >>> >>> If p.vec_len would not count the entry held in `cur` (IOW: vec_len = >>> WALK_SIZE - 1), then pagemap_scan_output() wouldn't need the big >>> comment about adding or subtracting 1 when checking for overflow. The >>> output vector needs to have space for at least one entrry to make GET >>> useful. Maybe `cur` could be renamed or annotated to express that it >>> always holds the last entry? >> Ohhh.. This can be done by doing subtracting 1 from empty_slots. But I've >> explored the idea. I don't see any benefit. If we do this, then I'll have >> to put a comment why subtracting 1 is needed. Seems like same problem: >> >> --- a/fs/proc/task_mmu.c >> +++ b/fs/proc/task_mmu.c >> @@ -1909,7 +1909,7 @@ static int pagemap_scan_output(bool wt, bool file, >> bool pres, bool swap, >> * represents the current index of vec array. We add 1 to the >> * vec_index while performing checks to account for data in >> cur. >> */ >> - if (cur->len && (p->vec_index + 1) >= p->vec_len) >> + if (cur->len && p->vec_index >= p->vec_len) >> return -ENOSPC; >> >> if (cur->len) { >> @@ -2202,7 +2202,7 @@ static long do_pagemap_scan(struct mm_struct *mm, >> if (IS_PM_SCAN_GET(p.flags)) { >> p.vec_index = 0; >> >> - empty_slots = arg.vec_len - vec_index; >> + empty_slots = arg.vec_len - 1 - vec_index; >> p.vec_len = min(p.vec_len, empty_slots); >> } >> >> Lets leave it as it is. I can change `cur` to `last` or any other name. >> Please suggest. > > The difference is that you have the subtraction only once per the > outer page_walk loop iteration, but in the current version the > addition has to happen every pagemap_scan_output() call after a hole. I'll update as you are saying. After updating, we cannot abort this outer loop if length is 0 as the last element is present in cur. > > From the readability perspective, "if (next_index >= vec_len)" is > short and self-documenting. Also I'd use "p.vec_len = min(p.vec_len, > empty_slots - 1)" as it also conveys the intent better (in that `vec` > is holding all but the last entry, but arg.vec_len holds the final > output buffer length). > > If we're picking colors, then maybe make `arg.vec_len` have a > different name than `p.vec_len` (same for `vec_index`) so that there > is less confusion possible as to what they refer to. Maybe keep > `arg.vec_len`, but have `p.vec_buf`, `p.vec_buf_len`, and > `p.next_buf_index`? So we do have colors in variable names. :) I'll update. > > (Note: you'd also need to decrement p.vec_len where it is first assigned.) > > [...] > Best Regards > Michał Mirosław
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c index 6259dd432eeb..ea29a298f7b8 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> @@ -1764,11 +1765,515 @@ static int pagemap_release(struct inode *inode, struct file *file) return 0; } +#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, cur; + unsigned long vec_len, vec_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 = *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 inline bool pagemap_scan_check_page_written(struct pagemap_scan_private *p) +{ + return (p->required_mask | p->anyof_mask | p->excluded_mask) & + PAGE_IS_WRITTEN; +} + +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 (pagemap_scan_check_page_written(p) && (!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 = &p->cur; + + 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->bitmap == bitmap && + cur->start + cur->len * PAGE_SIZE == addr) { + cur->len += n_pages; + p->found_pages += n_pages; + } else { + /* + * All data is copied to cur first. When more data is found, we + * push cur to vec and copy new data to cur. The vec_index + * represents the current index of vec array. We add 1 to the + * vec_index while performing checks to account for data in cur. + */ + if (cur->len && (p->vec_index + 1) >= p->vec_len) + return -ENOSPC; + + if (cur->len) { + memcpy(&p->vec[p->vec_index], cur, sizeof(*p->vec)); + p->vec_index++; + } + + cur->start = addr; + cur->len = n_pages; + cur->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; + unsigned long addr = end; + pte_t *pte, *orig_pte; + spinlock_t *ptl; + bool is_written; + 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_PM_SCAN_WP(p->flags)) + flush_tlb_range(vma, start, end); + + spin_unlock(ptl); + + arch_leave_lazy_mmu_mode(); + return ret; + } + +process_smaller_pages: + if (pmd_trans_unstable(pmd)) { + arch_leave_lazy_mmu_mode(); + walk->action = ACTION_AGAIN; + return 0; + } +#endif + + orig_pte = pte = pte_offset_map_lock(vma->vm_mm, pmd, start, &ptl); + for (addr = start; addr < end && !ret; pte++, addr += PAGE_SIZE) { + is_written = !is_pte_uffd_wp(*pte); + + if (IS_PM_SCAN_GET(p->flags)) + ret = pagemap_scan_output(is_written, vma->vm_file, + pte_present(*pte), + is_swap_pte(*pte), + p, addr, 1); + + if (ret >= 0 && is_written && IS_PM_SCAN_WP(p->flags)) + make_uffd_wp_pte(vma, addr, pte); + } + + if (IS_PM_SCAN_WP(p->flags)) + 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 pte; + + 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); + } + + pte = huge_ptep_get(ptep); + is_written = !is_huge_pte_uffd_wp(pte); + + /* + * Partial hugetlb page clear isn't supported + */ + if (is_written && IS_PM_SCAN_WP(p->flags) && + n_pages < HPAGE_SIZE/PAGE_SIZE) { + ret = -EPERM; + goto unlock_and_return; + } + + if (IS_PM_SCAN_GET(p->flags)) { + ret = pagemap_scan_output(is_written, vma->vm_file, + pte_present(pte), is_swap_pte(pte), + 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, pte); + 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 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, + struct pm_scan_arg __user *uarg) +{ + 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.flags = arg.flags; + p.required_mask = arg.required_mask; + p.anyof_mask = arg.anyof_mask; + p.excluded_mask = arg.excluded_mask; + p.return_mask = arg.return_mask; + p.cur.start = p.cur.len = p.cur.bitmap = 0; + p.vec = NULL; + p.vec_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 between different walks and + * store the p.cur at the end of the walk to the user buffer. + */ + if (IS_PM_SCAN_GET(p.flags)) { + p.vec = kmalloc_array(p.vec_len, sizeof(*p.vec), GFP_KERNEL); + if (!p.vec) + 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_index = 0; + + empty_slots = arg.vec_len - vec_index; + p.vec_len = min(p.vec_len, empty_slots); + } + + 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 != -ENOSPC && ret != PM_SCAN_FOUND_MAX_PAGES) + goto free_data; + + walk_start = walk_end; + if (IS_PM_SCAN_GET(p.flags) && p.vec_index) { + if (copy_to_user(&vec[vec_index], p.vec, + p.vec_index * sizeof(*p.vec))) { + /* + * Return error even though the OP succeeded + */ + ret = -EFAULT; + goto free_data; + } + vec_index += p.vec_index; + } + } + + if (IS_PM_SCAN_GET(p.flags) && p.cur.len) { + if (copy_to_user(&vec[vec_index], &p.cur, sizeof(*p.vec))) { + 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); + return ret; +} + +static long do_pagemap_cmd(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct pm_scan_arg __user *uarg = (struct pm_scan_arg __user *)arg; + struct mm_struct *mm = file->private_data; + + switch (cmd) { + case PAGEMAP_SCAN: + return do_pagemap_scan(mm, uarg); + + 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 21f942025fec..e067a944fe77 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 2b8559f9c1e2..e7711055fea0 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -4983,7 +4983,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;
This IOCTL, PAGEMAP_SCAN on pagemap file can be used to get and/or clear the info about page table entries. The following operations are supported in this ioctl: - Get the information if the pages have been written-to (PAGE_IS_WRITTEN), file mapped (PAGE_IS_FILE), present (PAGE_IS_PRESENT) or swapped (PAGE_IS_SWAPPED). - Find pages which have been written-to and/or write protect the pages (atomic PM_SCAN_OP_GET + PM_SCAN_OP_WP) This IOCTL can be extended to get information about more PTE bits. Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> --- Changes in 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 changes --- fs/proc/task_mmu.c | 505 ++++++++++++++++++++++++++++++++++++++++ include/linux/hugetlb.h | 1 + include/uapi/linux/fs.h | 53 +++++ mm/hugetlb.c | 2 +- 4 files changed, 560 insertions(+), 1 deletion(-)