Re: [PATCH v4] mm: Add PM_HUGE_THP_MAPPING to /proc/pid/pagemap

[David Hildenbrand]

On 10.11.21 09:57, Peter Xu wrote:
> 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..

Me too :)

IIRC changing permissions that was not an option -- which is why the
first approach suggested a new /proc/self/pageflags. But I guess Mina
can remind us (and eventually document all that in the patch description
:) ).


-- 
Thanks,

David / dhildenb