Re: [to-be-updated] taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch removed from -mm tree

[Oleg Nesterov <oleg@redhat.com>]

On 10/07, Bradley Morgan wrote:
>
> Changes since V1:
> - spin_lock_irq() instead of spin_lock_irqsave() (Oleg)

I have already reviewed this version... Let me quote
https://lore.kernel.org/all/anMC3eRoRdaST0Se@redhat.com/

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

	However. I won't blame your fix, but I am not sure it actually fixes the
	real problem.

	In the scenario above, if thread B takes ->siglock first (before thread A)
	it will report TASKSTATS_TYPE_TGID/AGROUP without accounting thread A.
	Looks "obviously wrong" but I forgot everything about taskstats.

	And OTOH... for_each_thread() in fill_stats_for_tgid() can't rely on the
	tsk->exit_state check, the same thread can be accounted twice.

So perhaps the changelog should mention that this patch doesn't fix all the
related problems? Or may be you can try to make V3 which fixes them all?

> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -635,7 +635,10 @@ void taskstats_exit(struct task_struct *tsk, int group_dead)
>  	if (!stats)
>  		goto err;
>
> +	/* This was racy before, copy the stats under siglock. */

Well, this comment looks strange to me. I'd suggest to drop it.

> +	spin_lock_irq(&tsk->sighand->siglock);
>  	memcpy(stats, tsk->signal->stats, sizeof(*stats));
> +	spin_unlock_irq(&tsk->sighand->siglock);
>  	stats->version = TASKSTATS_VERSION;

Oleg.