Re: [PATCH] taskstats: fix data-race

[Dmitry Vyukov]

On Sat, Oct 5, 2019 at 1:28 PM Christian Brauner
<christian.brauner@xxxxxxxxxx> wrote:
>
> When assiging and testing taskstats in taskstats
> taskstats_exit() there's a race around writing and reading sig->stats.
>
> cpu0:
> task calls exit()
> do_exit()
>         -> taskstats_exit()
>                 -> taskstats_tgid_alloc()
> The task takes sighand lock and assigns new stats to sig->stats.
>
> cpu1:
> task catches signal
> do_exit()
>         -> taskstats_tgid_alloc()
>                 -> taskstats_exit()
> The tasks reads sig->stats __without__ holding sighand lock seeing
> garbage.
>
> Fix this by taking sighand lock when reading sig->stats.
>
> Reported-by: syzbot+c5d03165a1bd1dead0c1@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> ---
>  kernel/taskstats.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/taskstats.c b/kernel/taskstats.c
> index 13a0f2e6ebc2..58b145234c4a 100644
> --- a/kernel/taskstats.c
> +++ b/kernel/taskstats.c
> @@ -553,26 +553,32 @@ static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
>
>  static struct taskstats *taskstats_tgid_alloc(struct task_struct *tsk)
>  {
> +       int empty;
> +       struct taskstats *stats_new, *stats = NULL;
>         struct signal_struct *sig = tsk->signal;
> -       struct taskstats *stats;
> -
> -       if (sig->stats || thread_group_empty(tsk))
> -               goto ret;
>
>         /* No problem if kmem_cache_zalloc() fails */
> -       stats = kmem_cache_zalloc(taskstats_cache, GFP_KERNEL);
> +       stats_new = kmem_cache_zalloc(taskstats_cache, GFP_KERNEL);

This seems to be over-pessimistic wrt performance b/c:
1. We always allocate the object and free it on every call, even if
the stats are already allocated, whereas currently we don't.
2. We always allocate the object and free it if thread_group_empty,
whereas currently we don't.
3. We do lock/unlock on every call.

I would suggest to fix the double-checked locking properly.
Locking is not the only correct way to synchronize things. Lock-free
synchronization is also possible. It's more tricky, but it can be
correct and it's supported by KCSAN/KTSAN. It just needs to be
properly implemented and expressed. For some cases we may decide to
switch to locking instead, but it needs to be an explicit decision.

We can fix the current code by doing READ_ONCE on sig->stats (which
implies smp_read_barrier_depends since 4.15), and storing to it with
smp_store_release.

> +       empty = thread_group_empty(tsk);
>
>         spin_lock_irq(&tsk->sighand->siglock);
> +       if (sig->stats || empty) {
> +               stats = sig->stats;
> +               spin_unlock_irq(&tsk->sighand->siglock);
> +               goto free_cache;
> +       }
> +
>         if (!sig->stats) {
> -               sig->stats = stats;
> -               stats = NULL;
> +               sig->stats = stats_new;
> +               spin_unlock_irq(&tsk->sighand->siglock);
> +               return stats_new;
>         }
>         spin_unlock_irq(&tsk->sighand->siglock);
>
> -       if (stats)
> -               kmem_cache_free(taskstats_cache, stats);
> -ret:
> -       return sig->stats;
> +free_cache:
> +       kmem_cache_free(taskstats_cache, stats_new);
> +       return stats;
>  }
>
>  /* Send pid data out on exit */
> --
> 2.23.0
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@xxxxxxxxxxxxxxxxx
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20191005112806.13960-1-christian.brauner%40ubuntu.com.