Re: [PATCH v2 1/2] mm/migrate: Fix read-only page got writable when recover pte

From: Alistair Popple <apopple@nvidia.com>
To: Peter Xu <peterx@redhat.com>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Andrea Arcangeli <aarcange@redhat.com>,
	Axel Rasmussen <axelrasmussen@google.com>,
	Ives van Hoorne <ives@codesandbox.io>,
	Nadav Amit <nadav.amit@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Mike Rapoport <rppt@linux.vnet.ibm.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH v2 1/2] mm/migrate: Fix read-only page got writable when recover pte
Date: Mon, 14 Nov 2022 17:22:15 +1100	[thread overview]
Message-ID: <871qq6vxx6.fsf@nvidia.com> (raw)
In-Reply-To: <Y3GELocAJ+p+gKpc@x1n>

Peter Xu <peterx@redhat.com> writes:

> On Fri, Nov 11, 2022 at 10:42:13AM +1100, Alistair Popple wrote:
>> Hi Peter, for the patch feel free to add:
>> 
>> Reviewed-by: Alistair Popple <apopple@nvidia.com>
>
> Will do, thanks.
>
>> 
>> I did wonder if this should be backported further for migrate_vma as
>> well given that a migration failure there might lead a shmem read-only
>> PTE to become read-write. I couldn't think of an obvious reason why that
>> would cause an actual problem though.
>> 
>> I think folio_mkclean() will wrprotect the pte for writeback to swap,
>> but it holds the page lock which prevents migrate_vma installing
>> migration entries in the first place.
>> 
>> I suppose there is a small window there because migrate_vma will unlock
>> the page before removing the migration entries. So to be safe we could
>> consider going back to 8763cb45ab96 ("mm/migrate: new memory migration
>> helper for use with device memory") but I doubt in practice it's a real
>> problem.
>
> IIRC migrate_vma API only supports anonymous memory, then it's not
> affected by this issue?

It does only support anonymous memory, but it doesn't actually check
that until after the page mapping has already been replaced with
migration entries.

So I was worried that could remap a previously read-only page as
read-write and what interaction that would have with a page that was
swapped out to disk say. But I haven't tought of a case where that would
be a problem, and I'm pretty convinced it isn't. It's just I haven't had
the time to entirely prove that for myself.

> One thing reminded me is I thought mprotect could be affected but I think
> it's actually not, because mprotect is vma-based, and that should always be
> fine with current mk_pte().  I'll remove the paragraph on mprotect in the
> commit message; that could be slightly misleading.