Re: [syzbot] WARNING in kthread_is_per_cpu

[Valentin Schneider]

On 20/04/21 10:51, Peter Zijlstra wrote:
> On Mon, Apr 19, 2021 at 08:58:26PM +0100, Valentin Schneider wrote:
>
>> Looks about right, IIUC the key being:
>>
>>   p->flags & PF_KTHREAD + p->set_child_tid => the struct kthread is
>>   persistent
>>
>>   p->flags & PF_KTHREAD => you may or may not have a struct kthread (see
>>   kernel/umh.c kernel_thread() uses). PF_KTHREAD isn't even guaranteed to
>>   persist (begin_new_exec()), which seems to be what the syzbot hit.
>
> Ack, that's nicely put.
>
>> While we're at it, does free_kthread_struct() want the __to_kthread()
>> treatment as well? The other to_kthread() callsites looked like they only
>> made sense with a "proper" kthread anyway.
>
> I think free_kthread_struct() is ok, because a task at that point in its
> lifetime cannot be also doing exec().
>

What if it's one of those kthreads created by directly invoking
kernel_thread()? AFAICT right now it's only umh, and that one does execve()
so it ends up stripped of PF_KTHREAD. It could however go through an error
path, i.e. not call exec, and exit, giving us:

  put_task_struct(p)
  `\
    free_task(p)
    `\
      if (tsk->flags & PF_KTHREAD)
          free_kthread_struct(tsk);
          `\
            to_kthread(p)

> kthread_func() is another 'fun' trainwreck waiting to happen -- luckily
> the only caller uses current, still let me go fix it.
>
> kthread_probe_data() relies on PF_WQ_WORKER implying PF_KTHREAD but
> otherwise seems very fragile too.
>
> Something like so then?
>

Other than the above:

Reviewed-by: Valentin Schneider <Valentin.Schneider@xxxxxxx>