procfs: Do not release pid_ns->proc_mnt too early
in [Kernel]
|
From: Eric W. Biederman on 21 Jun 2010 10:30 Louis Rilling <Louis.Rilling(a)kerlabs.com> writes: > On 21/06/10 5:58 -0700, Eric W. Biederman wrote: >> Louis Rilling <Louis.Rilling(a)kerlabs.com> writes: >> >> > On 18/06/10 18:27 +0200, Oleg Nesterov wrote: >> >> On 06/18, Louis Rilling wrote: >> >> > >> >> > On 17/06/10 23:36 +0200, Oleg Nesterov wrote: >> >> > > On 06/17, Eric W. Biederman wrote: >> >> > > > >> >> > > > The task->children isn't changed until __unhash_process() which runs >> >> > > > after flush_proc_task(). >> >> > > >> >> > > Yes. But this is only the current implementation detail. >> >> > > It would be nice to cleanup the code so that EXIT_DEAD tasks are >> >> > > never sit in ->children list. >> >> > > >> >> > > > So we should be able to come up with >> >> > > > a variant of do_wait() that zap_pid_ns_processes can use that does >> >> > > > what we need. >> >> > > >> >> > > See above... >> >> > > >> >> > > Even if we modify do_wait() or add the new variant, how the caller >> >> > > can wait for EXIT_DEAD tasks? I don't think we want to modify >> >> > > release_task() to do __wake_up_parent() or something similar. >> >> > >> >> > Indeed, I was thinking about calling __wake_up_parent() from release_task() >> >> > once parent->children becomes empty. >> >> > >> >> > Not sure about the performance impact though. Maybe some WAIT_NO_CHILDREN flag >> >> > in parent->signal could limit it. But if EXIT_DEAD children are removed from >> >> > ->children before release_task(), I'm afraid that this becomes impossible. >> >> >> >> Thinking more, even the current do_wait() from zap_pid_ns_processes() >> >> is not really good. Suppose that some none-init thread is ptraced, then >> >> zap_pid_ns_processes() will hange until the tracer does do_wait() or >> >> exits. >> > >> > Is this really a bad thing? If somebody ptraces a task in a pid namespace, that >> > sounds reasonable to have this namespace (and it's init task) pinned. >> >> Louis. Have you seen this problem hit without my setns patch? > > Yes. I hit it with Kerrighed patches. I also have an ugly reproducer on > 2.6.35-rc3 (see attachments). Ugly because I introduced artifical delays > in release_task(). I couldn't trigger the bug without it, probably because the > scheduler is too kind :) > > I'm using memory poisoining (SLAB and DEBUG_SLAB) to make it easy to observe the > bug. > > Example: > # ./proc_flush_task-bug-reproducer 1 > >> >> I'm pretty certain that this hits because there are processes do_wait >> does not wait for, in particular processes in a disjoint process tree. > > Indeed do_wait() misses EXIT_DEAD children. > >> >> So at this point I am really favoring killing the do_wait and making >> this all asynchronous. > > Any idea about how to do it? Some variant of the patches Oleg just recently posted. I'm still not comfortable with the extending the kernel mount to the entire lifetime of the pid_namespace. But it certainly is better than a lot of the alternatives. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 21 Jun 2010 10:50
On 06/21, Louis Rilling wrote: > > On 18/06/10 19:55 +0200, Oleg Nesterov wrote: > > > Yes, this should be fixed, I already realized this. work->func(ns) is > > called when ns is already fixed. > > > > Otherwise, nobody should use ns->proc_mount when ns is already dead/freed. > > I meant the opposite. proc_mnt can be kept mounted somewhere, I think, no. If it is kept mounted vfsmount should be different. > and accesses to it > will likely try to access (freed) pid_ns from it. Not sure, there must be no tasks (and pids) in this ns. By anyway I agree. This needs more thinking and we should do something with sb->s_fs_info. But given that nobody (including me) seem to like this approach - let's forget this patch. Thanks, Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |