Re: [PATCH v5 3/9] powerpc: Prepare for moving thread_info into task_struct

[Christophe Leroy]

On 10/06/2018 12:40 PM, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@xxxxxx> writes:
>
> > diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
> > index 47a03b9b528b..818451bf629c 100644
> > --- a/arch/powerpc/include/asm/livepatch.h
> > +++ b/arch/powerpc/include/asm/livepatch.h
> > @@ -49,7 +49,7 @@ static inline void klp_init_thread_info(struct thread_info *ti)
> >   	ti->livepatch_sp = (unsigned long *)(ti + 1) + 1;
>
> We need to do something about this.

Oops I missed that one.

> Currently the thread_info sits at the low address of the stack, and we
> use the space immediately above that as a miniature upward growing stack
> for livepatching.
>
> If we keep the livepatch_sp in the thread_info then we need to
> initialise it somewhere when the task starts running on a stack. And I
> don't know how that works if we end up running on the emergency stack
> for example.
>
> So I'm not sure that makes much sense.
>
> Instead we might need to keep the livepatch_sp on the stack page at the
> base, where thread_info currently lives.
>
> That obviously sucks because you can still overflow into it and trash
> it, but it's no worse than what we have now for livepatching.
>
> Need to think about it some more.

I think for that serie we can leave with it in the stack, as it won't be 
worst than before. Then in a future patch we can change it. I'll open an 
issue for that on github.

Then for now, I'll add the following change in this patch.

diff --git a/arch/powerpc/include/asm/livepatch.h 
b/arch/powerpc/include/asm/livepatch.h
index 47a03b9b528b..8a81d10ccc82 100644
--- a/arch/powerpc/include/asm/livepatch.h
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -43,13 +43,14 @@ static inline unsigned long 
klp_get_ftrace_location(unsigned long faddr)
 	return ftrace_location_range(faddr, faddr + 16);
 }

-static inline void klp_init_thread_info(struct thread_info *ti)
+static inline void klp_init_thread_info(struct task_struct *p)
 {
+	struct thread_info *ti = task_thread_info(p);
 	/* + 1 to account for STACK_END_MAGIC */
-	ti->livepatch_sp = (unsigned long *)(ti + 1) + 1;
+	ti->livepatch_sp = end_of_stack(p) + 1;
 }
 #else
-static void klp_init_thread_info(struct thread_info *ti) { }
+static inline void klp_init_thread_info(struct task_struct *p) { }
 #endif /* CONFIG_LIVEPATCH */

 #endif /* _ASM_POWERPC_LIVEPATCH_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index d9d4eb2ea6c9..a12307ebb7ef 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1632,7 +1632,7 @@ int copy_thread(unsigned long clone_flags, 
unsigned long usp,
 	unsigned long sp = (unsigned long)task_stack_page(p) + THREAD_SIZE;
 	struct thread_info *ti = task_thread_info(p);

-	klp_init_thread_info(ti);
+	klp_init_thread_info(p);

 	/* Copy registers */
 	sp -= sizeof(struct pt_regs);
diff --git a/arch/powerpc/kernel/setup-common.c 
b/arch/powerpc/kernel/setup-common.c
index 9ca9db707bcb..8054a7b9e026 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -940,7 +940,7 @@ void __init setup_arch(char **cmdline_p)
 	/* Reserve large chunks of memory for use by CMA for KVM. */
 	kvm_cma_reserve();

-	klp_init_thread_info(&init_thread_info);
+	klp_init_thread_info(&init_task);

 	init_mm.start_code = (unsigned long)_stext;
 	init_mm.end_code = (unsigned long) _etext;

Christophe