All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@kernel.org>
To: Toshi Kani <toshi.kani@hpe.com>
Cc: mhocko@suse.com, akpm@linux-foundation.org, tglx@linutronix.de,
	mingo@redhat.com, hpa@zytor.com, bp@suse.de,
	catalin.marinas@arm.com, guohanjun@huawei.com,
	will.deacon@arm.com, wxf.wang@hisilicon.com, linux-mm@kvack.org,
	x86@kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Andy Lutomirski <luto@kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>
Subject: Re: [PATCH 1/2] mm/vmalloc: Add interfaces to free unused page table
Date: Thu, 8 Mar 2018 09:08:47 +0100	[thread overview]
Message-ID: <20180308080847.dvwd3w6wuhwsg3qo@gmail.com> (raw)
In-Reply-To: <20180307183227.17983-2-toshi.kani@hpe.com>


* Toshi Kani <toshi.kani@hpe.com> wrote:

> On architectures with CONFIG_HAVE_ARCH_HUGE_VMAP set, ioremap()
> may create pud/pmd mappings.  Kernel panic was observed on arm64
> systems with Cortex-A75 in the following steps as described by
> Hanjun Guo.
> 
> 1. ioremap a 4K size, valid page table will build,
> 2. iounmap it, pte0 will set to 0;
> 3. ioremap the same address with 2M size, pgd/pmd is unchanged,
>    then set the a new value for pmd;
> 4. pte0 is leaked;
> 5. CPU may meet exception because the old pmd is still in TLB,
>    which will lead to kernel panic.
> 
> This panic is not reproducible on x86.  INVLPG, called from iounmap,
> purges all levels of entries associated with purged address on x86.

Where does x86 iounmap() do that?

> x86 still has memory leak.
> Add two interfaces, pud_free_pmd_page() and pmd_free_pte_page(),
> which clear a given pud/pmd entry and free up a page for the lower
> level entries.
> 
> This patch implements their stub functions on x86 and arm64, which
> work as workaround.

At minimum the ordering of the patches is very confusing: why don't you introduce 
the new methods in patch #1, and then use them in patch #2?

Also please double check the coding style of your patches, there's a number of 
obvious problems of outright bad patterns and also cases where you clearly don't 
try to follow the (correct) style of existing code.

Thanks,

	Ingo

WARNING: multiple messages have this Message-ID (diff)
From: mingo@kernel.org (Ingo Molnar)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/2] mm/vmalloc: Add interfaces to free unused page table
Date: Thu, 8 Mar 2018 09:08:47 +0100	[thread overview]
Message-ID: <20180308080847.dvwd3w6wuhwsg3qo@gmail.com> (raw)
In-Reply-To: <20180307183227.17983-2-toshi.kani@hpe.com>


* Toshi Kani <toshi.kani@hpe.com> wrote:

> On architectures with CONFIG_HAVE_ARCH_HUGE_VMAP set, ioremap()
> may create pud/pmd mappings.  Kernel panic was observed on arm64
> systems with Cortex-A75 in the following steps as described by
> Hanjun Guo.
> 
> 1. ioremap a 4K size, valid page table will build,
> 2. iounmap it, pte0 will set to 0;
> 3. ioremap the same address with 2M size, pgd/pmd is unchanged,
>    then set the a new value for pmd;
> 4. pte0 is leaked;
> 5. CPU may meet exception because the old pmd is still in TLB,
>    which will lead to kernel panic.
> 
> This panic is not reproducible on x86.  INVLPG, called from iounmap,
> purges all levels of entries associated with purged address on x86.

Where does x86 iounmap() do that?

> x86 still has memory leak.
> Add two interfaces, pud_free_pmd_page() and pmd_free_pte_page(),
> which clear a given pud/pmd entry and free up a page for the lower
> level entries.
> 
> This patch implements their stub functions on x86 and arm64, which
> work as workaround.

At minimum the ordering of the patches is very confusing: why don't you introduce 
the new methods in patch #1, and then use them in patch #2?

Also please double check the coding style of your patches, there's a number of 
obvious problems of outright bad patterns and also cases where you clearly don't 
try to follow the (correct) style of existing code.

Thanks,

	Ingo

  parent reply	other threads:[~2018-03-08  8:08 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-07 18:32 [PATCH 0/2] fix memory leak / panic in ioremap huge pages Toshi Kani
2018-03-07 18:32 ` Toshi Kani
2018-03-07 18:32 ` Toshi Kani
2018-03-07 18:32 ` [PATCH 1/2] mm/vmalloc: Add interfaces to free unused page table Toshi Kani
2018-03-07 18:32   ` Toshi Kani
2018-03-07 18:32   ` Toshi Kani
2018-03-07 22:54   ` Andrew Morton
2018-03-07 22:54     ` Andrew Morton
2018-03-07 23:02     ` Kani, Toshi
2018-03-07 23:02       ` Kani, Toshi
2018-03-07 22:55   ` Andrew Morton
2018-03-07 22:55     ` Andrew Morton
2018-03-08  4:00   ` Matthew Wilcox
2018-03-08  4:00     ` Matthew Wilcox
2018-03-08 15:56     ` Kani, Toshi
2018-03-08 15:56       ` Kani, Toshi
2018-03-08 22:07       ` Matthew Wilcox
2018-03-08 22:07         ` Matthew Wilcox
2018-03-08 23:27         ` Kani, Toshi
2018-03-08 23:27           ` Kani, Toshi
2018-03-08  8:08   ` Ingo Molnar [this message]
2018-03-08  8:08     ` Ingo Molnar
2018-03-08 18:04   ` Will Deacon
2018-03-08 18:04     ` Will Deacon
2018-03-08 19:30     ` Kani, Toshi
2018-03-08 19:30       ` Kani, Toshi
2018-03-07 18:32 ` [PATCH 2/2] x86/mm: implement free pmd/pte page interfaces Toshi Kani
2018-03-07 18:32   ` Toshi Kani
2018-03-07 18:32   ` Toshi Kani
2018-03-07 23:01   ` Andrew Morton
2018-03-07 23:01     ` Andrew Morton
2018-03-07 23:22     ` Kani, Toshi
2018-03-07 23:22       ` Kani, Toshi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180308080847.dvwd3w6wuhwsg3qo@gmail.com \
    --to=mingo@kernel.org \
    --cc=a.p.zijlstra@chello.nl \
    --cc=akpm@linux-foundation.org \
    --cc=bp@suse.de \
    --cc=catalin.marinas@arm.com \
    --cc=guohanjun@huawei.com \
    --cc=hpa@zytor.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luto@kernel.org \
    --cc=mhocko@suse.com \
    --cc=mingo@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=toshi.kani@hpe.com \
    --cc=will.deacon@arm.com \
    --cc=wxf.wang@hisilicon.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.