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
| ||
|
Date: Mon, 02 Feb 2009 19:25:44 -0800 From: ebiederm@...ssion.com (Eric W. Biederman) To: Oleg Nesterov <oleg@...hat.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() Oleg Nesterov <oleg@...hat.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 -- 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/
Powered by blists - more mailing lists