[RFC][PATCH] s390: add arch_change_pid for arch updates after task pid change

[RFC][PATCH] s390: add arch_change_pid for arch updates after task pid change

[Martin Schwidefsky]

The s390 architecture uses the task pid to tag the samples stored
by the sampling facility. The user space tool can use the tag to
distinguish the samples of different tasks. The pid is announced
to the sampling facility with the LPP instruction, the update of
the tag is done in __switch_to().

In a multi-threaded program any thread can call execve(). If this
is not done by the thread group leader, the de_thread() function
replaces the pid of the task that calls execve() with the pid of
thread group leader. If the task reaches user space again without
going over __switch_to() the sampling tag is still set to the old
pid.

Add the arch_change_pid function to re-issue the LPP to set the
new pid for the task in case of a thread ground leader change.

Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/include/asm/thread_info.h |  2 ++
 arch/s390/kernel/process.c          | 10 ++++++++++
 kernel/pid.c                        |  5 +++++
 3 files changed, 17 insertions(+)

diff --git a/arch/s390/include/asm/thread_info.h b/arch/s390/include/asm/thread_info.h
index 83ba575..6f99b2f 100644
--- a/arch/s390/include/asm/thread_info.h
+++ b/arch/s390/include/asm/thread_info.h
@@ -42,6 +42,8 @@ struct thread_info {
 	.flags		= 0,			\
 }
 
+enum pid_type;
+void arch_change_pid(struct task_struct *task, enum pid_type type);
 void arch_release_task_struct(struct task_struct *tsk);
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
 
diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index 70576a2..1a716e7 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -29,6 +29,7 @@
 #include <linux/random.h>
 #include <linux/export.h>
 #include <linux/init_task.h>
+#include <asm/cpu_mf.h>
 #include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/vtimer.h>
@@ -68,6 +69,15 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 	return 0;
 }
 
+void arch_change_pid(struct task_struct *task, enum pid_type type)
+{
+	if (type == PIDTYPE_PID && task == current) {
+		S390_lowcore.current_pid = current->pid;
+		if (test_facility(40))
+			lpp(&S390_lowcore.lpp);
+	}
+}
+
 int copy_thread_tls(unsigned long clone_flags, unsigned long new_stackp,
 		    unsigned long arg, struct task_struct *p, unsigned long tls)
 {
diff --git a/kernel/pid.c b/kernel/pid.c
index 157fe4b..d38ac18 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -299,11 +299,16 @@ void detach_pid(struct task_struct *task, enum pid_type type)
 	__change_pid(task, type, NULL);
 }
 
+void __weak arch_change_pid(struct task_struct *task, enum pid_type type)
+{
+}
+
 void change_pid(struct task_struct *task, enum pid_type type,
 		struct pid *pid)
 {
 	__change_pid(task, type, pid);
 	attach_pid(task, type);
+	arch_change_pid(task, type);
 }
 
 /* transfer_pid is an optimization of attach_pid(new), detach_pid(old) */
-- 
2.7.4

Re: [RFC][PATCH] s390: add arch_change_pid for arch updates after task pid change

[Oleg Nesterov]

On 04/19, Martin Schwidefsky wrote:
>
> In a multi-threaded program any thread can call execve(). If this
> is not done by the thread group leader, the de_thread() function
> replaces the pid of the task that calls execve() with the pid of
> thread group leader. If the task reaches user space again without
> going over __switch_to() the sampling tag is still set to the old
> pid.

If this is the only reason for arch_change_pid() hook, then perhaps
it would be better/simpler to add it into de_thread() right after
change_pid(tsk, PIDTYPE_PID, task_pid(leader)) ?

note also that this way arch_change_pid() doesn't need any checks and
any arguments, you can simply do

void arch_change_pid(void)
{
	S390_lowcore.current_pid = current->pid;
	if (test_facility(40))
		lpp(&S390_lowcore.lpp);
}

Oleg.

Re: [RFC][PATCH] s390: add arch_change_pid for arch updates after task pid change

[Martin Schwidefsky]

On Thu, 19 Apr 2018 16:07:29 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 04/19, Martin Schwidefsky wrote:
> >
> > In a multi-threaded program any thread can call execve(). If this
> > is not done by the thread group leader, the de_thread() function
> > replaces the pid of the task that calls execve() with the pid of
> > thread group leader. If the task reaches user space again without
> > going over __switch_to() the sampling tag is still set to the old
> > pid.  
> 
> If this is the only reason for arch_change_pid() hook, then perhaps
> it would be better/simpler to add it into de_thread() right after
> change_pid(tsk, PIDTYPE_PID, task_pid(leader)) ?
> 
> note also that this way arch_change_pid() doesn't need any checks and
> any arguments, you can simply do
> 
> void arch_change_pid(void)
> {
> 	S390_lowcore.current_pid = current->pid;
> 	if (test_facility(40))
> 		lpp(&S390_lowcore.lpp);
> }

Yeah, the first patch I created to verify that this indeed the problem
basically looked like your proposal. But then why not make it a more
general hook for any kind of PID change? Dunno if this is a worthwhile
approach, the simpler version for sure works as well.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC][PATCH] s390: add arch_change_pid for arch updates after task pid change

[Oleg Nesterov]

On 04/19, Martin Schwidefsky wrote:
>
> On Thu, 19 Apr 2018 16:07:29 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 04/19, Martin Schwidefsky wrote:
> > >
> > > In a multi-threaded program any thread can call execve(). If this
> > > is not done by the thread group leader, the de_thread() function
> > > replaces the pid of the task that calls execve() with the pid of
> > > thread group leader. If the task reaches user space again without
> > > going over __switch_to() the sampling tag is still set to the old
> > > pid.
> >
> > If this is the only reason for arch_change_pid() hook, then perhaps
> > it would be better/simpler to add it into de_thread() right after
> > change_pid(tsk, PIDTYPE_PID, task_pid(leader)) ?
> >
> > note also that this way arch_change_pid() doesn't need any checks and
> > any arguments, you can simply do
> >
> > void arch_change_pid(void)
> > {
> > 	S390_lowcore.current_pid = current->pid;
> > 	if (test_facility(40))
> > 		lpp(&S390_lowcore.lpp);
> > }
>
> Yeah, the first patch I created to verify that this indeed the problem
> basically looked like your proposal. But then why not make it a more
> general hook for any kind of PID change?

Well, I doubt very much we will ever need an arch-specific hook for
sys_setsid() or setpgid()... plus

> Dunno if this is a worthwhile
> approach, the simpler version for sure works as well.

and perhaps you can make another, even more simple change? can't you just
introduce the s390 version of arch_setup_new_exec, something like

	void arch_setup_new_exec(void)
	{
		if (S390_lowcore.current_pid != current->pid) {
			S390_lowcore.current_pid = current->pid;
			...
		}
	}

?

Oleg.

Re: [RFC][PATCH] s390: add arch_change_pid for arch updates after task pid change

[Martin Schwidefsky]

On Thu, 19 Apr 2018 18:08:28 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 04/19, Martin Schwidefsky wrote:
> >
> > On Thu, 19 Apr 2018 16:07:29 +0200
> > Oleg Nesterov <oleg@redhat.com> wrote:
> >  
> > > On 04/19, Martin Schwidefsky wrote:  
> > > >
> > > > In a multi-threaded program any thread can call execve(). If this
> > > > is not done by the thread group leader, the de_thread() function
> > > > replaces the pid of the task that calls execve() with the pid of
> > > > thread group leader. If the task reaches user space again without
> > > > going over __switch_to() the sampling tag is still set to the old
> > > > pid.  
> > >
> > > If this is the only reason for arch_change_pid() hook, then perhaps
> > > it would be better/simpler to add it into de_thread() right after
> > > change_pid(tsk, PIDTYPE_PID, task_pid(leader)) ?
> > >
> > > note also that this way arch_change_pid() doesn't need any checks and
> > > any arguments, you can simply do
> > >
> > > void arch_change_pid(void)
> > > {
> > > 	S390_lowcore.current_pid = current->pid;
> > > 	if (test_facility(40))
> > > 		lpp(&S390_lowcore.lpp);
> > > }  
> >
> > Yeah, the first patch I created to verify that this indeed the problem
> > basically looked like your proposal. But then why not make it a more
> > general hook for any kind of PID change?  
> 
> Well, I doubt very much we will ever need an arch-specific hook for
> sys_setsid() or setpgid()... plus
> 
> > Dunno if this is a worthwhile
> > approach, the simpler version for sure works as well.  
> 
> and perhaps you can make another, even more simple change? can't you just
> introduce the s390 version of arch_setup_new_exec, something like
> 
> 	void arch_setup_new_exec(void)
> 	{
> 		if (S390_lowcore.current_pid != current->pid) {
> 			S390_lowcore.current_pid = current->pid;
> 			...
> 		}
> 	}
> 
> ?

Nice, set_new_exec is called after flush_old_exec/de_thread. Yes, that
should work as well and does not require a new arch hook.

Thanks Oleg!

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.