From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754791AbcJEQPH (ORCPT ); Wed, 5 Oct 2016 12:15:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57442 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754336AbcJEQPG (ORCPT ); Wed, 5 Oct 2016 12:15:06 -0400 Date: Wed, 5 Oct 2016 18:14:55 +0200 From: Jiri Olsa To: Thomas Gleixner Cc: Prarit Bhargava , linux-kernel@vger.kernel.org, Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Peter Zijlstra , Len Brown , Borislav Petkov , Andi Kleen , Juergen Gross , dyoung@redhat.com, Eric Biederman , kexec@lists.infradead.org Subject: Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs Message-ID: <20161005161455.GA26029@krava> References: <1475514432-27682-1-git-send-email-prarit@redhat.com> <57F39C09.10001@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.0 (2016-08-17) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Wed, 05 Oct 2016 16:15:00 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 04, 2016 at 04:38:16PM +0200, Thomas Gleixner wrote: > On Tue, 4 Oct 2016, Prarit Bhargava wrote: > > On 10/04/2016 06:58 AM, Thomas Gleixner wrote: > > > While it is the right thing to initialize the package map in that case, it > > > still papers over a robustness issue in the uncore code, which needs to be > > > fixed first. > > > > I will include a separate patch with an error check for pkg == 0xffff in the > > uncore code. > > 0xffff? That won't help. The id returned is -1 if the entry is not > initialized. And aside of that just patching that particular place is not > helping as the uncore code and also rapl is relying on the package map > being populated. > > So we need a sanity check in the initialization code which prevents any of > this being executed. I still need to test this, but how about something like this? thanks, jirka --- diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c index 28865938aadf..61d087a2f25d 100644 --- a/arch/x86/events/intel/rapl.c +++ b/arch/x86/events/intel/rapl.c @@ -598,8 +598,13 @@ static int rapl_cpu_online(unsigned int cpu) static int rapl_cpu_prepare(unsigned int cpu) { - struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu); + struct rapl_pmu *pmu; + int pkg = topology_logical_package_id(cpu); + + if (WARN_ON(pkg == -1)) + return -EINVAL; + pmu = cpu_to_rapl_pmu(cpu); if (pmu) return 0; @@ -613,7 +618,7 @@ static int rapl_cpu_prepare(unsigned int cpu) pmu->timer_interval = ms_to_ktime(rapl_timer_ms); pmu->cpu = -1; rapl_hrtimer_init(pmu); - rapl_pmus->pmus[topology_logical_package_id(cpu)] = pmu; + rapl_pmus->pmus[pkg] = pmu; return 0; } diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c index 463dc7a5a6c3..3f657fbbf787 100644 --- a/arch/x86/events/intel/uncore.c +++ b/arch/x86/events/intel/uncore.c @@ -1352,7 +1352,7 @@ static int __init intel_uncore_init(void) { const struct x86_cpu_id *id; struct intel_uncore_init_fun *uncore_init; - int pret = 0, cret = 0, ret; + int pret = 0, cret = 0, ret, cpu; id = x86_match_cpu(intel_uncore_match); if (!id) @@ -1363,6 +1363,13 @@ static int __init intel_uncore_init(void) max_packages = topology_max_packages(); + for_each_present_cpu(cpu) { + int pkg = topology_logical_package_id(cpu); + + if (WARN_ON(pkg == -1)) + return -EINVAL; + } + uncore_init = (struct intel_uncore_init_fun *)id->driver_data; if (uncore_init->pci_init) { pret = uncore_init->pci_init(); From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx1.redhat.com ([209.132.183.28]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1broqn-0004df-ME for kexec@lists.infradead.org; Wed, 05 Oct 2016 16:15:22 +0000 Date: Wed, 5 Oct 2016 18:14:55 +0200 From: Jiri Olsa Subject: Re: [PATCH] arch/x86: Fix kdump on x86 with physically hotadded CPUs Message-ID: <20161005161455.GA26029@krava> References: <1475514432-27682-1-git-send-email-prarit@redhat.com> <57F39C09.10001@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: Thomas Gleixner Cc: Prarit Bhargava , Len Brown , Andi Kleen , Juergen Gross , Peter Zijlstra , x86@kernel.org, kexec@lists.infradead.org, linux-kernel@vger.kernel.org, Ingo Molnar , Eric Biederman , "H. Peter Anvin" , Borislav Petkov , dyoung@redhat.com On Tue, Oct 04, 2016 at 04:38:16PM +0200, Thomas Gleixner wrote: > On Tue, 4 Oct 2016, Prarit Bhargava wrote: > > On 10/04/2016 06:58 AM, Thomas Gleixner wrote: > > > While it is the right thing to initialize the package map in that case, it > > > still papers over a robustness issue in the uncore code, which needs to be > > > fixed first. > > > > I will include a separate patch with an error check for pkg == 0xffff in the > > uncore code. > > 0xffff? That won't help. The id returned is -1 if the entry is not > initialized. And aside of that just patching that particular place is not > helping as the uncore code and also rapl is relying on the package map > being populated. > > So we need a sanity check in the initialization code which prevents any of > this being executed. I still need to test this, but how about something like this? thanks, jirka --- diff --git a/arch/x86/events/intel/rapl.c b/arch/x86/events/intel/rapl.c index 28865938aadf..61d087a2f25d 100644 --- a/arch/x86/events/intel/rapl.c +++ b/arch/x86/events/intel/rapl.c @@ -598,8 +598,13 @@ static int rapl_cpu_online(unsigned int cpu) static int rapl_cpu_prepare(unsigned int cpu) { - struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu); + struct rapl_pmu *pmu; + int pkg = topology_logical_package_id(cpu); + + if (WARN_ON(pkg == -1)) + return -EINVAL; + pmu = cpu_to_rapl_pmu(cpu); if (pmu) return 0; @@ -613,7 +618,7 @@ static int rapl_cpu_prepare(unsigned int cpu) pmu->timer_interval = ms_to_ktime(rapl_timer_ms); pmu->cpu = -1; rapl_hrtimer_init(pmu); - rapl_pmus->pmus[topology_logical_package_id(cpu)] = pmu; + rapl_pmus->pmus[pkg] = pmu; return 0; } diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c index 463dc7a5a6c3..3f657fbbf787 100644 --- a/arch/x86/events/intel/uncore.c +++ b/arch/x86/events/intel/uncore.c @@ -1352,7 +1352,7 @@ static int __init intel_uncore_init(void) { const struct x86_cpu_id *id; struct intel_uncore_init_fun *uncore_init; - int pret = 0, cret = 0, ret; + int pret = 0, cret = 0, ret, cpu; id = x86_match_cpu(intel_uncore_match); if (!id) @@ -1363,6 +1363,13 @@ static int __init intel_uncore_init(void) max_packages = topology_max_packages(); + for_each_present_cpu(cpu) { + int pkg = topology_logical_package_id(cpu); + + if (WARN_ON(pkg == -1)) + return -EINVAL; + } + uncore_init = (struct intel_uncore_init_fun *)id->driver_data; if (uncore_init->pci_init) { pret = uncore_init->pci_init(); _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec