From: Chintan Pandya <cpandya@codeaurora.org> To: Mark Rutland <mark.rutland@arm.com> Cc: catalin.marinas@arm.com, will.deacon@arm.com, arnd@arndb.de, ard.biesheuvel@linaro.org, marc.zyngier@arm.com, james.morse@arm.com, kristina.martsenko@arm.com, takahiro.akashi@linaro.org, gregkh@linuxfoundation.org, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, akpm@linux-foundation.org, toshi.kani@hpe.com Subject: Re: [PATCH v1 3/4] arm64: Fix the page leak in pud/pmd_set_huge Date: Wed, 14 Mar 2018 16:57:29 +0530 [thread overview] Message-ID: <a3d0adc4-293a-328b-a8ea-ea410baa6849@codeaurora.org> (raw) In-Reply-To: <20180314105343.nxw2mwkm4pao3hur@lakrids.cambridge.arm.com> On 3/14/2018 4:23 PM, Mark Rutland wrote: > On Wed, Mar 14, 2018 at 02:18:24PM +0530, Chintan Pandya wrote: >> While setting huge page, we need to take care of >> previously existing next level mapping. Since, >> we are going to overrite previous mapping, the >> only reference to next level page table will get >> lost and the next level page table will be zombie, >> occupying space forever. So, free it before >> overriding. > >> @@ -939,6 +940,9 @@ int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot) >> return 0; >> >> BUG_ON(phys & ~PUD_MASK); >> + if (pud_val(*pud) && !pud_huge(*pud)) >> + free_page((unsigned long)__va(pud_val(*pud))); >> + >> set_pud(pudp, pfn_pud(__phys_to_pfn(phys), sect_prot)); >> return 1; >> } >> @@ -953,6 +957,9 @@ int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot) >> return 0; >> >> BUG_ON(phys & ~PMD_MASK); >> + if (pmd_val(*pmd) && !pmd_huge(*pmd)) >> + free_page((unsigned long)__va(pmd_val(*pmd))); >> + > > As Marc noted, (assuming the subsequent revert is applied) in both of > these cases, these tables are still live, and thus it is not safe to > free them. > > Consider that immediately after freeing the pages, they may get > re-allocated elsewhere, where they may be modified. If this happens > before TLB invalidation, page table walks may allocate junk into TLBs, > resulting in a number of problems. Ah okay. What about this sequence, 1) I store old PMD/PUD values 2) Update the PMD/PUD with section mapping 3) Invalidate TLB 4) Then free the *leaked* page > > It is *never* safe to free a live page table, therefore NAK to this > patch. > > Thanks, > Mark. > Chintan -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
WARNING: multiple messages have this Message-ID (diff)
From: cpandya@codeaurora.org (Chintan Pandya) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v1 3/4] arm64: Fix the page leak in pud/pmd_set_huge Date: Wed, 14 Mar 2018 16:57:29 +0530 [thread overview] Message-ID: <a3d0adc4-293a-328b-a8ea-ea410baa6849@codeaurora.org> (raw) In-Reply-To: <20180314105343.nxw2mwkm4pao3hur@lakrids.cambridge.arm.com> On 3/14/2018 4:23 PM, Mark Rutland wrote: > On Wed, Mar 14, 2018 at 02:18:24PM +0530, Chintan Pandya wrote: >> While setting huge page, we need to take care of >> previously existing next level mapping. Since, >> we are going to overrite previous mapping, the >> only reference to next level page table will get >> lost and the next level page table will be zombie, >> occupying space forever. So, free it before >> overriding. > >> @@ -939,6 +940,9 @@ int pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot) >> return 0; >> >> BUG_ON(phys & ~PUD_MASK); >> + if (pud_val(*pud) && !pud_huge(*pud)) >> + free_page((unsigned long)__va(pud_val(*pud))); >> + >> set_pud(pudp, pfn_pud(__phys_to_pfn(phys), sect_prot)); >> return 1; >> } >> @@ -953,6 +957,9 @@ int pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot) >> return 0; >> >> BUG_ON(phys & ~PMD_MASK); >> + if (pmd_val(*pmd) && !pmd_huge(*pmd)) >> + free_page((unsigned long)__va(pmd_val(*pmd))); >> + > > As Marc noted, (assuming the subsequent revert is applied) in both of > these cases, these tables are still live, and thus it is not safe to > free them. > > Consider that immediately after freeing the pages, they may get > re-allocated elsewhere, where they may be modified. If this happens > before TLB invalidation, page table walks may allocate junk into TLBs, > resulting in a number of problems. Ah okay. What about this sequence, 1) I store old PMD/PUD values 2) Update the PMD/PUD with section mapping 3) Invalidate TLB 4) Then free the *leaked* page > > It is *never* safe to free a live page table, therefore NAK to this > patch. > > Thanks, > Mark. > Chintan -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
next prev parent reply other threads:[~2018-03-14 11:27 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-14 8:48 [PATCH v1 0/4] Fix issues with huge mapping in ioremap Chintan Pandya 2018-03-14 8:48 ` Chintan Pandya 2018-03-14 8:48 ` [PATCH v1 1/4] asm/tlbflush: Add flush_tlb_pgtable() for ARM64 Chintan Pandya 2018-03-14 8:48 ` Chintan Pandya 2018-03-16 8:26 ` kbuild test robot 2018-03-16 8:26 ` kbuild test robot 2018-03-16 8:26 ` kbuild test robot 2018-03-16 8:26 ` kbuild test robot 2018-03-14 8:48 ` [PATCH v1 2/4] ioremap: Invalidate TLB after huge mappings Chintan Pandya 2018-03-14 8:48 ` Chintan Pandya 2018-03-14 10:48 ` Mark Rutland 2018-03-14 10:48 ` Mark Rutland 2018-03-14 11:20 ` Chintan Pandya 2018-03-14 11:20 ` Chintan Pandya 2018-03-14 11:48 ` Mark Rutland 2018-03-14 11:48 ` Mark Rutland 2018-03-14 8:48 ` [PATCH v1 3/4] arm64: Fix the page leak in pud/pmd_set_huge Chintan Pandya 2018-03-14 8:48 ` Chintan Pandya 2018-03-14 10:35 ` Marc Zyngier 2018-03-14 10:35 ` Marc Zyngier 2018-03-14 10:53 ` Mark Rutland 2018-03-14 10:53 ` Mark Rutland 2018-03-14 11:27 ` Chintan Pandya [this message] 2018-03-14 11:27 ` Chintan Pandya 2018-03-14 11:50 ` Mark Rutland 2018-03-14 11:50 ` Mark Rutland 2018-03-16 14:50 ` kbuild test robot 2018-03-16 14:50 ` kbuild test robot 2018-03-16 14:50 ` kbuild test robot 2018-03-16 14:50 ` kbuild test robot 2018-03-14 8:48 ` [PATCH v1 4/4] Revert "arm64: Enforce BBM for huge IO/VMAP mappings" Chintan Pandya 2018-03-14 8:48 ` Chintan Pandya 2018-03-14 10:46 ` Marc Zyngier 2018-03-14 10:46 ` Marc Zyngier 2018-03-14 11:32 ` Chintan Pandya 2018-03-14 11:32 ` Chintan Pandya 2018-03-14 14:38 ` [PATCH v1 0/4] Fix issues with huge mapping in ioremap Kani, Toshi 2018-03-14 14:38 ` Kani, Toshi 2018-03-15 7:17 ` Chintan Pandya 2018-03-15 7:17 ` Chintan Pandya 2018-03-15 14:38 ` Kani, Toshi 2018-03-15 14:38 ` 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=a3d0adc4-293a-328b-a8ea-ea410baa6849@codeaurora.org \ --to=cpandya@codeaurora.org \ --cc=akpm@linux-foundation.org \ --cc=ard.biesheuvel@linaro.org \ --cc=arnd@arndb.de \ --cc=catalin.marinas@arm.com \ --cc=gregkh@linuxfoundation.org \ --cc=james.morse@arm.com \ --cc=kristina.martsenko@arm.com \ --cc=linux-arch@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=marc.zyngier@arm.com \ --cc=mark.rutland@arm.com \ --cc=takahiro.akashi@linaro.org \ --cc=tglx@linutronix.de \ --cc=toshi.kani@hpe.com \ --cc=will.deacon@arm.com \ /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: linkBe 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.