linux-kernel - RE: [PATCH V3] LoongArch: Provide kernel fpu functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Mar 2023 17:16:58 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Huacai Chen' <chenhuacai@...ngson.cn>,
        Arnd Bergmann <arnd@...db.de>,
        Huacai Chen <chenhuacai@...nel.org>
CC:     "loongarch@...ts.linux.dev" <loongarch@...ts.linux.dev>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        Xuefeng Li <lixuefeng@...ngson.cn>,
        Guo Ren <guoren@...nel.org>, Xuerui Wang <kernel@...0n.name>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "loongson-kernel@...ts.loongnix.cn" 
        <loongson-kernel@...ts.loongnix.cn>
Subject: RE: [PATCH V3] LoongArch: Provide kernel fpu functions

From: Huacai Chen
> Sent: 06 March 2023 10:00
> 
> Provide kernel_fpu_begin()/kernel_fpu_end() to allow the kernel itself
> to use fpu. They can be used by some other kernel components, e.g., the
> AMDGPU graphic driver for DCN.
> 
...
> diff --git a/arch/loongarch/kernel/kfpu.c b/arch/loongarch/kernel/kfpu.c
> new file mode 100644
> index 000000000000..cd2a18fecdcc
> --- /dev/null
> +++ b/arch/loongarch/kernel/kfpu.c
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Loongson Technology Corporation Limited
> + */
> +
> +#include <linux/cpu.h>
> +#include <linux/init.h>
> +#include <asm/fpu.h>
> +#include <asm/smp.h>
> +
> +static DEFINE_PER_CPU(bool, in_kernel_fpu);
> +
> +void kernel_fpu_begin(void)
> +{
> +	if (this_cpu_read(in_kernel_fpu))
> +		return;

Isn't this check entirely broken?
It absolutely needs to be inside the preempt_disable().
If there are nested requests then fpu use is disabled by the first
kernel_fpu_end() call.

> +
> +	preempt_disable();
> +	this_cpu_write(in_kernel_fpu, true);
> +
> +	if (!is_fpu_owner())
> +		enable_fpu();
> +	else
> +		_save_fp(&current->thread.fpu);
> +}

More interestingly, unless the kernel is doing the kind of
'lazy fpu switch' that x86 used to do (not sure it still does in Linux)
where the fpu registers can contain values for a different process
isn't it actually enough for kernel_fpu_begin() to just be:

	preempt_disable();
	if (current->fpu_regs_live)
		__save_fp(current);
	preempt_enable();

and for kernel_fpu_end() to basically be a nop.

Then rely on the 'return to user' path to pick up the
live fpu registers from the save area.

After all, you pretty much don't want to load the fpu regs
every time a process wakes up and goes back to sleep without
returning to user.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)