On 5 Dec 2024, at 3:10, David Hildenbrand wrote: > On 05.12.24 09:04, Geert Uytterhoeven wrote: >> Hi Zi, >> >> On Wed, Dec 4, 2024 at 7:30 PM Zi Yan <ziy@xxxxxxxxxx> wrote: >>> On 4 Dec 2024, at 12: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? >>> >>> Hi Geert, >>> >>> Regarding the above concern, have you ever had CONFIG_INIT_ON_ALLOC_DEFAULT_ON >>> for your MIPS machine and encountered any issue? Or let me know if my reasoning >>> above is flawed. >>> >>> To test it, I wonder if you can 1) revert my patch and 2) turn on >>> CONFIG_INIT_ON_ALLOC_DEFAULT_ON for your MIPS machine and run some applications >>> to see if any error happens. >> >> That seems to work fine... >> >> Kernel log confirms it's enabled: >> -mem auto-init: stack:off, heap alloc:off, heap free:off >> +mem auto-init: stack:off, heap alloc:on, heap free:off > > If I'm not wrong that's expected ... because we'll be double-zeroing that memory, clearing the cache :) > > I guess the question is, how *effective* is CONFIG_INIT_ON_ALLOC_DEFAULT_ON on systems to prevent exposing un-zeroed data to userspace, when it doesn't end up doing the flush we really need. Hi Geert, Is it possible to run a 32bit kernel with HIGHMEM and CONFIG_INIT_ON_ALLOC_DEFAULT_ON on the machine (of course with my patch reverted)? Just to check my reasoning below. Thanks. Yes, it should work, since I forgot the actual issue is HIGHMEM+cache flush, not just cache flush is needed after clearing user page. For arch which needs to flush cache after clearing user page, with HIGHMEM, init_on_alloc first clears the page using kmap_addr0 without flushing the cache, then clear_user_page() clears the page using kmap_addr1 with cache flush. After returning to userspace, the cache lines of kmap_addr0 will be evicted and written back to RAM eventually, corrupting user data with 0s, because no one flushes them before returning to userspace. For a proper fix, I will add ARCH_HAS_OPS_AFTER_CLEAR_USER_PAGE and make mips, sh, sparc, arm, xtensa, nios2, m68k, parisc, csky, arc, and powerpc select it, then make alloc_zeroed() returns false if ARCH_HAS_OPS_AFTER_CLEAR_USER_PAGE is enabled. If my reasoning above is verified to be true, I will send a separate patch to disable CONFIG_INIT_ON_ALLOC_DEFAULT_ON if HIGHMEM && ARCH_HAS_OPS_AFTER_CLEAR_USER_PAGE. Best Regards, Yan, Zi
