Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220120202805.3369-1-shy828301@gmail.com> <af603cbe-4a38-9947-5e6d-9a9328b473fb@redhat.com>
 <CAG48ez1xuZdELb=5ed1i0ruoFu5kAaWsf0LgRXEGhrDAcHz8fw@mail.gmail.com>
 <f7f82234-7599-9e39-1108-f8fbe2c1efc9@redhat.com> <CAG48ez17d3p53tSfuDTNCaANyes8RNNU-2i+eFMqkMwuAbRT4Q@mail.gmail.com>
 <5b4e2c29-8f1a-5a68-d243-a30467cc02d4@redhat.com> <CAHbLzkqLTkVJk+z8wpa03ponf7k30=Sx6qULwsGsvr5cq5d1aw@mail.gmail.com>
 <5a565d5a-0540-4041-ce63-a8fd5d1bb340@redhat.com> <CAHbLzkqXy-W9sD5HFOK_rm_TR8uSP29b+RjKjA5zOZ+0dkqMbQ@mail.gmail.com>
 <2a1c5bd2-cb8c-b93b-68af-de620438d19a@redhat.com> <CAHbLzkrQiQyh=36fOtqcODU3RO92jBVxU0o7wU8PyHJ_83LjiQ@mail.gmail.com>
 <8c0430e5-fecb-3eda-3d40-e94caa8cbd78@redhat.com>
In-Reply-To: <8c0430e5-fecb-3eda-3d40-e94caa8cbd78@redhat.com>
From:   Yang Shi <shy828301@gmail.com>
Date:   Fri, 28 Jan 2022 08:53:05 -0800
Message-ID: <CAHbLzkpyShTYkSfUMsBPohm0eGvBhvmDmRNKUUWJk3wqh+nrkQ@mail.gmail.com>
Subject: Re: [v2 PATCH] fs/proc: task_mmu.c: don't read mapcount for migration entry
To:     David Hildenbrand <david@redhat.com>
Cc:     Jann Horn <jannh@google.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Matthew Wilcox <willy@infradead.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        stable <stable@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jan 28, 2022 at 12:02 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 27.01.22 22:16, Yang Shi wrote:
> > On Wed, Jan 26, 2022 at 10:54 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >>>>> 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."
> >
> > It seems a little bit confusing to me, it is not safe to call with PTL
> > held either, right? I'd like to rephrase the note to:
>
> The implication that could have been spelled out is that only a mapped
> page can get unmapped. (I know, there are some weird migration entries
> nowadays ...)

Yes, I see your point. Just felt "only a mapped page can get unmapped"
sounds not that straightforward (just my personal feeling). How's
about "It is not safe to call page_mapcount() even with PTL held if
the page is not mapped, especially for migration entries".

>
> /*
>  * 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, a mapped page cannot get unmapped and it's at safe to call
>  * page_mapcount(). Especially for migration entries, it's not safe to
>  * call page_mapcount(), so we treat the mapcount as being 1.
>  */
>
>
>
> --
> Thanks,
>
> David / dhildenb
>