Re: [PATCH 1/2] fs/proc/task_mmu: add guard region bit to pagemap

On Wed, Mar 19, 2025 at 07:12:45PM +0000, Lorenzo Stoakes wrote:
> +cc Greg for stable question
> 
> On Wed, Mar 19, 2025 at 11:22:40AM -0700, Andrei Vagin wrote:
> > On Mon, Feb 24, 2025 at 2:39 AM David Hildenbrand <[email protected]> wrote:
> > >
> > > On 24.02.25 11:18, Lorenzo Stoakes wrote:
> 
> [snip]
> > > >>
> > > >> Acked-by: David Hildenbrand <[email protected]>
> > > >
> > > > Thanks! :)
> > > >>
> > > >> Something that might be interesting is also extending the PAGEMAP_SCAN
> > > >> ioctl.
> > > >
> > > > Yeah, funny you should mention that, I did see that, but on reading the man
> > > > page it struck me that it requires the region to be uffd afaict? All the
> > > > tests seem to establish uffd, and the man page implies it:
> > > >
> > > >         To start tracking the written state (flag) of a page or range of
> > > >         memory, the UFFD_FEATURE_WP_ASYNC must be enabled by UFFDIO_API
> > > >         ioctl(2) on userfaultfd and memory range must be registered with
> > > >         UFFDIO_REGISTER ioctl(2) in UFFDIO_REGISTER_MODE_WP mode.
> > > >
> > > > It would be a bit of a weird edge case to add support there. I was excited
> > > > when I first saw this ioctl, then disappointed afterwards... but maybe I
> > > > got it wrong?
> >
> > > >
> > >
> > > I never managed to review that fully, but I thing that
> > > UFFD_FEATURE_WP_ASYNC thingy is only required for PM_SCAN_CHECK_WPASYNC
> > > and PM_SCAN_WP_MATCHING.
> > >
> > > See pagemap_scan_test_walk().
> > >
> > > I do recall that it works on any VMA.
> > >
> > > Ah yes, tools/testing/selftests/mm/vm_util.c ends up using it for
> > > pagemap_is_swapped() and friends via page_entry_is() to sanity check
> > > that what pagemap gives us is consistent with what pagemap_scan gives us.
> > >
> > > So it should work independent of the uffd magic.
> > > I might be wrong, though ...
> >
> >
> > PAGEMAP_SCAN can work without the UFFD magic. CRIU utilizes PAGEMAP_SCAN
> > as a more efficient alternative to /proc/pid/pagemap:
> > https://github.com/checkpoint-restore/criu/blob/d18912fc88f3dc7bde5fdfa3575691977eb21753/criu/pagemap-cache.c#L178
> >
> 
> Yeah we ascertained that - is on my list, LSF coming up next week means we
> aren't great on timing here, but I'll prioritise this. When I'm back.
> 
> > For CRIU, obtaining information about guard regions is critical.
> > Without this functionality in the kernel, CRIU is broken. We probably should
> > consider backporting these changes to the 6.13 and 6.14 stable branches.
> >
> 
> I'm not sure on precedent for backporting a feature like this - Greg? Am
> happy to do it though.

If it's a regression, sure, we can take it for stable.

> As a stop gap we can backport the pagemap feature if Greg feels this is
> appropriate?

Which do the maintainers of the code feel is appropriate?  I'll defer to
them for making that call :)

thanks,

greg k-h