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

["Eric W. Biederman" <ebiederm@xmission.com>]

Thomas Gleixner <tglx@kernel.org> 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@kernel.org>
> ---
>  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.

>  /*
>   * 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;
>