Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221207103936.2198407-1-ardb@kernel.org> <20221207103936.2198407-2-ardb@kernel.org>
In-Reply-To: <20221207103936.2198407-2-ardb@kernel.org>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Thu, 15 Dec 2022 11:22:46 +0100
Message-ID: <CACRpkdZ6qYG2N=W-zcOutPApJ+r_3xJ5ax5dQBhVjS9hky1trA@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] ARM: vfp: Manipulate VFP state with softirqs disabled
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     linux-arm-kernel@lists.infradead.org, linux@armlinux.org.uk,
        linux-crypto@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Dec 7, 2022 at 11:39 AM Ard Biesheuvel <ardb@kernel.org> wrote:

> In a subsequent patch, we will relax the kernel mode NEON policy, and
> permit kernel mode NEON to be used not only from task context, as is
> permitted today, but also from softirq context.
>
> Given that softirqs may trigger over the back of any IRQ unless they are
> explicitly disabled, we need to address the resulting races in the VFP
> state handling, by disabling softirq processing in two distinct but
> related cases:
> - kernel mode NEON will leave the FPU disabled after it completes, so
>   any kernel code sequence that enables the FPU and subsequently accesses
>   its registers needs to disable softirqs until it completes;
> - kernel_neon_begin() will preserve the userland VFP state in memory,
>   and if it interrupts the ordinary VFP state preserve sequence, the
>   latter will resume execution with the VFP registers corrupted, and
>   happily save them to memory.
>
> Given that disabling softirqs also disables preemption, we can replace
> the existing preempt_disable/enable occurrences in the VFP state
> handling asm code with new macros that dis/enable softirqs instead.
> In the VFP state handling C code, add local_bh_disable/enable() calls
> in those places where the VFP state is preserved.
>
> One thing to keep in mind is that, once we allow NEON use in softirq
> context, the result of any such interruption is that the FPEXC_EN bit in
> the FPEXC register will be cleared, and vfp_current_hw_state[cpu] will
> be NULL. This means that any sequence that [conditionally] clears
> FPEXC_EN and/or sets vfp_current_hw_state[cpu] to NULL does not need to
> run with softirqs disabled, as the result will be the same. Furthermore,
> the handling of THREAD_NOTIFY_SWITCH is guaranteed to run with IRQs
> disabled, and so it does not need protection from softirq interruptions
> either.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Tricky patch, I had to read it a few times and visualize the concepts,
but I am sufficiently convinced that it does the right thing.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij