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

[John Hubbard <jhubbard@nvidia.com>]

On 12/4/24 1:21 PM, Zi Yan wrote:
> On 4 Dec 2024, at 13:16, Zi Yan wrote:
> 
>> On 4 Dec 2024, at 13:13, Zi Yan wrote:
>>
>>> 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.
>>
>> I notice that arm64 has __HAVE_ARCH_COPY_USER_HIGHPAGE defined, so I will
>> need to look for an alternative.
> 
> It turns out sh, sparc, arm, xtensa, nios2, m68k, parisc, csky, and powerpc all have cache flush operations in clear_user_page() compared to clear_page() and
> arc clears PG_dc_clean bit in addition to clear_page().
> 
> So __GFP_ZERO cannot simply replace clear_user_page()/clear_user_highpage().
> I can add ARCH_REQUIRE_CLEAR_USER_PAGE for these arch and use it to decide
> whether clear_user_page()/clear_user_highpage() needs to be used regardless of
> the presence of init_on_alloc.
> 
> I also wonder if INIT_ON_ALLOC_DEFAULT_ON works on these arch or not.
> 

Well, I've been waiting to point out that if you actually *delete* the
entire INIT_ON_ALLOC feature, you'd be my personal hero. Defense in depth
is nice, but at some point, it crosses a line into the absurd, and I think
we are there. </pause and put on asbestos flame suit> :)

thanks,
-- 
John Hubbard