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
