On Wed, Nov 10, 2021 at 09:30:50AM +0100, David Hildenbrand wrote:
> On 10.11.21 09:27, Peter Xu wrote:
> > On Wed, Nov 10, 2021 at 09:14:42AM +0100, David Hildenbrand wrote:
> >> On 10.11.21 08:03, Peter Xu wrote:
> >>> Hi, Mina,
> >>>
> >>> Sorry to comment late.
> >>>
> >>> On Sun, Nov 07, 2021 at 03:57:54PM -0800, Mina Almasry wrote:
> >>>> diff --git a/Documentation/admin-guide/mm/pagemap.rst b/Documentation/admin-guide/mm/pagemap.rst
> >>>> index fdc19fbc10839..8a0f0064ff336 100644
> >>>> --- a/Documentation/admin-guide/mm/pagemap.rst
> >>>> +++ b/Documentation/admin-guide/mm/pagemap.rst
> >>>> @@ -23,7 +23,8 @@ There are four components to pagemap:
> >>>> * Bit 56 page exclusively mapped (since 4.2)
> >>>> * Bit 57 pte is uffd-wp write-protected (since 5.13) (see
> >>>> :ref:`Documentation/admin-guide/mm/userfaultfd.rst <userfaultfd>`)
> >>>> - * Bits 57-60 zero
> >>>> + * Bit 58 page is a huge (PMD size) THP mapping
> >>>> + * Bits 59-60 zero
> >>>> * Bit 61 page is file-page or shared-anon (since 3.5)
> >>>> * Bit 62 page swapped
> >>>> * Bit 63 page present
> >>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> >>>> index ad667dbc96f5c..6f1403f83b310 100644
> >>>> --- a/fs/proc/task_mmu.c
> >>>> +++ b/fs/proc/task_mmu.c
> >>>> @@ -1302,6 +1302,7 @@ struct pagemapread {
> >>>> #define PM_SOFT_DIRTY BIT_ULL(55)
> >>>> #define PM_MMAP_EXCLUSIVE BIT_ULL(56)
> >>>> #define PM_UFFD_WP BIT_ULL(57)
> >>>> +#define PM_HUGE_THP_MAPPING BIT_ULL(58)
> >>>
> >>> The ending "_MAPPING" seems redundant to me, how about just call it "PM_THP" or
> >>> "PM_HUGE" (as THP also means HUGE already)?
> >>>
> >>> IMHO the core problem is about permission controls, and it seems to me we're
> >>> actually trying to workaround it by duplicating some information we have.. so
> >>> it's kind of a pity. Totally not against this patch, but imho it'll be nicer
> >>> if it's the permission part that to be enhanced, rather than a new but slightly
> >>> duplicated interface.
> >>
> >> It's not a permission problem AFAIKS: even with permissions "changed",
> >> any attempt to use /proc/kpageflags is just racy. Let's not go down that
> >> path, it's really the wrong mechanism to export to random userspace.
> >
> > I agree it's racy, but IMHO that's fine. These are hints for userspace to make
> > decisions, they cannot be always right. Even if we fetch atomically and seeing
> > that this pte is swapped out, it can be quickly accessed at the same time and
> > it'll be in-memory again. Only if we can freeze the whole pgtable but we
> > can't, so they can only be used as hints.
>
> Sorry, I don't think /proc/kpageflags (or exporting the PFNs to random
> users via /proc/self/pagemap) is the way to go.
>
> "Since Linux 4.0 only users with the CAP_SYS_ADMIN capability can get
> PFNs. In 4.0 and 4.1 opens by unprivileged fail with -EPERM. Starting
> from 4.2 the PFN field is zeroed if the user does not have
> CAP_SYS_ADMIN. Reason: information about PFNs helps in exploiting
> Rowhammer vulnerability."
IMHO these are two problems that you mentioned. That's also what I was
wondering about: could the app be granted with CAP_SYS_ADMIN then?
I am not sure whether that'll work well with /proc/kpage* though, as it's by
default 0400. So perhaps we need to manual adjust the file permission too to
make sure the app can both access PFNs (with SYS_ADMIN) and the flags. Totally
no expert on the permissions..
>
> >
> >>
> >> We do have an interface to access this information from userspace
> >> already: /proc/self/smaps IIRC. Mina commented that they are seeing
> >> performance issues with that approach.
> >>
> >> It would be valuable to add these details to the patch description,
> >> including a performance difference when using both interfaces we have
> >> available. As the patch description stands, there is no explanation
> >> "why" we want this change.
> >
> > I didn't notice Mina mention about performance issues with kpageflags, if so
> > then I agree this solution helps.
> The performance issue seems to be with /proc/self/smaps.
This also reminded me that we've got issue with smaps being too slow, and in
many cases we're only interested in a small portion of the whole memory. This
made me wonder how about a new smaps interface taking memory range as input.
Thanks,
--
Peter Xu
