linux-kernel - Re: [PATCH 3/4] kthreads: rework kthread

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 2 Feb 2009 20:41:05 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Rusty Russell <rusty@...tcorp.com.au>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Christoph Hellwig <hch@....de>, Ingo Molnar <mingo@...e.hu>,
	Pavel Emelyanov <xemul@...nvz.org>,
	Vitaliy Gusev <vgusev@...nvz.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] kthreads: rework kthread_stop()

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.
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.

> 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.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/