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=-6.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS 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 AD49AC4332F for ; Mon, 6 Sep 2021 07:49:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 95EA160F43 for ; Mon, 6 Sep 2021 07:49:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240248AbhIFHuv (ORCPT ); Mon, 6 Sep 2021 03:50:51 -0400 Received: from mail.kernel.org ([198.145.29.99]:36980 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240232AbhIFHuu (ORCPT ); Mon, 6 Sep 2021 03:50:50 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id BFAF260F43 for ; Mon, 6 Sep 2021 07:49:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1630914585; bh=Gc0lprsv7w/okSpJYAt55fjl1xzEXuErDUTeV6+8RZ4=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=JqP03wy8XTp7qDt1TgdOCzYKiph2r9C8s08SL+DZr0/Ssf56d7gjsvNFoitU8FIcK ttUIED0qi0nr1tE6fzWtStAVsAQWfNlJdz4nWl7DqJ88ywAtzWVL1qK6CN8USnVLRS GziyBcDGCB9M8SDb6JErMmJjd8tSrUuqsIcO+MAo+HSkukbC9s9WrvesnwYM1eJguB UBkgBTeOU8H3sWLkx3goAhleGOunGjpstAM0ahnBpny5xrdHeN3YBb24AI6i87J2Nq PPvMTXedZIq/TlgW5ZO3QxzPlD+i0BJ4wWtRHIBlcFQ1AookCd2DfJzU20lfktVpEm UIc8s7yxyCXEQ== Received: by mail-ot1-f54.google.com with SMTP id g66-20020a9d12c8000000b0051aeba607f1so7790950otg.11 for ; Mon, 06 Sep 2021 00:49:45 -0700 (PDT) X-Gm-Message-State: AOAM530LIx8Zisq64dQTEjlfkCWAv/0MPO3WONXKu9zAJdWMd+KkMh4Y s5kOnQyfjcc2ZBODp0Q2LJnKeUizZWLfsXpzAqI= X-Google-Smtp-Source: ABdhPJw2KhJSX56bD1ujc4nIOhS/AujhCUCRQrLrbPPE+SfehRlXCts4tMFpDIOxUFDz2OkohPYafgZKcJSPeDIus0w= X-Received: by 2002:a9d:12e2:: with SMTP id g89mr10172221otg.112.1630914585091; Mon, 06 Sep 2021 00:49:45 -0700 (PDT) MIME-Version: 1.0 References: <20210902155429.3987201-1-keithp@keithp.com> <20210904060908.1310204-1-keithp@keithp.com> <20210904060908.1310204-3-keithp@keithp.com> <8735qifcy6.fsf@keithp.com> In-Reply-To: <8735qifcy6.fsf@keithp.com> From: Ard Biesheuvel Date: Mon, 6 Sep 2021 09:49:33 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 2/3] ARM: Move thread_info into task_struct (v7 only) To: Keith Packard Cc: Linux Kernel Mailing List , Abbott Liu , Alexander Sverdlin , Andrew Morton , Anshuman Khandual , Arnd Bergmann , Bjorn Andersson , Florian Fainelli , Geert Uytterhoeven , Hartley Sweeten , Jens Axboe , Jian Cai , Joe Perches , Kees Cook , Krzysztof Kozlowski , Linus Walleij , Linux ARM , Manivannan Sadhasivam , Marc Zyngier , Masahiro Yamada , Miguel Ojeda , Mike Rapoport , Nathan Chancellor , Nick Desaulniers , Nicolas Pitre , Rob Herring , Russell King , Thomas Gleixner , =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= , Valentin Schneider , Viresh Kumar , "Wolfram Sang (Renesas)" , YiFei Zhu Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 6 Sept 2021 at 08:14, Keith Packard wrote: > > Ard Biesheuvel writes: > > > c13 is not a register, it is a value in one of the dimensions of the > > ARM sysreg space, and probably covers other system registers that > > pre-v7 cores do implement. > > > Better to use its architectural name (TPIDRPRW) and clarify that > > pre-v6k/v7 cores simply don't implement it. > > Thanks, I'll reword the commit message > > > Could we split this up? > > I could split up the assembler macro changes which add a temporary > register to the calls getting the current thread_info/task_struct value? > If that would help get this reviewed, I'd be happy to do > that. Otherwise, it's pretty much all-or-nothing as enabling > THREAD_INFO_IN_TASK requires a bunch of synchronized changes. > Sure, so it is precisely for that reason that it is better to isolate changes that can be isolated. ... > >> +DECLARE_PER_CPU(struct task_struct *, current_task); > >> + > >> +static __always_inline struct task_struct *get_current(void) > >> +{ > >> + return raw_cpu_read(current_task); > > > > This needs to run with preemption disabled, or we might get migrated > > to another CPU halfway through, and produce the wrong result (i.e., > > the current task of the CPU we migrated from). However, it appears > > that manipulating the preempt count will create a circular dependency > > here. > > Yeah, I really don't understand the restrictions of this API well. Any > code fetching the current task pointer better not be subject to > preemption or that value will be stale at some point after it was > computed. Do you know if it could ever be run in a context allowing > preemption? All the time. 'current' essentially never changes value from the POV of code running in task context, so there is usually no reason to care about preemption/migration when referring to it. Using per-CPU variables is what creates the problem here. > > > > Instead of using a per-CPU variable for current, I think it might be > > better to borrow another system register (TPIDRURO or TPIDRURW) to > > carry 'current' when THREAD_INFO_IN_TASK is in effect, similar to how > > arm64 uses SP_EL0 - those registers could be preserved/restored in the > > entry/exit from/to user space paths rather than in the context switch > > path, and then be used any way we like while running in the kernel. > > Making sure these values don't leak through to user space somehow? I'm > not excited by that prospect at all. Moving the code that pokes the right user space value into these registers from the context switch patch to the user space exit path should be rather straight-forward, so I am not too concerned about security issues here (famous last words) > But, perhaps someone can help me > understand the conditions under which the current task value can be > computed where preemption was enabled and have that not be a problem for > the enclosing code? > In principle, it should be sufficient to run the per-CPU variable load sequence with preemption disabled. For instance, your asm version movw \dst, #:lower16:\sym movt \dst, #:upper16:\sym this_cpu_offset \tmp ldr \dst, [\dst, \tmp] must not be preempted and migrated right before the ldr instruction, because that would end up accessing another CPU's value for 'current'. However, the preempt_count itself is stored in thread_info as well, so I'm not sure there is a way we can safely disable preemption for this sequence to begin with, unless we run the above with interrupts disabled. Given that we are already relying on the MP extensions for this anyway, I personally think that using another thread ID register to carry 'current' is a reasonable approach as well, since it would also allow us to get per-task stack protector support into the compiler. But I would like to hear from some other folks on cc as well. 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=-4.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 49CE1C433EF for ; Mon, 6 Sep 2021 07:53:08 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 098AB60200 for ; Mon, 6 Sep 2021 07:53:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 098AB60200 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=r7cOlxUxGGwZbRa0Q6UudD4EsWgiDnhvgJkRYNgPlg4=; b=G0WId7O0hN9JAD F34V872f3I2MOAYwOUi7ufIM1pp+E6K9Ukm51L8prsPj/xuOxRyzYaJl7ERpfv+Kusx9WTelHcQqt mnOA8FxpZEDjc4QYkyOVp7++70xqubGD2yEeoIfqeW88V0yfQaffh3n+sLBL0YtC2yfMZnb5r9/mz +rRY+GdxED7gHy3lMGhyPDt4dKreyovPGSnfcB6RecTcYcaftkki1tdzd4z0INaN7PjxsxETI7nVC RaF6ttIdVYlsn9/mkQGit6OJwf4xZMroSnBCihDf86zxcXqdELL+tnyo5SbTadsithNEqICjVGHRH 7s8ic5OkQTGQNwQRlXFA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mN9O1-0006O4-U9; Mon, 06 Sep 2021 07:49:50 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mN9Ny-0006NH-DZ for linux-arm-kernel@lists.infradead.org; Mon, 06 Sep 2021 07:49:47 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id 011CA6108E for ; Mon, 6 Sep 2021 07:49:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1630914586; bh=Gc0lprsv7w/okSpJYAt55fjl1xzEXuErDUTeV6+8RZ4=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=q54BRWXDnc86P5/wtO+TfKnpMpsPR2yFx3/pgmmfmN+7JkjiOs6CH4ucbM4VWCtiN L+btOPcMLSFrDpigMDDLRqf4KdQSv1oQL5KOVghbUXpq0BqPu3x7JfOlI8ttEAVozm CMji4eBvN2DDN1F2kOLnE7fQXXsHwZZnR2Mew8LzXF2/ual34K0T2yzMIynFYI/Z84 nBPgVkSOL9okXTp6LBjwz+t6Ob4uRX76Vm2LjWJb6nSLMnhQA4osWAHvB1gteKvKzX 4OlV+WQV/SKMy27Oklaq28Na3QbU0jUA2DGBlAXVSn3ZNZma/SxWwnGHm4gpmeyr6h yIaMWUVWRyB2Q== Received: by mail-ot1-f49.google.com with SMTP id i3-20020a056830210300b0051af5666070so7825765otc.4 for ; Mon, 06 Sep 2021 00:49:45 -0700 (PDT) X-Gm-Message-State: AOAM53044LW7HhRIRpzW1dc9DPs9/ZMPL7udbjmbIoqUfhgcAkhzNZ7H 4HBoeHqJH3Wn+z4LBz0IPl/6quIhimr7LzpP88o= X-Google-Smtp-Source: ABdhPJw2KhJSX56bD1ujc4nIOhS/AujhCUCRQrLrbPPE+SfehRlXCts4tMFpDIOxUFDz2OkohPYafgZKcJSPeDIus0w= X-Received: by 2002:a9d:12e2:: with SMTP id g89mr10172221otg.112.1630914585091; Mon, 06 Sep 2021 00:49:45 -0700 (PDT) MIME-Version: 1.0 References: <20210902155429.3987201-1-keithp@keithp.com> <20210904060908.1310204-1-keithp@keithp.com> <20210904060908.1310204-3-keithp@keithp.com> <8735qifcy6.fsf@keithp.com> In-Reply-To: <8735qifcy6.fsf@keithp.com> From: Ard Biesheuvel Date: Mon, 6 Sep 2021 09:49:33 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 2/3] ARM: Move thread_info into task_struct (v7 only) To: Keith Packard Cc: Linux Kernel Mailing List , Abbott Liu , Alexander Sverdlin , Andrew Morton , Anshuman Khandual , Arnd Bergmann , Bjorn Andersson , Florian Fainelli , Geert Uytterhoeven , Hartley Sweeten , Jens Axboe , Jian Cai , Joe Perches , Kees Cook , Krzysztof Kozlowski , Linus Walleij , Linux ARM , Manivannan Sadhasivam , Marc Zyngier , Masahiro Yamada , Miguel Ojeda , Mike Rapoport , Nathan Chancellor , Nick Desaulniers , Nicolas Pitre , Rob Herring , Russell King , Thomas Gleixner , =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= , Valentin Schneider , Viresh Kumar , "Wolfram Sang (Renesas)" , YiFei Zhu X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210906_004946_546054_94372B40 X-CRM114-Status: GOOD ( 44.73 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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, 6 Sept 2021 at 08:14, Keith Packard wrote: > > Ard Biesheuvel writes: > > > c13 is not a register, it is a value in one of the dimensions of the > > ARM sysreg space, and probably covers other system registers that > > pre-v7 cores do implement. > > > Better to use its architectural name (TPIDRPRW) and clarify that > > pre-v6k/v7 cores simply don't implement it. > > Thanks, I'll reword the commit message > > > Could we split this up? > > I could split up the assembler macro changes which add a temporary > register to the calls getting the current thread_info/task_struct value? > If that would help get this reviewed, I'd be happy to do > that. Otherwise, it's pretty much all-or-nothing as enabling > THREAD_INFO_IN_TASK requires a bunch of synchronized changes. > Sure, so it is precisely for that reason that it is better to isolate changes that can be isolated. ... > >> +DECLARE_PER_CPU(struct task_struct *, current_task); > >> + > >> +static __always_inline struct task_struct *get_current(void) > >> +{ > >> + return raw_cpu_read(current_task); > > > > This needs to run with preemption disabled, or we might get migrated > > to another CPU halfway through, and produce the wrong result (i.e., > > the current task of the CPU we migrated from). However, it appears > > that manipulating the preempt count will create a circular dependency > > here. > > Yeah, I really don't understand the restrictions of this API well. Any > code fetching the current task pointer better not be subject to > preemption or that value will be stale at some point after it was > computed. Do you know if it could ever be run in a context allowing > preemption? All the time. 'current' essentially never changes value from the POV of code running in task context, so there is usually no reason to care about preemption/migration when referring to it. Using per-CPU variables is what creates the problem here. > > > > Instead of using a per-CPU variable for current, I think it might be > > better to borrow another system register (TPIDRURO or TPIDRURW) to > > carry 'current' when THREAD_INFO_IN_TASK is in effect, similar to how > > arm64 uses SP_EL0 - those registers could be preserved/restored in the > > entry/exit from/to user space paths rather than in the context switch > > path, and then be used any way we like while running in the kernel. > > Making sure these values don't leak through to user space somehow? I'm > not excited by that prospect at all. Moving the code that pokes the right user space value into these registers from the context switch patch to the user space exit path should be rather straight-forward, so I am not too concerned about security issues here (famous last words) > But, perhaps someone can help me > understand the conditions under which the current task value can be > computed where preemption was enabled and have that not be a problem for > the enclosing code? > In principle, it should be sufficient to run the per-CPU variable load sequence with preemption disabled. For instance, your asm version movw \dst, #:lower16:\sym movt \dst, #:upper16:\sym this_cpu_offset \tmp ldr \dst, [\dst, \tmp] must not be preempted and migrated right before the ldr instruction, because that would end up accessing another CPU's value for 'current'. However, the preempt_count itself is stored in thread_info as well, so I'm not sure there is a way we can safely disable preemption for this sequence to begin with, unless we run the above with interrupts disabled. Given that we are already relying on the MP extensions for this anyway, I personally think that using another thread ID register to carry 'current' is a reasonable approach as well, since it would also allow us to get per-task stack protector support into the compiler. But I would like to hear from some other folks on cc as well. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel