Re: [PATCH] mm: avoid zeroing user movable page twice with init_on_alloc=1

[Zi Yan <ziy@nvidia.com>]

On 4 Dec 2024, at 12:46, Vlastimil Babka wrote:

> On 12/4/24 18:33, Zi Yan wrote:
>> On 4 Dec 2024, at 11:29, Matthew Wilcox wrote:
>>
>>> On Wed, Dec 04, 2024 at 11:16:51AM -0500, Zi Yan wrote:
>>>>> So maybe the clearing done as part of page allocator isn't enough here.
>>>>>
>>>> Basically, mips needs to flush data cache if kmap address is aliased to
>>>
>>> People use "aliased" in contronym ways.  Do you mean "has a
>>> non-congruent alias" or "has a congruent alias"?
>>>
>>>> userspace address. This means when mips has THP on, the patch below
>>>> is not enough to fix the issue.
>>>>
>>>> In post_alloc_hook(), it does not make sense to pass userspace address
>>>> in to determine whether to flush dcache or not.
>>>>
>>>> One way to fix it is to add something like arch_userpage_post_alloc()
>>>> to flush dcache if kmap address is aliased to userspace address.
>>>> But my questions are that
>>>> 1) if kmap address will always be the same for two separate kmap_local() calls,
>>>
>>> No.  It just takes the next address in the stack.
>>
>> Hmm, if kmap_local() gives different addresses, wouldn’t init_on_alloc be
>> causing issues before my patch? In the page allocator, the page is zeroed
>> from one kmap address without flush, then clear_user_highpage() clears
>> it again with another kmap address with flush. After returning to userspace,
>> the user application works on the page but when the cache line used by
>> init_on_alloc is written back (with 0s) at eviction, user data is corrupted.
>> Am I missing anything? Or all arch with cache aliasing never enables
>> init_on_alloc?
>
> Maybe the arch also defines some hooks like arch_kmap_local_post_unmap() ?

But this does not solve the possible init_on_alloc issue, since init_on_alloc
is done in mm/page_alloc.c without userspace address and has no knowledge of
whether the zeroed page will be used in userspace nor the cache line will
be the same as the userspace page cache line. If dcache is flushed
unconditionally for kmap_local, that could degrade performance.

> As for the fix, could it rely on e.g. __HAVE_ARCH_COPY_USER_HIGHPAGE instead
> of CONFIG_MIPS? That affects more arches, I don't know if we broke only mips
> or others too.

Yes, this is much better, since this issue affects any arch with cache aliasing.
Let me update my fix. Thanks.


Best Regards,
Yan, Zi