Re: [RFC PATCH 1/3] LoongArch: Implement CONFIG_THREAD_INFO_IN_TASK

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The default thread flags initialization drops `_TIF_FIXADE`, silently disabling unaligned memory access software fixups by default.
- [High] The `__switch_to` assembly function incorrectly saves the `sched_ra` and `sched_cfa` arguments due to a shifted parameter list, corrupting the saved stack unwinding information.
- [Low] The `reg02` field added to `thread_struct` is dead code, never read or restored, and left uninitialized for kernel threads.
--

commit 40aa85795dd4ee444da3115a1909cec1be7bed8d
Author: Tiezhu Yang <yangtiezhu@loongson.cn>

LoongArch: Implement CONFIG_THREAD_INFO_IN_TASK

This commit selects THREAD_INFO_IN_TASK for LoongArch to move thread_info
off the stack into task_struct. It introduces a per-CPU variable __entry_task
to store the current task_struct pointer and updates the context switch path
and exception entry to use it.

>     Additionally, add reg02 ($tp) to thread_struct and initialize it in
>     both INIT_THREAD and copy_thread() to ensure that the "current" task
>     pointer is correctly set up for the initial idle task and all future
>     child processes. While __switch_to() directly updates $tp from a1 for
>     efficiency, storing it in thread_struct ensures correct restoration
>     for new processes and in non-standard switch paths.

Is this description accurate? Looking at the code, the reg02 field does not
appear to be read during restoration. New threads restore their user tp from
pt_regs, and cpu_restore_nonscratch does not seem to restore it. 

> diff --git a/arch/loongarch/include/asm/processor.h b/arch/loongarch/include/asm/processor.h
> index ce8b953f8c79..df927a4318cc 100644
> --- a/arch/loongarch/include/asm/processor.h
> +++ b/arch/loongarch/include/asm/processor.h
[ ... ]
> @@ -109,7 +109,7 @@ struct loongarch_vdso_info;
>   */
>  struct thread_struct {
>  	/* Main processor registers. */
> -	unsigned long reg01, reg03, reg22; /* ra sp fp */
> +	unsigned long reg01, reg02, reg03, reg22; /* ra tp sp fp */
>  	unsigned long reg23, reg24, reg25, reg26; /* s0-s3 */
>  	unsigned long reg27, reg28, reg29, reg30, reg31; /* s4-s8 */

Could this reg02 field be dead code? If it is never read back out of the
thread_struct, is it needed here?

> diff --git a/arch/loongarch/include/asm/switch_to.h b/arch/loongarch/include/asm/switch_to.h
> index 5b225aff3ba2..9932429cfe17 100644
> --- a/arch/loongarch/include/asm/switch_to.h
> +++ b/arch/loongarch/include/asm/switch_to.h
[ ... ]
> @@ -23,7 +31,7 @@ struct task_struct;
>   * the context of next. Returns prev.
>   */
>  extern asmlinkage struct task_struct *__switch_to(struct task_struct *prev,
> -			struct task_struct *next, struct thread_info *next_ti,
> +			struct task_struct *next,
>  			void *sched_ra, void *sched_cfa);

When the next_ti argument is removed here, sched_ra and sched_cfa shift to 
registers a2 and a3 under the LoongArch C calling convention.

Does the assembly implementation of __switch_to() in switch.S also need to be 
updated? If it still reads from a3 and a4, could this corrupt the saved stack 
unwinding information by storing the stack pointer into THREAD_SCHED_RA
instead? This might break stack unwinding for parked tasks.

> diff --git a/arch/loongarch/include/asm/thread_info.h b/arch/loongarch/include/asm/thread_info.h
> index 4d7117fcdc78..2c95a5134976 100644
> --- a/arch/loongarch/include/asm/thread_info.h
> +++ b/arch/loongarch/include/asm/thread_info.h
[ ... ]
> @@ -37,20 +35,11 @@ struct thread_info {
>   */
>  #define INIT_THREAD_INFO(tsk)			\
>  {						\
> -	.task		= &tsk,			\
> -	.flags		= _TIF_FIXADE,		\
> +	.flags		= 0,			\
>  	.cpu		= 0,			\
>  	.preempt_count	= INIT_PREEMPT_COUNT,	\
>  }

Does removing _TIF_FIXADE here globally disable the unaligned memory access 
software fixups? 

Since all newly created tasks inherit their initial thread info flags from 
init_task, could this cause user-space applications performing unaligned 
accesses to crash with a bus error?

> diff --git a/arch/loongarch/kernel/process.c b/arch/loongarch/kernel/process.c
> index 5505fc355e1b..71c9c6468e60 100644
> --- a/arch/loongarch/kernel/process.c
> +++ b/arch/loongarch/kernel/process.c
[ ... ]
> @@ -223,6 +226,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
>  	if (clone_flags & CLONE_SETTLS)
>  		childregs->regs[2] = tls;
>  
> +	/* Set tp to the new task structure for context switching */
> +	p->thread.reg02 = (unsigned long)p;
> +
>  out:
>  	ptrace_hw_copy_thread(p);

For kernel threads, copy_thread() jumps to the out label before reaching this 
initialization. Does this leave p->thread.reg02 uninitialized for kernel
threads, potentially retaining a stale task pointer from the parent process?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428072021.7289-1-yangtiezhu@loongson.cn?part=1