Re: [PATCH 3/4] kthreads: rework kthread_stop()

[ebiederm@xmission.com (Eric W. Biederman)]

Oleg Nesterov <oleg@redhat.com> writes:

> On 02/02, Eric W. Biederman wrote:
>>
>> Oleg on that note we should not need a barrier at all. We should be
>> able to simply say:
>>
>> cmplp = k->vfork_done;
>> if (cmplp){
>> 	/* if vfork_done is NULL we have passed mm_release */
>> 	kthread = container_of(cmplp, struct kthread, exited);
>> 	kthread->should_stop = 1;
>> 	wake_up_process(k);
>> 	wait_for_completion(&kthread->exited);
>> }
>
> Yes, but the compiler can read ->vfork_done twice, and turn this code
> into
>
> 	cmplp = k->vfork_done;
> 	if (cmplp){
> 		kthread = container_of(k->vfork_done, struct kthread, exited);
> 		...
>
> and when we read k->vfork_done again it can be already NULL.
> Probably we could use ACCESS_ONCE() instead.
>
> Perhaps this barrier() is not needed in practice, but just to be safe.

Certainly.   I definitely see where you are coming from.
And of course all of this only works because a pointer is a word size
so it is read and updated atomically by the compiler.

I wish we had a good idiom we could use to make it clear what we
are doing.  The rcu pointer read code perhaps?

> And in fact I saw the bug report with this code:
>
> 	ac.ac_tty = current->signal->tty ?
> 		old_encode_dev(tty_devnum(current->signal->tty)) : 0;
>
> this code is wrong anyway, but ->tty was read twice. I specially
> asked for .s file because I wasn't able to believe the bug manifests
> itself this way.

Interesting.

>> Thinking of it I wish we had someplace we could store a pointer
>> that would not be cleared so we could remove that whole confusing
>> conditional.  I just looked through task_struct and there doesn't
>> appear to be anything promising.
>>
>> Perhaps we could rename vfork_done mm_done and not clear it in
>> mm_release.
>
> Yes, in that case we don't need the barrier().
>
> I was thinking about changing mm_release() too, but we should clear
> ->vfork_done (or whatever) in exec_mmap() anyway.

Yes.  I realized that just after I wrote that.  So clearing
vfork_done in all cases is a good idea so we don't make get sloppy.

Eric