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

["Michał Mirosław" <emmir@google.com>]

(A quick reply to answer open questions in case they help the next version.)

On Wed, 14 Jun 2023 at 19:10, Muhammad Usama Anjum
<usama.anjum@collabora.com> wrote:
> On 6/14/23 8:14 PM, Michał Mirosław wrote:
> > On Wed, 14 Jun 2023 at 15:46, Muhammad Usama Anjum
> > <usama.anjum@collabora.com> wrote:
> >>
> >> On 6/14/23 3:36 AM, Michał Mirosław wrote:
> >>> On Tue, 13 Jun 2023 at 12:29, Muhammad Usama Anjum
> >>> <usama.anjum@collabora.com> wrote:
[...]
> >>>> +       if (cur_buf->bitmap == bitmap &&
> >>>> +           cur_buf->start + cur_buf->len * PAGE_SIZE == addr) {
> >>>> +               cur_buf->len += n_pages;
> >>>> +               p->found_pages += n_pages;
> >>>> +       } else {
> >>>> +               if (cur_buf->len && p->vec_buf_index >= p->vec_buf_len)
> >>>> +                       return -ENOMEM;
> >>>
> >>> Shouldn't this be -ENOSPC? -ENOMEM usually signifies that the kernel
> >>> ran out of memory when allocating, not that there is no space in a
> >>> user-provided buffer.
> >> There are 3 kinds of return values here:
> >> * PM_SCAN_FOUND_MAX_PAGES (1) ---> max_pages have been found. Abort the
> >> page walk from next entry
> >> * 0 ---> continue the page walk
> >> * -ENOMEM --> Abort the page walk from current entry, user buffer is full
> >> which is not error, but only a stop signal. This -ENOMEM is just
> >> differentiater from (1). This -ENOMEM is for internal use and isn't
> >> returned to user.
> >
> > But why ENOSPC is not good here? I was used before, I think.
> -ENOSPC is being returned in form of true error from
> pagemap_scan_hugetlb_entry(). So I'd to remove -ENOSPC from here as it
> wasn't true error here, it was only a way to abort the walk immediately.
> I'm liking the following erturn code from here now:
>
> #define PM_SCAN_BUFFER_FULL     (-256)

I guess this will be reworked anyway, but I'd prefer this didn't need
custom errors etc. If we agree to decoupling the selection and GET
output, it could be:

bool is_interesting_page(p, flags); // this one does the
required/anyof/excluded match
size_t output_range(p, start, len, flags); // this one fills the
output vector and returns how many pages were fit

In this setup, `is_interesting_page() && (n_out = output_range()) <
n_pages` means this is the final range, no more will fit. And if
`n_out == 0` then no pages fit and no WP is needed (no other special
cases).

> >>> For flags name: PM_REQUIRE_WRITE_ACCESS?
> >>> Or Is it intended to be checked only if doing WP (as the current name
> >>> suggests) and so it would be redundant as WP currently requires
> >>> `p->required_mask = PAGE_IS_WRITTEN`?
> >> This is intended to indicate that if userfaultfd is needed. If
> >> PAGE_IS_WRITTEN is mentioned in any of mask, we need to check if
> >> userfaultfd has been initialized for this memory. I'll rename to
> >> PM_SCAN_REQUIRE_UFFD.
> >
> > Why do we need that check? Wouldn't `is_written = false` work for vmas
> > not registered via uffd?
> UFFD_FEATURE_WP_ASYNC and UNPOPULATED needs to be set on the memory region
> for it to report correct written values on the memory region. Without UFFD
> WP ASYNC and UNPOUPULATED defined on the memory, we consider UFFD_WP state
> undefined. If user hasn't initialized memory with UFFD, he has no right to
> set is_written = false.

How about calculating `is_written = is_uffd_registered() &&
is_uffd_wp()`? This would enable a user to apply GET+WP for the whole
address space of a process regardless of whether all of it is
registered.

> > While here, I wonder if we really need to fail the call if there are
> > unknown bits in those masks set: if this bit set is expanded with
> > another category flags, a newer userspace run on older kernel would
> > get EINVAL even if the "treat unknown as 0" be what it requires.
> > There is no simple way in the API to discover what bits the kernel
> > supports. We could allow a no-op (no WP nor GET) call to help with
> > that and then rejecting unknown bits would make sense.
> I've not seen any examples of this. But I've seen examples of returning
> error if kernel doesn't support a feature. Each new feature comes with a
> kernel version, greater than this version support this feature. If user is
> trying to use advanced feature which isn't present in a kernel, we should
> return error and not proceed to confuse the user/kernel. In fact if we look
> at userfaultfd_api(), we return error immediately if feature has some bit
> set which kernel doesn't support.

I think we should have a way of detecting the supported flags if we
don't want a forward compatibility policy for flags here. Maybe it
would be enough to allow all the no-op combinations for this purpose?

> >>> [...]
> >>>> --- a/include/uapi/linux/fs.h
> >>>> +++ b/include/uapi/linux/fs.h
> >>>> +/*
> >>>> + * struct page_region - Page region with bitmap flags
> >>>> + * @start:     Start of the region
> >>>> + * @len:       Length of the region in pages
> >>>> + * bitmap:     Bits sets for the region
> >>>
> >>> '@' is missing for the third field. BTW, maybe we can call it
> >>> something like `flags` or `category` (something that hints at the
> >>> meaning of the value instead of its data representation).
> >> The deification of this struct says, "with bitmap flags". Bitmap was a
> >> different name. I'll update it to flags.
> >
> > From the implementation and our discussions I guess the
> > `bitmap`/`flags` field is holding a set of matching categories: a bit
> > value 1 = pages are in this category, value 0 = pages are not in this
> > category.
> >
> >>>> +/*
> >>>> + * struct pm_scan_arg - Pagemap ioctl argument
> >>>> + * @size:              Size of the structure
> >>>> + * @flags:             Flags for the IOCTL
> >>>> + * @start:             Starting address of the region
> >>>> + * @len:               Length of the region (All the pages in this length are included)
> >>>
> >>> Maybe `scan_start`, `scan_len` - so that there is a better distinction
> >>> from the structure's `size` field?
> >> As start and len already communicate the meaning. We are making things more
> >> verbose.
> >
> > We are describing (in the name) only that it is a range, but not of
> > what or what purpose. That information is only in the docstring, but
> > it is harder to get by someone just reading the code.
> Agreed. But I'm using same names, start and len which mincore (a historic
> syscall) is using. I've followed mincore here.

mincore() doesn't take parameters as a struct, but as three positional
arguments (whose names don't matter nor appear at call point) - I
wouldn't take it as a precedent for structure field naming.

> >>>> + * @vec:               Address of page_region struct array for output
> >>>> + * @vec_len:           Length of the page_region struct array
> >>>> + * @max_pages:         Optional max return pages
> >>>> + * @required_mask:     Required mask - All of these bits have to be set in the PTE
> >>>> + * @anyof_mask:                Any mask - Any of these bits are set in the PTE
> >>>> + * @excluded_mask:     Exclude mask - None of these bits are set in the PTE
> >>>> + * @return_mask:       Bits that are to be reported in page_region
> >>>> + */
> >>>
> >>> I skipped most of the page walk implementation as maybe the comments
> >>> above could make it simpler. Reading this patch and the documentation
> >>> I still feel confused about how the filtering/limiting parameters
> >> I'm really sad to hear this. I've been working on making this series from
> >> so many revisions. I was hopping that it would make complete sense to
> >> reviewers and later to users.
> >>
> >> What do you think is missing which is restricting these patches getting
> >> accepted to upstream?
> >>
> >>> should affect GET, WP and WP+GET. Should they limit the pages walked
> >>> (and WP-ed)? Or only the GET's output? How about GET+WP case?
> >> The address range needs to be walked until max pages pages are found, user
> >> buffer is full or whole range is walked. If the page will be added to user
> >> buffer or not depends on the selection criteria (*masks). There is no
> >> difference in case of walk for GET, WP and GET+WP. Only that WP doesn't
> >> take any user buffer and just WPs the whole region.
> >
> > Ok, then this intent (if I understand correctly) does not entirely
> > match the implementation. Let's split up the conditions:
> >
> > 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