Message-ID: <23fbca83-a175-4d5a-8cf7-ed54c1830d37@arm.com>
Date: Mon, 29 Apr 2024 10:29:07 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: John Hubbard <jhubbard@...dia.com>, Zi Yan <ziy@...dia.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Zi Yan
 <zi.yan@...rutgers.edu>, "Aneesh Kumar K.V" <aneesh.kumar@...nel.org>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] mm: Fix race between __split_huge_pmd_locked() and
 GUP-fast

On 27/04/2024 20:11, John Hubbard wrote:
> On 4/27/24 8:14 AM, Zi Yan wrote:
>> On 27 Apr 2024, at 0:41, John Hubbard wrote:
>>> On 4/25/24 10:07 AM, Ryan Roberts wrote:
>>>> __split_huge_pmd_locked() can be called for a present THP, devmap or
>>>> (non-present) migration entry. It calls pmdp_invalidate()
>>>> unconditionally on the pmdp and only determines if it is present or not
>>>> based on the returned old pmd. This is a problem for the migration entry
>>>> case because pmd_mkinvalid(), called by pmdp_invalidate() must only be
>>>> called for a present pmd.
>>>>
>>>> On arm64 at least, pmd_mkinvalid() will mark the pmd such that any
>>>> future call to pmd_present() will return true. And therefore any
>>>> lockless pgtable walker could see the migration entry pmd in this state
>>>> and start interpretting the fields as if it were present, leading to
>>>> BadThings (TM). GUP-fast appears to be one such lockless pgtable walker.
>>>> I suspect the same is possible on other architectures.
>>>>
>>>> Fix this by only calling pmdp_invalidate() for a present pmd. And for
>>>
>>> Yes, this seems like a good design decision (after reading through the
>>> discussion that you all had in the other threads).
>>
>> This will only be good for arm64 and does not prevent other arch developers
>> to write code breaking arm64, since only arm64's pmd_mkinvalid() can turn
>> a swap entry to a pmd_present() entry.
> 
> Well, let's characterize it in a bit more detail, then:

Hi All,

Thanks for all the feedback! I had thought that this patch would be entirely
uncontraversial - obviously I was wrong :)

I've read all the emails, and trying to summarize a way forward here...

> 
> 1) This patch will make things better for arm64. That's important!
> 
> 2) Equally important, this patch does not make anything worse for
>    other CPU arches.
> 
> 3) This patch represents a new design constraint on the CPU arch
>    layer, and thus requires documentation and whatever enforcement
>    we can provide, in order to keep future code out of trouble.

I know its only semantics, but I don't view this as a new design constraint. I
see it as an existing constraint that was previously being violated, and this
patch aims to fix that. The generic version of pmdp_invalidate() unconditionally
does a tlb invalidation on the address range covered by the pmd. That makes no
sense unless the pmd was previously present. So my conclusion is that the
function only expects to be called for present pmds.

Additionally Documentation/mm/arch_pgtable_helpers.rst already says this:

"
| pmd_mkinvalid             | Invalidates a mapped PMD [1]                     |
"

I read "mapped" to be a synonym for "present". So I think its already
documented. Happy to explcitly change "mapped" to "present" though, if it helps?

Finally, [1] which is linked from Documentation/mm/arch_pgtable_helpers.rst,
also implies this constraint, although it doesn't explicitly say it.

[1] https://lore.kernel.org/linux-mm/20181017020930.GN30832@redhat.com/

> 
> 3.a) See the VM_WARN_ON() hunks below.

It sounds like everybody would be happy if I sprinkle these into the arches that
override pmdp_invalidate[_ad]()? There are 3 arches that have their own version
of pmdp_invalidate(); powerpc, s390 and sparc. And 1 that has its own version of
pmdp_invalidate_ad(); x86. I'll add them in all of those.

I'll use VM_WARN_ON_ONCE() as suggested by John.

I'd rather not put it directly into pmd_mkinvalid() since that would set a
precedent for adding them absolutely everywhere. (e.g. pte_mkdirty(), ...).

> 
> 3.b) I like the new design constraint, because it is reasonable and
>      clearly understandable: don't invalidate a non-present page
>      table entry.
> 
> I do wonder if there is somewhere else that this should be documented?

If I change:

"
| pmd_mkinvalid             | Invalidates a mapped PMD [1]                     |
"

To:

"
| pmd_mkinvalid             | Invalidates a present PMD; do not call for       |
|                             non-present pmd [1]                              |
"

Is that sufficient? (I'll do the same for pud_mkinvalid() too.

> 
> 
> thanks,

Powered by blists - more mailing lists