Re: [PATCH] powerpc: don't try to copy ppc for task with NULL pt_regs

["Nicholas Piggin" <npiggin@gmail.com>]

On Mon Mar 27, 2023 at 8:26 PM AEST, Christophe Leroy wrote:
>
>
> Le 27/03/2023 à 08:36, Nicholas Piggin a écrit :
> > On Mon Mar 27, 2023 at 8:15 AM AEST, Jens Axboe wrote:
> >> Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
> >> from my (arguably very short) checking is not commonly done for other
> >> archs. This is fine, except when PF_IO_WORKER's have been created and
> >> the task does something that causes a coredump to be generated. Then we
> >> get this crash:
> > 
> > Hey Jens,
> > 
> > Thanks for the testing and the patch.
> > 
> > I think your patch would work, but I'd be inclined to give the IO worker
> > a pt_regs so it looks more like other archs and a regular user thread.
> > 
> > Your IO worker bug reminded me to resurrect some copy_thread patches I
> > had and I think they should do that
> > 
> > https://lists.ozlabs.org/pipermail/linuxppc-dev/2023-March/256271.html
> > 
> > I wouldn't ask you to test it until I've at least tried, do you have a
> > test case that triggers this?
>
> I fact, most architectures don't have thread.regs, but rely on something 
> like:
>
> #define task_pt_regs(p) \
> 	((struct pt_regs *)(THREAD_SIZE + task_stack_page(p)) - 1)
>
>
> In powerpc, thread.regs was there because of the regs not being at the 
> same place in the stack depending on which interrupt it was.
>
> However with the two commits below, we now have stable fixed location 
> for the regs, so thread.regs shouldn't be needed anymore:
> - db297c3b07af ("powerpc/32: Don't save thread.regs on interrupt entry")
> - b5cfc9cd7b04 ("powerpc/32: Fix critical and debug interrupts on BOOKE")
>
> But in the meantime, thread.regs started to be used for additional 
> purpose, like knowing if it is a user thread or a kernel thread by using 
> thread.regs nullity.
>
>
> Now that thread.regs doesn't change anymore at each interrupt, it would 
> probably be worth dropping it and falling back to task_pt_regs() as 
> defined on most architecture.
> Knowing whether a thread is a kernel or user thread can for instance be 
> known with PF_KTHREAD flag, so no need of thread.regs for that.

That would be nice if we can define regs that way, I agree. We should
look into doing that.

Thanks,
Nick