linux-kernel - Re: [v2 PATCH] fs/proc: task_mmu.c: don't read mapcount for migration entry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2a1c5bd2-cb8c-b93b-68af-de620438d19a@redhat.com>
Date:   Wed, 26 Jan 2022 19:54:53 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Yang Shi <shy828301@...il.com>
Cc:     Jann Horn <jannh@...gle.com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Matthew Wilcox <willy@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux MM <linux-mm@...ck.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>
Subject: Re: [v2 PATCH] fs/proc: task_mmu.c: don't read mapcount for migration
 entry

>>> Just page lock or elevated page refcount could serialize against THP
>>> split AFAIK.
>>>
>>>>
>>>> But yeah, using the mapcount of a page that is not even mapped
>>>> (migration entry) is clearly wrong.
>>>>
>>>> To summarize: reading the mapcount on an unlocked page will easily
>>>> return a wrong result and the result should not be relied upon. reading
>>>> the mapcount of a migration entry is dangerous and certainly wrong.
>>>
>>> Depends on your usecase. Some just want to get a snapshot, just like
>>> smaps, they don't care.
>>
>> Right, but as discussed, even the snapshot might be slightly wrong. That
>> might be just fine for smaps (and I would have enjoyed a comment in the
>> code stating that :) ).
> 
> I think that is documented already, see Documentation/filesystems/proc.rst:
> 
> Note: reading /proc/PID/maps or /proc/PID/smaps is inherently racy (consistent
> output can be achieved only in the single read call).

Right, but I think there is a difference between

* Atomic values that change immediately afterwards ("this value used to
  be true at one point in time")
* Values that are unstable because we cannot read them atomically ("this
  value never used to be true")

I'd assume with the documented race we actually talk about the first
point, but I might be just wrong.

> 
> Of course, if the extra note is preferred in the code, I could try to
> add some in a separate patch.

When staring at the (original) code I would have hoped to find something
like:

/*
 * We use page_mapcount() to get a snapshot of the mapcount. Without
 * holding the page lock this snapshot can be slightly wrong as we
 * cannot always read the mapcount atomically. As long we hold the PT
 * lock, the page cannot get unmapped and it's at safe to call
 * page_mapcount().
 */

With the addition of

"... For unmapped pages (e.g., migration entries) we cannot guarantee
that, so treat the mapcount as being 1."

But this is just my personal preference ... :) I do think the patch does
the right thing in regard to migration entries.

-- 
Thanks,

David / dhildenb