Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [syzbot] WARNING in kthread_is_per_cpu | Date | Tue, 20 Apr 2021 10:43:43 +0100 |
| |
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@arm.com>
| |