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=-5.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no 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 D983EC433E6 for ; Mon, 22 Feb 2021 17:59:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8856864E5C for ; Mon, 22 Feb 2021 17:59:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231895AbhBVR7Q (ORCPT ); Mon, 22 Feb 2021 12:59:16 -0500 Received: from mail.kernel.org ([198.145.29.99]:35544 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230398AbhBVR7L (ORCPT ); Mon, 22 Feb 2021 12:59:11 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id D238064E15; Mon, 22 Feb 2021 17:58:28 +0000 (UTC) Date: Mon, 22 Feb 2021 17:58:26 +0000 From: Catalin Marinas To: Vincenzo Frascino Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com, Andrew Morton , Will Deacon , Dmitry Vyukov , Andrey Ryabinin , Alexander Potapenko , Marco Elver , Evgenii Stepanov , Branislav Rankov , Andrey Konovalov , Lorenzo Pieralisi Subject: Re: [PATCH v13 4/7] arm64: mte: Enable TCO in functions that can read beyond buffer limits Message-ID: <20210222175825.GE19604@arm.com> References: <20210211153353.29094-1-vincenzo.frascino@arm.com> <20210211153353.29094-5-vincenzo.frascino@arm.com> <20210212172128.GE7718@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Feb 22, 2021 at 12:08:07PM +0000, Vincenzo Frascino wrote: > On 2/12/21 5:21 PM, Catalin Marinas wrote: > >> + > >> + /* > >> + * This function is called on each active smp core at boot > >> + * time, hence we do not need to take cpu_hotplug_lock again. > >> + */ > >> + static_branch_enable_cpuslocked(&mte_async_mode); > >> } > > Sorry, I missed the cpuslocked aspect before. Is there any reason you > > need to use this API here? I suggested to add it to the > > mte_enable_kernel_sync() because kasan may at some point do this > > dynamically at run-time, so the boot-time argument doesn't hold. But > > it's also incorrect as this function will be called for hot-plugged > > CPUs as well after boot. > > > > The only reason for static_branch_*_cpuslocked() is if it's called from > > a region that already invoked cpus_read_lock() which I don't think is > > the case here. > > I agree with your analysis on why static_branch_*_cpuslocked() is needed, in > fact cpus_read_lock() takes cpu_hotplug_lock as per comment on top of the line > of code. > > If I try to take that lock when enabling the secondary cores I end up in the > situation below: > > [ 0.283402] smp: Bringing up secondary CPUs ... > .... > [ 5.890963] Call trace: > [ 5.891050] dump_backtrace+0x0/0x19c > [ 5.891212] show_stack+0x18/0x70 > [ 5.891373] dump_stack+0xd0/0x12c > [ 5.891531] dequeue_task_idle+0x28/0x40 > [ 5.891686] __schedule+0x45c/0x6c0 > [ 5.891851] schedule+0x70/0x104 > [ 5.892010] percpu_rwsem_wait+0xe8/0x104 > [ 5.892174] __percpu_down_read+0x5c/0x90 > [ 5.892332] percpu_down_read.constprop.0+0xbc/0xd4 > [ 5.892497] cpus_read_lock+0x10/0x1c > [ 5.892660] static_key_enable+0x18/0x3c > [ 5.892823] mte_enable_kernel_async+0x40/0x70 > [ 5.892988] kasan_init_hw_tags_cpu+0x50/0x60 > [ 5.893144] cpu_enable_mte+0x24/0x70 > [ 5.893304] verify_local_cpu_caps+0x58/0x120 > [ 5.893465] check_local_cpu_capabilities+0x18/0x1f0 > [ 5.893626] secondary_start_kernel+0xe0/0x190 > [ 5.893790] 0x0 > [ 5.893975] bad: scheduling from the idle thread! > [ 5.894065] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G W > 5.11.0-rc7-10587-g22cd50bcfcf-dirty #6 > > and the kernel panics. That's because cpu_hotplug_lock is not a spinlock but a semaphore which implies sleeping. I don't think avoiding taking this semaphore altogether (as in the *_cpuslocked functions) is the correct workaround. The mte_enable_kernel_async() function is called on each secondary CPU but we don't really need to attempt to toggle the static key on each of them as they all have the same configuration. Maybe do something like: if (!static_branch_unlikely(&mte_async_mode))) static_branch_enable(&mte_async_mode); so that it's only set on the boot CPU. The alternative is to use a per-CPU mask/variable instead of static branches but it's probably too expensive for those functions that were meant to improve performance. We'll still have an issue with dynamically switching the async/sync mode at run-time. Luckily kasan doesn't do this now. The problem is that until the last CPU have been switched from async to sync, we can't toggle the static label. When switching from sync to async, we need to do it on the first CPU being switched. So, I think currently we can set the mte_async_mode label to true in mte_enable_kernel_async(), with the 'if' check above. For mte_enable_kernel_sync(), don't bother with setting the key to false but place a WARN_ONCE if the mte_async_mode is true. We can revisit it if kasan ever gains this run-time switch mode. -- Catalin 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=-5.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no 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 15AABC433DB for ; Mon, 22 Feb 2021 17:59:42 +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 BFDEF601FF for ; Mon, 22 Feb 2021 17:59:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BFDEF601FF 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=HskMLl30Xfv3Uyb8ZV9Jf+irI9Mf1Q+sdZ8V4pYKTbc=; b=pqOW+jX2vtQhVUibamHtniOmS yvmXXeY0BlKrmyV5cDk9XXQKjuEzD4PuzkpOzEAD7sy/JK8vaV91cpmSYJ6rTyBgUyBiVSFWMnu3S lqZZIExZ3DhH6qAH4RReEah8PkapRfYLxuKXMKOtXCmahaK94neWb2ZQFeOi4cHzo7ZwWzChErFBR cphpg4fIVFMO1gqDyOyoghRakJBs6pcVak0SZytBOx70PMa1spQU8FFxekUjJCLknINkmupFiv6F8 2KdmxlX4sp2tjTkC9OzmclE8qBJmZUcHEaBRAbbNxYNYZuhi+2QnOWmO+oxVrL9OtM3eO/xshUYAE x/CPAO3ew==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1lEFTf-0008Vk-87; Mon, 22 Feb 2021 17:58:35 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1lEFTc-0008VC-8E for linux-arm-kernel@lists.infradead.org; Mon, 22 Feb 2021 17:58:33 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id D238064E15; Mon, 22 Feb 2021 17:58:28 +0000 (UTC) Date: Mon, 22 Feb 2021 17:58:26 +0000 From: Catalin Marinas To: Vincenzo Frascino Subject: Re: [PATCH v13 4/7] arm64: mte: Enable TCO in functions that can read beyond buffer limits Message-ID: <20210222175825.GE19604@arm.com> References: <20210211153353.29094-1-vincenzo.frascino@arm.com> <20210211153353.29094-5-vincenzo.frascino@arm.com> <20210212172128.GE7718@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210222_125832_463881_8536F76F X-CRM114-Status: GOOD ( 27.91 ) 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: Branislav Rankov , Marco Elver , Lorenzo Pieralisi , Andrey Konovalov , Evgenii Stepanov , linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com, Alexander Potapenko , linux-arm-kernel@lists.infradead.org, Andrey Ryabinin , Andrew Morton , Will Deacon , Dmitry Vyukov 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 Mon, Feb 22, 2021 at 12:08:07PM +0000, Vincenzo Frascino wrote: > On 2/12/21 5:21 PM, Catalin Marinas wrote: > >> + > >> + /* > >> + * This function is called on each active smp core at boot > >> + * time, hence we do not need to take cpu_hotplug_lock again. > >> + */ > >> + static_branch_enable_cpuslocked(&mte_async_mode); > >> } > > Sorry, I missed the cpuslocked aspect before. Is there any reason you > > need to use this API here? I suggested to add it to the > > mte_enable_kernel_sync() because kasan may at some point do this > > dynamically at run-time, so the boot-time argument doesn't hold. But > > it's also incorrect as this function will be called for hot-plugged > > CPUs as well after boot. > > > > The only reason for static_branch_*_cpuslocked() is if it's called from > > a region that already invoked cpus_read_lock() which I don't think is > > the case here. > > I agree with your analysis on why static_branch_*_cpuslocked() is needed, in > fact cpus_read_lock() takes cpu_hotplug_lock as per comment on top of the line > of code. > > If I try to take that lock when enabling the secondary cores I end up in the > situation below: > > [ 0.283402] smp: Bringing up secondary CPUs ... > .... > [ 5.890963] Call trace: > [ 5.891050] dump_backtrace+0x0/0x19c > [ 5.891212] show_stack+0x18/0x70 > [ 5.891373] dump_stack+0xd0/0x12c > [ 5.891531] dequeue_task_idle+0x28/0x40 > [ 5.891686] __schedule+0x45c/0x6c0 > [ 5.891851] schedule+0x70/0x104 > [ 5.892010] percpu_rwsem_wait+0xe8/0x104 > [ 5.892174] __percpu_down_read+0x5c/0x90 > [ 5.892332] percpu_down_read.constprop.0+0xbc/0xd4 > [ 5.892497] cpus_read_lock+0x10/0x1c > [ 5.892660] static_key_enable+0x18/0x3c > [ 5.892823] mte_enable_kernel_async+0x40/0x70 > [ 5.892988] kasan_init_hw_tags_cpu+0x50/0x60 > [ 5.893144] cpu_enable_mte+0x24/0x70 > [ 5.893304] verify_local_cpu_caps+0x58/0x120 > [ 5.893465] check_local_cpu_capabilities+0x18/0x1f0 > [ 5.893626] secondary_start_kernel+0xe0/0x190 > [ 5.893790] 0x0 > [ 5.893975] bad: scheduling from the idle thread! > [ 5.894065] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G W > 5.11.0-rc7-10587-g22cd50bcfcf-dirty #6 > > and the kernel panics. That's because cpu_hotplug_lock is not a spinlock but a semaphore which implies sleeping. I don't think avoiding taking this semaphore altogether (as in the *_cpuslocked functions) is the correct workaround. The mte_enable_kernel_async() function is called on each secondary CPU but we don't really need to attempt to toggle the static key on each of them as they all have the same configuration. Maybe do something like: if (!static_branch_unlikely(&mte_async_mode))) static_branch_enable(&mte_async_mode); so that it's only set on the boot CPU. The alternative is to use a per-CPU mask/variable instead of static branches but it's probably too expensive for those functions that were meant to improve performance. We'll still have an issue with dynamically switching the async/sync mode at run-time. Luckily kasan doesn't do this now. The problem is that until the last CPU have been switched from async to sync, we can't toggle the static label. When switching from sync to async, we need to do it on the first CPU being switched. So, I think currently we can set the mte_async_mode label to true in mte_enable_kernel_async(), with the 'if' check above. For mte_enable_kernel_sync(), don't bother with setting the key to false but place a WARN_ONCE if the mte_async_mode is true. We can revisit it if kasan ever gains this run-time switch mode. -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel