linux-kernel - Re: [PATCH] LoongArch: Define the symbol fault as a local label in fpu.S

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]

Message-ID: <a4bd839b-d55e-07e5-67a5-016d77f77112@loongson.cn>
Date:   Tue, 29 Aug 2023 20:40:16 +0800
From:   Tiezhu Yang <yangtiezhu@...ngson.cn>
To:     Xi Ruoyao <xry111@...111.site>, Huacai Chen <chenhuacai@...nel.org>
Cc:     loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org,
        loongson-kernel@...ts.loongnix.cn
Subject: Re: [PATCH] LoongArch: Define the symbol fault as a local label in
 fpu.S



On 08/29/2023 02:45 PM, Xi Ruoyao wrote:
> On Tue, 2023-08-29 at 14:28 +0800, Tiezhu Yang wrote:
>> The initial aim is to silence the following objtool warnings:
>>
>>   arch/loongarch/kernel/fpu.o: warning: objtool: _save_fp_context() falls through to next function fault()
>>   arch/loongarch/kernel/fpu.o: warning: objtool: _restore_fp_context() falls through to next function fault()
>>   arch/loongarch/kernel/fpu.o: warning: objtool: _save_lsx_context() falls through to next function fault()
>>   arch/loongarch/kernel/fpu.o: warning: objtool: _restore_lsx_context() falls through to next function fault()
>>   arch/loongarch/kernel/fpu.o: warning: objtool: _save_lasx_context() falls through to next function fault()
>>   arch/loongarch/kernel/fpu.o: warning: objtool: _restore_lasx_context() falls through to next function fault()
>>
>> Obviously, the symbol fault is not a function, it is just a local label,
>
> Hmm why?  To me this seems a function.  We don't branch to it but store
> its address (a "function pointer") in the extable.
>
> And these warnings do not make any sense to me:
>
> /* snip */
>
>> diff --git a/arch/loongarch/kernel/fpu.S b/arch/loongarch/kernel/fpu.S
>> index b4032de..7defe50 100644
>> --- a/arch/loongarch/kernel/fpu.S
>> +++ b/arch/loongarch/kernel/fpu.S
>> @@ -521,7 +521,7 @@ SYM_FUNC_START(_restore_lasx_context)
>>         jr      ra
>
> _restore_lasx_context returns with this instruction.  How can it fall
> through into fault?
>
>>  SYM_FUNC_END(_restore_lasx_context)
>
>> -SYM_FUNC_START(fault)
>> +SYM_CODE_START_LOCAL(fault)
>>         li.w    a0, -EFAULT                             # failure
>>         jr      ra
>> -SYM_FUNC_END(fault)
>> +SYM_CODE_END(fault)
>

In the current code, SYM_FUNC_END() defines the symbol as SYM_T_FUNC
which is STT_FUNC, the objtool warnings are generated through the
following code.

arch/loongarch/include/asm/linkage.h
#define SYM_FUNC_END(name)				\
	.cfi_endproc;					\
	SYM_END(name, SYM_T_FUNC)

include/linux/linkage.h
/* SYM_T_FUNC -- type used by assembler to mark functions */
#ifndef SYM_T_FUNC
#define SYM_T_FUNC				STT_FUNC
#endif

tools/objtool/include/objtool/check.h
static inline struct symbol *insn_func(struct instruction *insn)
{
	struct symbol *sym = insn->sym;

	if (sym && sym->type != STT_FUNC)
		sym = NULL;

	return sym;
}

tools/objtool/check.c
static int validate_branch(struct objtool_file *file, struct symbol *func,
			   struct instruction *insn, struct insn_state state)
{
...
	while (1) {
		next_insn = next_insn_to_validate(file, insn);

		if (func && insn_func(insn) && func != insn_func(insn)->pfunc) {
			/* Ignore KCFI type preambles, which always fall through */
			if (!strncmp(func->name, "__cfi_", 6) ||
			    !strncmp(func->name, "__pfx_", 6))
				return 0;

			WARN("%s() falls through to next function %s()",
			     func->name, insn_func(insn)->name);
			return 1;
		}
...
}

We can see that the fixup can be a local label in the following code.

arch/loongarch/include/asm/asm-extable.h
#define __ASM_EXTABLE_RAW(insn, fixup, type, data)	\
	.pushsection	__ex_table, "a";		\
	.balign		4;				\
	.long		((insn) - .);			\
	.long		((fixup) - .);			\
	.short		(type);				\
	.short		(data);				\
	.popsection;

	.macro		_asm_extable, insn, fixup
	__ASM_EXTABLE_RAW(\insn, \fixup, EX_TYPE_FIXUP, 0)
	.endm

Like arch/loongarch/lib/*.S does, I prefer the following changes,
if you are ok, I will send v2 later.

diff --git a/arch/loongarch/kernel/fpu.S b/arch/loongarch/kernel/fpu.S
index b4032deb8e3b..3177725ea832 100644
--- a/arch/loongarch/kernel/fpu.S
+++ b/arch/loongarch/kernel/fpu.S
@@ -22,7 +22,7 @@

         .macro  EX insn, reg, src, offs
  .ex\@: \insn   \reg, \src, \offs
-       _asm_extable .ex\@, fault
+       _asm_extable .ex\@, .L_fixup_fpu_fault
         .endm

         .macro sc_save_fp base
@@ -521,7 +521,6 @@ SYM_FUNC_START(_restore_lasx_context)
         jr      ra
  SYM_FUNC_END(_restore_lasx_context)

-SYM_FUNC_START(fault)
+.L_fixup_fpu_fault:
         li.w    a0, -EFAULT                             # failure
         jr      ra
-SYM_FUNC_END(fault)
diff --git a/arch/loongarch/kernel/lbt.S b/arch/loongarch/kernel/lbt.S
index ecf08bbff650..402eb8ec4024 100644
--- a/arch/loongarch/kernel/lbt.S
+++ b/arch/loongarch/kernel/lbt.S
@@ -16,7 +16,7 @@

         .macro  EX insn, reg, src, offs
  .ex\@: \insn   \reg, \src, \offs
-       _asm_extable .ex\@, lbt_fault
+       _asm_extable .ex\@, .L_fixup_lbt_fault
         .endm

  /*
@@ -150,7 +150,6 @@ SYM_FUNC_START(_restore_ftop_context)
         jr              ra
  SYM_FUNC_END(_restore_ftop_context)

-SYM_FUNC_START(lbt_fault)
+.L_fixup_lbt_fault:
         li.w            a0, -EFAULT             # failure
         jr              ra
-SYM_FUNC_END(lbt_fault)

Thanks,
Tiezhu