[PATCH 2/2] posix-cpu-timers: use ->sighand instead of ->signal to check the task is alive

[PATCH 2/2] posix-cpu-timers: use ->sighand instead of ->signal to check the task is alive

[Oleg Nesterov]

No functional changes.

It doesn't matter which pointer to check under tasklist to ensure the task
was not released, ->signal or ->sighand. But we are going to make ->signal
refcountable, change the code to use ->sighand.

Sadly, it is not trivial to audit kernel/posix-cpu-timers.c, but it really
abuses tasklist_lock. I believe it doesn't need this lock at all, but the
changes are not easy to test.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.29-rc3/kernel/posix-cpu-timers.c~2_CPU_TIMERS	2009-01-29 01:13:55.000000000 +0100
+++ 6.29-rc3/kernel/posix-cpu-timers.c	2009-02-04 00:09:23.000000000 +0100
@@ -297,7 +297,7 @@ int posix_cpu_clock_get(const clockid_t 
 				}
 			} else {
 				read_lock(&tasklist_lock);
-				if (thread_group_leader(p) && p->signal) {
+				if (thread_group_leader(p) && p->sighand) {
 					error =
 					    cpu_clock_sample_group(which_clock,
 							           p, &rtn);
@@ -374,7 +374,7 @@ int posix_cpu_timer_del(struct k_itimer 
 
 	if (likely(p != NULL)) {
 		read_lock(&tasklist_lock);
-		if (unlikely(p->signal == NULL)) {
+		if (unlikely(p->sighand == NULL)) {
 			/*
 			 * We raced with the reaping of the task.
 			 * The deletion should have cleared us off the list.
@@ -640,10 +640,10 @@ int posix_cpu_timer_set(struct k_itimer 
 	read_lock(&tasklist_lock);
 	/*
 	 * We need the tasklist_lock to protect against reaping that
-	 * clears p->signal.  If p has just been reaped, we can no
+	 * clears p->sighand.  If p has just been reaped, we can no
 	 * longer get any information about it at all.
 	 */
-	if (unlikely(p->signal == NULL)) {
+	if (unlikely(p->sighand == NULL)) {
 		read_unlock(&tasklist_lock);
 		put_task_struct(p);
 		timer->it.cpu.task = NULL;
@@ -812,7 +812,7 @@ void posix_cpu_timer_get(struct k_itimer
 		clear_dead = p->exit_state;
 	} else {
 		read_lock(&tasklist_lock);
-		if (unlikely(p->signal == NULL)) {
+		if (unlikely(p->sighand == NULL)) {
 			/*
 			 * The process has been reaped.
 			 * We can't even collect a sample any more.
@@ -1146,7 +1146,7 @@ void posix_cpu_timer_schedule(struct k_i
 		read_lock(&tasklist_lock); /* arm_timer needs it.  */
 	} else {
 		read_lock(&tasklist_lock);
-		if (unlikely(p->signal == NULL)) {
+		if (unlikely(p->sighand == NULL)) {
 			/*
 			 * The process has been reaped.
 			 * We can't even collect a sample any more.

Re: [PATCH 2/2] posix-cpu-timers: use ->sighand instead of ->signal to check the task is alive

[Peter Zijlstra]

On Wed, 2009-02-04 at 00:17 +0100, Oleg Nesterov wrote:
> Sadly, it is not trivial to audit kernel/posix-cpu-timers.c, but it really
> abuses tasklist_lock. I believe it doesn't need this lock at all, but the
> changes are not easy to test.

It uses that to hold of task reaping so ->signal doesn't go away.

If we make ->signal refcountable, and rcu freed along with the tasks I
think we can get away without tasklist_lock.

Re: [PATCH 2/2] posix-cpu-timers: use ->sighand instead of ->signal to check the task is alive

[Oleg Nesterov]

On 02/04, Peter Zijlstra wrote:
>
> On Wed, 2009-02-04 at 00:17 +0100, Oleg Nesterov wrote:
> > Sadly, it is not trivial to audit kernel/posix-cpu-timers.c, but it really
> > abuses tasklist_lock. I believe it doesn't need this lock at all, but the
> > changes are not easy to test.
>
> It uses that to hold of task reaping so ->signal doesn't go away.

Yes sure, but ->siglock alone is enough (this was not true when this code
was written, as far as I know). It is not trivial to remove tasklist
completely, but some places are trivial.

> If we make ->signal refcountable, and rcu freed along with the tasks I
> think we can get away without tasklist_lock.

I think this is possible even without this change (which is good anyway).
But the problem is not only that ->signal can go away. For example,
posix_cpu_timer_set/posix_cpu_timer_schedule should not proceed if the
task was already released, even if it had the valid ->signal.

Oleg.

Re: [PATCH 2/2] posix-cpu-timers: use ->sighand instead of ->signal to check the task is alive

[Roland McGrath]

> It doesn't matter which pointer to check under tasklist to ensure the task
> was not released, ->signal or ->sighand. But we are going to make ->signal
> refcountable, change the code to use ->sighand.

I haven't been following what that's about (signal_struct already has two
atomic counts!).  Uses here protecting cpu_clock_sample_group() e.g., are
around looking at ->signal->foobar, so if ->signal is still there, why not
look at it and be able to get the sample in whatever small window this is?

I don't really understand what this new case might mean though.  Most
things that look at ->signal need to lock it, so access doesn't make any
sense if there is no siglock because ->sighand is clear while ->signal is not.


Thanks,
Roland

Re: [PATCH 2/2] posix-cpu-timers: use ->sighand instead of ->signal to check the task is alive

[Oleg Nesterov]

On 02/04, Roland McGrath wrote:
>
> > It doesn't matter which pointer to check under tasklist to ensure the task
> > was not released, ->signal or ->sighand. But we are going to make ->signal
> > refcountable, change the code to use ->sighand.
>
> I haven't been following what that's about (signal_struct already has two
> atomic counts!).

We can't use them as refcounts. You can't bump ->live or ->count without
breaking group_dead or exec logic. Perhaps we can use ->count, but then
we need other changes. But this has nothing to do with this patch.

The goal is to keep task->signal after release_task(), it will be freed
by __put_task_struct(). This allows a lot of simplifications and we can
move some fields from task_struct to signal_struct.

But first we should change the code which does

	lock(tasklist);
	if (task->signal != NULL)
		/* Great! the task was not released */
		do_something(task);

This patch does not change the current behaviour, but makes possible
to change the "scope" of ->signal without breaking the current code.

> Uses here protecting cpu_clock_sample_group() e.g., are
> around looking at ->signal->foobar, so if ->signal is still there, why not
> look at it and be able to get the sample in whatever small window this is?

What if arm_timer() sees ->signal != NULL, proceeds, and attaches the
timer to the signal_struct of the already dead task? This signal_strcut
will be released with the pending timer.

Even cpu_clock_sample_group() is not safe, unless we add other changes.

> I don't really understand what this new case might mean though.  Most
> things that look at ->signal need to lock it, so access doesn't make any
> sense if there is no siglock because ->sighand is clear while ->signal is not.

For example. __sched_setscheduler() needs to read a single word,
signal->rlim[RLIMIT_RTPRIO].rlim_cur, but it has to lock ->siglock
to access ->signal.

Worse. Please look at __exit_signal()->task_rq_unlock_wait(). This hack
is absolutely stupid and mmust die.


But in any case. Even if we don't need the further changes, do you
agree this patch is correct and doesn't change the behaviour?

Oleg.

Re: [PATCH 2/2] posix-cpu-timers: use ->sighand instead of ->signal to check the task is alive

[Roland McGrath]

> We can't use them as refcounts. You can't bump ->live or ->count without
> breaking group_dead or exec logic. Perhaps we can use ->count, but then
> we need other changes. 

We certainly need to clean up exec anyway.

> But this has nothing to do with this patch.

Agreed.

> The goal is to keep task->signal after release_task(), it will be freed
> by __put_task_struct(). This allows a lot of simplifications and we can
> move some fields from task_struct to signal_struct.

That sounds fine to me in theory, but I still wonder what the story will be
about the use of siglock.

> But first we should change the code which does [...]

I did understand the rationale given the signal_struct lifetime change.

> > Uses here protecting cpu_clock_sample_group() e.g., are
> > around looking at ->signal->foobar, so if ->signal is still there, why not
> > look at it and be able to get the sample in whatever small window this is?
> 
> What if arm_timer() sees ->signal != NULL, proceeds, and attaches the
> timer to the signal_struct of the already dead task? This signal_strcut
> will be released with the pending timer.

Of course.  I distinctly mentioned the read-only uses (sample).

> Even cpu_clock_sample_group() is not safe, unless we add other changes.

Why?  It does no locking and only relies on the signal_struct lifetime.

> But in any case. Even if we don't need the further changes, do you
> agree this patch is correct and doesn't change the behaviour?

Yes.


Thanks,
Roland

Re: [PATCH 2/2] posix-cpu-timers: use ->sighand instead of ->signal to check the task is alive

[Oleg Nesterov]

On 02/05, Roland McGrath wrote:
>
> > We can't use them as refcounts. You can't bump ->live or ->count without
> > breaking group_dead or exec logic. Perhaps we can use ->count, but then
> > we need other changes.
>
> We certainly need to clean up exec anyway.

Agreed.

> > The goal is to keep task->signal after release_task(), it will be freed
> > by __put_task_struct(). This allows a lot of simplifications and we can
> > move some fields from task_struct to signal_struct.
>
> That sounds fine to me in theory, but I still wonder what the story will be
> about the use of siglock.

I think we should change nothing with the usage of siglock for now?

> > But first we should change the code which does [...]
>
> I did understand the rationale given the signal_struct lifetime change.

Ah, sorry for noise then.

> > Even cpu_clock_sample_group() is not safe, unless we add other changes.
>
> Why?  It does no locking and only relies on the signal_struct lifetime.

Yes, I was wrong, thanks. I forgot we should always have a reference
to task_struct anyway.

Oleg.