From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ispman.iskranet.ru (ispman.iskranet.ru [62.213.33.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id B44231A0C27 for ; Mon, 2 Feb 2015 16:07:05 +1100 (AEDT) Message-ID: <54CF05F4.3040406@kb.kras.ru> Date: Mon, 02 Feb 2015 12:07:00 +0700 From: Arseny Solokha MIME-Version: 1.0 To: Benjamin Herrenschmidt Subject: Re: [PATCH] powerpc/mm: bail out early when flushing TLB page References: <1422619707-30864-1-git-send-email-asolokha@kb.kras.ru> <1422736026.6200.14.camel@kernel.crashing.org> In-Reply-To: <1422736026.6200.14.camel@kernel.crashing.org> Content-Type: text/plain; charset=utf-8 Cc: Scott Wood , Paul Mackerras , linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , > On Fri, 2015-01-30 at 19:08 +0700, Arseny Solokha wrote: >> MMU_NO_CONTEXT is conditionally defined as 0 or (unsigned int)-1. However, >> in __flush_tlb_page() a corresponding variable is only tested for open >> coded 0, which can cause NULL pointer dereference if `mm' argument was >> legitimately passed as such. >> >> Bail out early in case the first argument is NULL, thus eliminate confusion >> between different values of MMU_NO_CONTEXT and avoid disabling and then >> re-enabling preemption unnecessarily. > > So the comment above isn't quite right... we don't *test* it for open > coded 0, we test it for MMU_NO_CONTEXT, however we *set* it to 0 for > NULL mm. > > This is actually correct... on all except 8xx :-) 0 *is* the PID of the > kernel context, and NULL mm usually means kernel context. > However, it's correct that this function will not deal properly with a > NULL mm for other reasons. It must only be called for user contexts. > > Instead of just returning, I would WARN_ON, because if it's ever called > for a kernel page, then it will not do what's expected and that will > need fixing. Just a silent return isn't right. Does this also hold true for __local_flush_tlb_page()? It's called from local_flush_tlb_page() as follows: __local_flush_tlb_page(vma ? vma->vm_mm : NULL, vmaddr, mmu_get_tsize(mmu_virtual_psize), 0); However, if MMU_NO_CONTEXT is 0 and __local_flush_tlb_page() is called with mm set to NULL, it's effectively a no-op. What else am I missing? Thanks, Arsény > This is different from returning on MMU_NO_CONTEXT, in this case, we > know there's no active TLB entries for the process, and thus nothing to > flush. > > Cheers, > Ben. > >> Signed-off-by: Arseny Solokha >> --- >> arch/powerpc/mm/tlb_nohash.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c >> index f38ea4d..ab0616b 100644 >> --- a/arch/powerpc/mm/tlb_nohash.c >> +++ b/arch/powerpc/mm/tlb_nohash.c >> @@ -284,8 +284,11 @@ void __flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr, >> struct cpumask *cpu_mask; >> unsigned int pid; >> >> + if (unlikely(!mm)) >> + return; >> + >> preempt_disable(); >> - pid = mm ? mm->context.id : 0 > > Here we test mm, if we pass NULL, that means the kernel mm which has PID > 0, which is not MMU_NO_CONTEXT >> ; >> + pid = mm->context.id; >> if (unlikely(pid == MMU_NO_CONTEXT)) >> goto bail; >> cpu_mask = mm_cpumask(mm); From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752305AbbBBFHJ (ORCPT ); Mon, 2 Feb 2015 00:07:09 -0500 Received: from ispman.iskranet.ru ([62.213.33.10]:58183 "EHLO ispman.iskranet.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750795AbbBBFHH (ORCPT ); Mon, 2 Feb 2015 00:07:07 -0500 Message-ID: <54CF05F4.3040406@kb.kras.ru> Date: Mon, 02 Feb 2015 12:07:00 +0700 From: Arseny Solokha User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Benjamin Herrenschmidt CC: Scott Wood , Paul Mackerras , Michael Ellerman , linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] powerpc/mm: bail out early when flushing TLB page References: <1422619707-30864-1-git-send-email-asolokha@kb.kras.ru> <1422736026.6200.14.camel@kernel.crashing.org> In-Reply-To: <1422736026.6200.14.camel@kernel.crashing.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > On Fri, 2015-01-30 at 19:08 +0700, Arseny Solokha wrote: >> MMU_NO_CONTEXT is conditionally defined as 0 or (unsigned int)-1. However, >> in __flush_tlb_page() a corresponding variable is only tested for open >> coded 0, which can cause NULL pointer dereference if `mm' argument was >> legitimately passed as such. >> >> Bail out early in case the first argument is NULL, thus eliminate confusion >> between different values of MMU_NO_CONTEXT and avoid disabling and then >> re-enabling preemption unnecessarily. > > So the comment above isn't quite right... we don't *test* it for open > coded 0, we test it for MMU_NO_CONTEXT, however we *set* it to 0 for > NULL mm. > > This is actually correct... on all except 8xx :-) 0 *is* the PID of the > kernel context, and NULL mm usually means kernel context. > However, it's correct that this function will not deal properly with a > NULL mm for other reasons. It must only be called for user contexts. > > Instead of just returning, I would WARN_ON, because if it's ever called > for a kernel page, then it will not do what's expected and that will > need fixing. Just a silent return isn't right. Does this also hold true for __local_flush_tlb_page()? It's called from local_flush_tlb_page() as follows: __local_flush_tlb_page(vma ? vma->vm_mm : NULL, vmaddr, mmu_get_tsize(mmu_virtual_psize), 0); However, if MMU_NO_CONTEXT is 0 and __local_flush_tlb_page() is called with mm set to NULL, it's effectively a no-op. What else am I missing? Thanks, Arsény > This is different from returning on MMU_NO_CONTEXT, in this case, we > know there's no active TLB entries for the process, and thus nothing to > flush. > > Cheers, > Ben. > >> Signed-off-by: Arseny Solokha >> --- >> arch/powerpc/mm/tlb_nohash.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c >> index f38ea4d..ab0616b 100644 >> --- a/arch/powerpc/mm/tlb_nohash.c >> +++ b/arch/powerpc/mm/tlb_nohash.c >> @@ -284,8 +284,11 @@ void __flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr, >> struct cpumask *cpu_mask; >> unsigned int pid; >> >> + if (unlikely(!mm)) >> + return; >> + >> preempt_disable(); >> - pid = mm ? mm->context.id : 0 > > Here we test mm, if we pass NULL, that means the kernel mm which has PID > 0, which is not MMU_NO_CONTEXT >> ; >> + pid = mm->context.id; >> if (unlikely(pid == MMU_NO_CONTEXT)) >> goto bail; >> cpu_mask = mm_cpumask(mm);