From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759969AbcJTLKQ (ORCPT ); Thu, 20 Oct 2016 07:10:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56284 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756897AbcJTLKO (ORCPT ); Thu, 20 Oct 2016 07:10:14 -0400 Date: Thu, 20 Oct 2016 13:10:11 +0200 From: Jiri Olsa To: Peter Zijlstra Cc: CAI Qian , Rob Herring , Kan Liang , Greg Kroah-Hartman , linux-kernel , Ingo Molnar Subject: [PATCH] perf: Protect pmu device removal with pmu_bus_running check CONFIG_DEBUG_TEST_DRIVER_REMOVE kernel panic Message-ID: <20161020111011.GA13361@krava> References: <907882571.66590.1476113724660.JavaMail.zimbra@redhat.com> <1219480016.67057.1476113847440.JavaMail.zimbra@redhat.com> <20161010172023.GA7148@kroah.com> <1035662571.647973.1476888331396.JavaMail.zimbra@redhat.com> <20161019191943.GA7951@krava> <20161020053944.GQ3102@twins.programming.kicks-ass.net> <20161020085803.GA31721@krava> <20161020090416.GS3102@twins.programming.kicks-ass.net> <20161020094259.GA14853@krava> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161020094259.GA14853@krava> User-Agent: Mutt/1.7.1 (2016-10-04) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 20 Oct 2016 11:10:14 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 20, 2016 at 11:42:59AM +0200, Jiri Olsa wrote: > On Thu, Oct 20, 2016 at 11:04:16AM +0200, Peter Zijlstra wrote: > > On Thu, Oct 20, 2016 at 10:58:03AM +0200, Jiri Olsa wrote: > > > > > @@ -8869,11 +8869,15 @@ void perf_pmu_unregister(struct pmu *pmu) > > > free_percpu(pmu->pmu_disable_count); > > > if (pmu->type >= PERF_TYPE_MAX) > > > idr_remove(&pmu_idr, pmu->type); > > > - if (pmu->nr_addr_filters) > > > - device_remove_file(pmu->dev, &dev_attr_nr_addr_filters); > > > - device_del(pmu->dev); > > > - put_device(pmu->dev); > > > + mutex_lock(&pmus_lock); > > > + if (pmu_bus_running) { > > > + if (pmu->nr_addr_filters) > > > + device_remove_file(pmu->dev, &dev_attr_nr_addr_filters); > > > + device_del(pmu->dev); > > > + put_device(pmu->dev); > > > + } > > > free_pmu_context(pmu); > > > + mutex_unlock(&pmus_lock); > > > } > > > EXPORT_SYMBOL_GPL(perf_pmu_unregister); > > > > I think that is still racy.. > > > > > > unregister: sysfs_init: > > > > mutex_lock(&pmus_lock); > > list_del_rcu(&pmu->entry); > > mutex_unlock(&pmus_lock); > > > > synchronize_*rcu(); > > > > mutex_lock(&pmus_lock); > > list_for_each_entry(pmu, &pmus, entry) { > > /* add device muck */ > > ah, I thought this part would add the device back.. but it's > already out of the pmu list.. right :-\ attached fix, thanks jirka --- CAI Qian reported crash [1] in uncore device removal related to CONFIG_DEBUG_TEST_DRIVER_REMOVE option. The reason for crash is that perf_pmu_unregister tries to remove pmu device which is not added at this point. We add pmu devices only after pmu_bus is registered which happens in perf_event_sysfs_init init call and sets pmu_bus_running flag. The fix is to get the pmu_bus_running flag state at the point the pmu is taken out of the pmus list and remove the device later only if it's set. [1] https://marc.info/?l=linux-kernel&m=147688837328451 Reported-by: CAI Qian Signed-off-by: Jiri Olsa --- kernel/events/core.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/kernel/events/core.c b/kernel/events/core.c index c6e47e97b33f..a5d2e62faf7e 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -8855,7 +8855,10 @@ EXPORT_SYMBOL_GPL(perf_pmu_register); void perf_pmu_unregister(struct pmu *pmu) { + int remove_device; + mutex_lock(&pmus_lock); + remove_device = pmu_bus_running; list_del_rcu(&pmu->entry); mutex_unlock(&pmus_lock); @@ -8869,10 +8872,12 @@ void perf_pmu_unregister(struct pmu *pmu) free_percpu(pmu->pmu_disable_count); if (pmu->type >= PERF_TYPE_MAX) idr_remove(&pmu_idr, pmu->type); - if (pmu->nr_addr_filters) - device_remove_file(pmu->dev, &dev_attr_nr_addr_filters); - device_del(pmu->dev); - put_device(pmu->dev); + if (remove_device) { + if (pmu->nr_addr_filters) + device_remove_file(pmu->dev, &dev_attr_nr_addr_filters); + device_del(pmu->dev); + put_device(pmu->dev); + } free_pmu_context(pmu); } EXPORT_SYMBOL_GPL(perf_pmu_unregister); -- 2.7.4