Re: [patch 7/8] posix-cpu-timers: Prevent enqueueing when PF_EXITING is set

[Eric W. Biederman]

"Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> writes:

2> Thomas Gleixner <tglx@xxxxxxxxxx> writes:
>
>> To prepare for cleaning up POSIX CPU timers in do_exit(), prevent
>> enqueueing POSIX CPU timers on a task which has PF_EXITING set.
>>
>> Queueing a timer on such a task is pointless because the task won't expire
>> the timer anymore.
>>
>> Pretending that the timer is armed allows to keep the POSIX timer mechanism
>> "working" so that the timer stays accessible up to the point where a task
>> is unhashed.
>>
>> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxx>
>> ---
>>  kernel/time/posix-cpu-timers.c |   27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>>
>> --- a/kernel/time/posix-cpu-timers.c
>> +++ b/kernel/time/posix-cpu-timers.c
>> @@ -628,6 +628,7 @@ static int posix_cpu_timer_del(struct k_
>>  	}
>>  
>>  	if (!ret) {
>> +		WARN_ON_ONCE(cpu_timer_queued(&timer->it.cpu));
>>  		put_pid(timer->it.cpu.pid);
>>  		timer->it_status = POSIX_TIMER_DISARMED;
>>  	}
>> @@ -674,6 +675,15 @@ void posix_cpu_timers_exit_group(struct
>>  	cleanup_timers(&tsk->signal->posix_cputimers);
>>  }
>>  
>> +static inline bool task_can_enqueue(struct k_itimer *timer, struct task_struct *p)
>> +{
>> +	if (likely(!(p->flags & PF_EXITING)))
>> +		return true;
>> +
>> +	/* Allow TGID type unless the last thread is on the way out. */
>> +	return clock_pid_type(timer->it_clock) == PIDTYPE_TGID && atomic_read(&p->signal->live);
>
> Couldn't this be?
>
> 	/* Allow TGID type unless the group is on the way out. */
> 	return (clock_pid_type(timer->it_clock) == PIDTYPE_TGID) &&
>         	!(p->signal->flags & SIGNAL_GROUP_EXIT);
>> +}
>
> I don't understand why we would want to re-arm a timer after it has
> been decided the group is dying.
>
> Plush I really don't like the idea of p->signal->live spreading to more
> places.  In the future that has the potential to complicate any changes
> to the group_dead calculation.

Hmm.

Now that I think about it this could be:

static inline bool task_can_enqueue(struct k_itimer *timer, struct task_struct *p)
{
	/* Is the process exiting? */
	if (signal->flags & SIGNAL_GROUP_EXIT)
        	return false;

	/* Is the thread exiting? */
	if ((clock_pid_type(timer->it_clock) == PIDTYPE_PID) &&
	    (p->flags & PF_EXITING))
		return false;

	return true;
}

Unless I am missing something subtle with the parts of shutdown.  As
there is a difference between starting the shutdown, and having reached
do_exit.

But unless there is some subtle reason to start a timer after some
thread has called exit() or after a fatal signal has been received
I suspect stopping as soon as SIGNAL_GROUP_EXIT is set is a good idea.



>>  /*
>>   * Insert the timer on the appropriate list before any timers that
>>   * expire later.  This must be called with the sighand lock held.
>> @@ -684,7 +694,24 @@ static void arm_timer(struct k_itimer *t
>>  	struct cpu_timer *ctmr = &timer->it.cpu;
>>  	u64 newexp = cpu_timer_getexpires(ctmr);
>>  
>> +	lockdep_assert_held(&p->sighand->siglock);
>> +
>>  	timer->it_status = POSIX_TIMER_ARMED;
>> +
>> +	/*
>> +	 * Don't enqueue timers when the task or the group is exiting. That
>> +	 * ensures that timer operations are still succeeding as long as the
>> +	 * tasks are visible, but won't enqueue the timers on the task or
>> +	 * process. They won't expire anyway because run_posix_cpu_timers()
>> +	 * exits early when PF_EXITING is set.
>> +	 *
>> +	 * Enqueue is skipped if PF_EXITING is set when the timer is per task
>> +	 * and when the last thread decremented p::signal::live to zero also for
>> +	 * per process timers.
>> +	 */
>> +	if (unlikely(!task_can_enqueue(timer, p)))
>> +		return;
>> +
>>  	if (!cpu_timer_enqueue(&base->tqhead, ctmr))
>>  		return;
>>  

Eric