From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4E8F3C433DF for ; Tue, 13 Oct 2020 14:06:44 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DAD66247B6 for ; Tue, 13 Oct 2020 14:06:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="gPVSjCuq" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DAD66247B6 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=R7JlPIOdPHP04WO09keutlc+1T4hhwO0hBwAnMY0jjw=; b=gPVSjCuq0WZisTCc4KRfj39ow s4RQxdVIO/9OiJZa3mPXfzMY9VOkLbZfaRsDx28U/OOZb5d3zPb0piH8cejCADGwEY9Ls1hi9SZgm WRJSYMjeKHjVzHBmalH43b+NjrJH7GsKWQ8WpNksuD1aAUc8J8aKm2kDaaqc2AWnP9dYPDOP1/+hj Bt2W8klsINArfaXgKmYy6opRWzqWn+4ZrHhnytciyviO1FOr10lJPcXcKxQQwlOqhXY4XPXs+8BFu Iul4qLKJvuIisMjXkxwc2oz6502NxRcXwjjqABMiaxjfZW4cNeaTcdI4yHLYuMrHCZKvJinT8VvQp qYNf2tWeQ==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kSKvJ-0002ZF-6L; Tue, 13 Oct 2020 14:05:05 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kSKvH-0002Yk-10 for linux-arm-kernel@lists.infradead.org; Tue, 13 Oct 2020 14:05:04 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0D03831B; Tue, 13 Oct 2020 07:05:01 -0700 (PDT) Received: from bogus (unknown [10.57.16.122]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4702E3F66B; Tue, 13 Oct 2020 07:05:00 -0700 (PDT) Date: Tue, 13 Oct 2020 15:04:57 +0100 From: Sudeep Holla To: Cristian Marussi Subject: Re: [PATCH] firmware: arm_scmi: fix notifications locking Message-ID: <20201013140457.GB23273@bogus> References: <20201013133109.49821-1-cristian.marussi@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201013133109.49821-1-cristian.marussi@arm.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201013_100503_196478_C85C0377 X-CRM114-Status: GOOD ( 25.39 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Sudeep Holla Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Oct 13, 2020 at 02:31:09PM +0100, Cristian Marussi wrote: > When a protocol registers its events the notification core takes care to > re-scan the hashtable of pending event handlers and activate all the > possibly existent handlers that refer to any of the events just registered > by the new protocol; when a pending handler becomes active the core takes > also care to ask the SCMI platform to enable the corresponding events' > notifications in the SCMI firmware. > > If, for whatever reason, the enable fails such invalid event handler must > be finally removed and freed but it must be treated as an active handler > like it has just become: ensure to use the scmi_put_active_handler() helper > which handles properly the needed additional mutexing. > > Failing to properly acquire all the needed mutexes exposes a race that > leads to the following splat being observed: > > [ 212.840876] ------------[ cut here ]------------ Please drop the timestamp next time you have to copy backtrace, it is useless in commit log. > [ 212.845569] refcount_t: underflow; use-after-free. > [ 212.850544] WARNING: CPU: 0 PID: 388 at lib/refcount.c:28 refcount_warn_saturate+0xf8/0x148 > [ 212.858913] Modules linked in: dummy_scmi_consumer(-) scmi_perf [last unloaded: scmi_cpufreq] > [ 212.867478] CPU: 0 PID: 388 Comm: rmmod Tainted: G W 5.9.0-rc1-00020-g9c4395e7867d-dirty #4 > [ 212.877153] Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jun 30 2020 > [ 212.887963] pstate: 40000005 (nZcv daif -PAN -UAO BTYPE=--) > [ 212.893554] pc : refcount_warn_saturate+0xf8/0x148 > [ 212.898361] lr : refcount_warn_saturate+0xf8/0x148 > [ 212.903160] sp : ffff80001298bc00 > [ 212.906480] x29: ffff80001298bc00 x28: ffff00097470aac0 > [ 212.911807] x27: 0000000000000000 x26: 0000000000000000 > [ 212.917135] x25: ffff00097357fc80 x24: 0000000000000002 > [ 212.922462] x23: ffff00097357fca8 x22: 0000000000000003 > [ 212.927790] x21: ffff000974474688 x20: ffff000971e04f00 > [ 212.933117] x19: 0000000000000001 x18: 0000000000000010 > [ 212.938444] x17: 0000000000000000 x16: 0000000000000000 > [ 212.943771] x15: ffffffffffffffff x14: ffff800012269948 > [ 212.949098] x13: ffff80009298b947 x12: ffff80001298b94f > [ 212.954426] x11: 0001000000000000 x10: 0000000000000a10 > [ 212.959753] x9 : ffff80001039fe88 x8 : ffff00097470b530 > [ 212.965080] x7 : 00000000ffffffff x6 : ffff00097ef1b200 > [ 212.970407] x5 : ffff00097ef1b200 x4 : 0000000000000000 > [ 212.975734] x3 : ffff00097ef2a398 x2 : 0000000000000001 > [ 212.981060] x1 : 4b16c3af5d721600 x0 : 0000000000000000 Also trim and remove all the above hex values. I will modify this time, again they are of not much use. > [ 212.986388] Call trace: > [ 212.988847] refcount_warn_saturate+0xf8/0x148 > [ 212.993308] scmi_put_handler_unlocked.isra.10+0x204/0x208 > [ 212.998811] scmi_put_handler+0x50/0xa0 > [ 213.002659] scmi_unregister_notifier+0x1bc/0x240 > [ 213.007385] scmi_notify_tester_remove+0x4c/0x68 [dummy_scmi_consumer] > [ 213.013931] scmi_dev_remove+0x54/0x68 > [ 213.017695] device_release_driver_internal+0x114/0x1e8 > [ 213.022934] driver_detach+0x58/0xe8 > [ 213.026520] bus_remove_driver+0x88/0xe0 > [ 213.030454] driver_unregister+0x38/0x68 > [ 213.034389] scmi_driver_unregister+0x1c/0x28 > [ 213.038763] scmi_drv_exit+0x1c/0xae0 [dummy_scmi_consumer] > [ 213.044354] __arm64_sys_delete_module+0x1a4/0x268 > [ 213.049160] el0_svc_common.constprop.3+0x94/0x178 > [ 213.053963] do_el0_svc+0x2c/0x98 > [ 213.057290] el0_sync_handler+0x148/0x1a8 > [ 213.061310] el0_sync+0x158/0x180 > [ 213.064632] ---[ end trace 5bff2d25d5820911 ]--- > > Fixes: e7c215f358a35 ("firmware: arm_scmi: Add notification callbacks-registration") > Signed-off-by: Cristian Marussi > --- > > Applied and tested on [1] on top of : > > commit: 9724722fde8f ("firmware: arm_scmi: Add missing Rx size > re-initialisation") > > [1]:https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi > --- > --- > drivers/firmware/arm_scmi/notify.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c > index 2754f9d01636..c24e427dce0d 100644 > --- a/drivers/firmware/arm_scmi/notify.c > +++ b/drivers/firmware/arm_scmi/notify.c > @@ -1403,15 +1403,21 @@ static void scmi_protocols_late_init(struct work_struct *work) > "finalized PENDING handler - key:%X\n", > hndl->key); > ret = scmi_event_handler_enable_events(hndl); > + if (ret) { > + dev_dbg(ni->handle->dev, > + "purging INVALID handler - key:%X\n", > + hndl->key); > + scmi_put_active_handler(ni, hndl); > + } > } else { > ret = scmi_valid_pending_handler(ni, hndl); > - } > - if (ret) { > - dev_dbg(ni->handle->dev, > - "purging PENDING handler - key:%X\n", > - hndl->key); > - /* this hndl can be only a pending one */ > - scmi_put_handler_unlocked(ni, hndl); > + if (ret) { > + dev_dbg(ni->handle->dev, > + "purging PENDING handler - key:%X\n", > + hndl->key); > + /* this hndl can be only a pending one */ > + scmi_put_handler_unlocked(ni, hndl); > + } > } > } > mutex_unlock(&ni->pending_mtx); > -- > 2.17.1 > -- Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel