| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ec95c6bc-fcf8-e83b-b260-0d9e13ebb870@linux.alibaba.com>
Date: Mon, 17 Jun 2019 13:52:17 -0700
From: Yang Shi <yang.shi@...ux.alibaba.com>
To: gregkh@...uxfoundation.org, akpm@...ux-foundation.org,
aneesh.kumar@...ux.ibm.com, jstancek@...hat.com, mgorman@...e.de,
minchan@...nel.org, namit@...are.com, npiggin@...il.com,
peterz@...radead.org, will.deacon@....com
Cc: stable@...r.kernel.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Subject: Re: [5.1-stable PATCH] mm: mmu_gather: remove __tlb_reset_range() for
force flush
This patch is wrong, please disregard this one. The corrected one will
be posted soon. Sorry for the inconvenience.
Yang
On 6/17/19 1:46 PM, Yang Shi wrote:
> A few new fields were added to mmu_gather to make TLB flush smarter for
> huge page by telling what level of page table is changed.
>
> __tlb_reset_range() is used to reset all these page table state to
> unchanged, which is called by TLB flush for parallel mapping changes for
> the same range under non-exclusive lock (i.e. read mmap_sem). Before
> commit dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in
> munmap"), the syscalls (e.g. MADV_DONTNEED, MADV_FREE) which may update
> PTEs in parallel don't remove page tables. But, the forementioned
> commit may do munmap() under read mmap_sem and free page tables. This
> may result in program hang on aarch64 reported by Jan Stancek. The
> problem could be reproduced by his test program with slightly modified
> below.
>
> ---8<---
>
> static int map_size = 4096;
> static int num_iter = 500;
> static long threads_total;
>
> static void *distant_area;
>
> void *map_write_unmap(void *ptr)
> {
> int *fd = ptr;
> unsigned char *map_address;
> int i, j = 0;
>
> for (i = 0; i < num_iter; i++) {
> map_address = mmap(distant_area, (size_t) map_size, PROT_WRITE | PROT_READ,
> MAP_SHARED | MAP_ANONYMOUS, -1, 0);
> if (map_address == MAP_FAILED) {
> perror("mmap");
> exit(1);
> }
>
> for (j = 0; j < map_size; j++)
> map_address[j] = 'b';
>
> if (munmap(map_address, map_size) == -1) {
> perror("munmap");
> exit(1);
> }
> }
>
> return NULL;
> }
>
> void *dummy(void *ptr)
> {
> return NULL;
> }
>
> int main(void)
> {
> pthread_t thid[2];
>
> /* hint for mmap in map_write_unmap() */
> distant_area = mmap(0, DISTANT_MMAP_SIZE, PROT_WRITE | PROT_READ,
> MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> munmap(distant_area, (size_t)DISTANT_MMAP_SIZE);
> distant_area += DISTANT_MMAP_SIZE / 2;
>
> while (1) {
> pthread_create(&thid[0], NULL, map_write_unmap, NULL);
> pthread_create(&thid[1], NULL, dummy, NULL);
>
> pthread_join(thid[0], NULL);
> pthread_join(thid[1], NULL);
> }
> }
> ---8<---
>
> The program may bring in parallel execution like below:
>
> t1 t2
> munmap(map_address)
> downgrade_write(&mm->mmap_sem);
> unmap_region()
> tlb_gather_mmu()
> inc_tlb_flush_pending(tlb->mm);
> free_pgtables()
> tlb->freed_tables = 1
> tlb->cleared_pmds = 1
>
> pthread_exit()
> madvise(thread_stack, 8M, MADV_DONTNEED)
> zap_page_range()
> tlb_gather_mmu()
> inc_tlb_flush_pending(tlb->mm);
>
> tlb_finish_mmu()
> if (mm_tlb_flush_nested(tlb->mm))
> __tlb_reset_range()
>
> __tlb_reset_range() would reset freed_tables and cleared_* bits, but
> this may cause inconsistency for munmap() which do free page tables.
> Then it may result in some architectures, e.g. aarch64, may not flush
> TLB completely as expected to have stale TLB entries remained.
>
> Use fullmm flush since it yields much better performance on aarch64 and
> non-fullmm doesn't yields significant difference on x86.
>
> The original proposed fix came from Jan Stancek who mainly debugged this
> issue, I just wrapped up everything together.
>
> Fixes: dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")
> Reported-by: Jan Stancek <jstancek@...hat.com>
> Tested-by: Jan Stancek <jstancek@...hat.com>
> Suggested-by: Will Deacon <will.deacon@....com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Nick Piggin <npiggin@...il.com>
> Cc: "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>
> Cc: Nadav Amit <namit@...are.com>
> Cc: Minchan Kim <minchan@...nel.org>
> Cc: Mel Gorman <mgorman@...e.de>
> Cc: stable@...r.kernel.org 4.20+
> Signed-off-by: Yang Shi <yang.shi@...ux.alibaba.com>
> Signed-off-by: Jan Stancek <jstancek@...hat.com>
> ---
> mm/mmu_gather.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/mm/mmu_gather.c b/mm/mmu_gather.c
> index f2f03c6..3543b82 100644
> --- a/mm/mmu_gather.c
> +++ b/mm/mmu_gather.c
> @@ -92,9 +92,30 @@ void arch_tlb_finish_mmu(struct mmu_gather *tlb,
> {
> struct mmu_gather_batch *batch, *next;
>
> + /*
> + * If there are parallel threads are doing PTE changes on same range
> + * under non-exclusive lock (e.g., mmap_sem read-side) but defer TLB
> + * flush by batching, one thread may end up seeing inconsistent PTEs
> + * and result in having stale TLB entries. So flush TLB forcefully
> + * if we detect parallel PTE batching threads.
> + *
> + * However, some syscalls, e.g. munmap(), may free page tables, this
> + * needs force flush everything in the given range. Otherwise this
> + * may result in having stale TLB entries for some architectures,
> + * e.g. aarch64, that could specify flush what level TLB.
> + */
> if (force) {
> + /*
> + * The aarch64 yields better performance with fullmm by
> + * avoiding multiple CPUs spamming TLBI messages at the
> + * same time.
> + *
> + * On x86 non-fullmm doesn't yield significant difference
> + * against fullmm.
> + */
> + tlb->fullmm = 1;
> __tlb_reset_range(tlb);
> - __tlb_adjust_range(tlb, start, end - start);
> + tlb->freed_tables = 1;
> }
>
> tlb_flush_mmu(tlb);
Powered by blists - more mailing lists