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.schnei...@arm.com>
