Re: re. Spurious wakeup on a newly created kthread

[Eric W. Biederman]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:

> On Fri, Jun 24, 2022 at 10:00 PM Tejun Heo <tj@xxxxxxxxxx> wrote:
>>
>> So, Petr debugged a NULL deref in workqueue code to a spurious wakeup
>> on a newly created kthread.
>
> What? No. That patch can't be right for several reasons.
>
> What we call "spurious wakeups" exist, but they are about wakeups that
> happen from being on a _previous_ wait-queue, and having already been
> removed from it.
>
> They aren't "really" spurious, they are just asynchronous enough (and
> thus unexpected) that you basically should never have a "sleep on
> wait-queue" without then looping and re-testing the condition.
>
> There is no _truly_ spurious wakeup. You were always woken up for a
> reason, it's just that there are more reasons than the entirely
> obvious ones.
>
> For example, the reason that quoted patch cannot be right is that this
> code pattern:
>
>   while (wait_for_completion_interruptible(&worker->ready_to_start))
>     ;
>
> is not valid kernel code. EVER. There is absolutely no way that can be correct.
>
> Either that code can take a signal, or it cannot. If it can take a
> signal, it had better react to said signal. If it cannot, it must not
> use an interruptble sleep - since now that loop turned into a
> kernel-side busy-loop.
>
> So NAK on this kind of crazy "I don't know what happened, so I'll just
> add *more* bugs to the code" voodoo programming.
>
> And no, we don't "fix" that by then adding a timeout.
>
> Stop this "add random code" thing.
>
> If you cannot be woken up before X happens, then you should:
>
>  - don't go to sleep before X happens
>
>  - don't add yourself to any wait-queues before X happens
>
>  - don't expose your process to others before X happens
>
> The solution is *not* to add some completion with random "wait for
> this before waking".
>
> I think the problem here is much more fundamental: you expect a new
> thread to not wake up until you've told it to.
>
> We do have that infrastructure in the kernel: when  you create a new
> thread, you can do various setup, and the thread won't actually run
> until you do "wake_up_new_task()" on it.
>
> However, that's not how kernel_thread() (or kernel_clone()) works.
> Those will call wake_up_new_task(p) for you, and as such a new kernel
> thread will immediately start running.
>
> So I think the expectations here are entirely wrong.  I think
> create_worker() is fundamentally buggy, in how it does that
>
>         /* start the newly created worker */
>         ..
>         wake_up_process(worker->task);
>
> because that wake_up_process() is already much too late. The process
> got woken up already, because it was created by create_kthread() ->
> kernel_thread() -> kernel_clone, which does that wake_up_new_task()
> and it starts running.
>
> If you want to do thread setup *bnefore* the kernel is running, it
> needs to be done before that wake_up_new_task().
>
> That's very possible. Look at what create_io_thread() does, for
> example: it never calls wake_up_new_process() at all, and leaves that
> to the caller, which has done the extra setup.
>
> Or the kernel_clone_args could have a "init" function that gets called
> before doing the wake_up_new_task() is done. Or a number of other
> solutions.
>
> But no, we're not randomly adding some new completion because people
> were confused and thought they were waking things up when it was
> already awake from before.

Ugh.  The use of kthread_bind_mask is buggy and kthread_bind_mask and
kthread_bind look buggy by definition.  As far as I know the kind of
games that involve wait_task_inactive are only safe in special stop
states which TASK_UNINTERRUPTIBLE is not.

I don't know if we have any current code in the kernel that does this
but I have seen code proposed that for suspend/resume that would wake up
everything not in a special stop state, and have every kernel task
figure out if it should be sleeping or not after such a resume.

This implies that kthread_park needs to be used for this logic to work
at all.  So the only legitimate user of the __kthread_bind and
__kthread_bind_mask appears to be kthread_unpark.



Let me suggest someone create a new variant of kthread_create that takes
all of the parameters the workqueue code wants to set.

Hopefully that will be enough that we can use it to remove the handful
of buggy cases that call kthread_bind_mask and kthread_bind.

Eric