Re: + taskstats-copy-signal-stats-under-siglock-in-taskstats_exit.patch added to mm-nonmm-unstable branch

[Oleg Nesterov <oleg@redhat.com>]

On 08/04, Bradley Morgan wrote:
>
> cpu0 (thread A, not last)        cpu1 (thread B, last)
> ===========================      ==============================
> atomic_dec(&signal->live)        atomic_dec(&signal->live) -> 0
> group_dead = 0                   group_dead = 1
> ...                              taskstats_exit(tsk, 1)
> taskstats_exit(tsk, 0)           fill_tgid_exit(tsk) [siglock]
> fill_tgid_exit(tsk)              memcpy(stats, signal->stats)
>   spin_lock(siglock)               reads ac_utime (new)
>   stats->ac_utime += x             reads ac_stime (old)
>   stats->ac_stime += y             torn snapshot -> netlink
>   spin_unlock(siglock)
>
> The listeners receive a partially updated tgid snapshot, with some
> fields from before the concurrent update and some from after. There
> is no crash or splat, which is likely why this went unnoticed since
> 2006. A userspace model of the same shape, writer under a lock and
> a lockless memcpy reader, produces millions of torn reads in a few
> seconds.
>
> Take siglock around the copy like every other access does. sighand
> is still alive here because taskstats_exit() runs before
> exit_notify(), and fill_tgid_exit() already takes this same lock
> earlier in this function.

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.

Oleg.