On 08/20, Eric W. Biederman wrote: > > Oleg Nesterov <o...@redhat.com> writes: > > >> > >> The patch below also needs CLONE_SIGHAND. You can't meaningfully share > >> signal handlers if you can't represent the pid in the siginfo. pids and > >> signals are too interconnected. > > > > I don't really understand. If we allow to share ->mm (with this patch), > > why it is bad to share sighand_struct->action[] ? This only shares the > > pointers to the code which handles a signal. > > Not the signal queues? I guess it is only CLONE_THREAD that shares the > signal queues between tasks.
Yes, and we should nack CLONE_THREAD anyway. > I believe that sharing just the signal handlers between tasks is also a > problem because while in principle you could distinguish the signals. > In practice it will require at least an extra system call to do so. I still do not think this is a problem. If nothing else they share the code, the fact that they also share the entry point for the signal handler is not relevant, I think. And they share it anyway until the child or parent does sigaction(). But this doesn't matter, we both agree that it would be better to deny CLONE_SIGHAND anyway. > So I am thinking something like the diff below. CLONE_SIGHAND as in > theory you can figure out which task you are in and sort it out, > although I don't expect that to happen in practice. Well, I do not really mind. And I won't argue if you submit this patch. But can't we at least move this CLONE_NEWUSER to copy_process? closer to other clone_flags checks. As for your patch, > + /* Dissallow unshare(CLONE_NEWPID) ; clone(CLONE_NEWPID). > + * That can result in a possibly empty parent pid namespace > + * which makes no sense. > + */ > + if ((clone_flags & CLONE_NEWPID) && > + task_active_pid_ns(current) != current->nsproxy->pid_ns) > + return ERR_PTR(-EINVAL); This looks unneeded... copy_pid_ns() should fail in this case, no? > + if ((clone_flags & (CLONE_THREAD|CLONE_SIGHAND|CLONE_PARENT)) && > ... > + if (clone_flags & (CLONE_THREAD | CLONE_PARENT | CLONE_SIGHAND)) This doesn't really matter, but CLONE_THREAD can be omitted. Still. Can't we make a single check? Like the initial patch I sent, but this one moves the check into copy_process() and checks CLONE_* first. Looks a bit simpler. And more understandable to me but this is subjective. But once again, I won't argue, it's up to you. Oleg. --- x/kernel/fork.c +++ x/kernel/fork.c @@ -1173,12 +1173,13 @@ static struct task_struct *copy_process( return ERR_PTR(-EINVAL); /* - * If the new process will be in a different pid namespace - * don't allow the creation of threads. + * -------------- COMMENT ----------------- */ - if ((clone_flags & (CLONE_VM|CLONE_NEWPID)) && - (task_active_pid_ns(current) != current->nsproxy->pid_ns)) - return ERR_PTR(-EINVAL); + if (clone_flags & (CLONE_SIGHAND | CLONE_PARENT)) { + if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) || + (task_active_pid_ns(current) != current->nsproxy->pid_ns)) + return -EINVAL; + } retval = security_task_create(clone_flags); if (retval) @@ -1575,15 +1576,6 @@ long do_fork(unsigned long clone_flags, long nr; /* - * Do some preliminary argument and permissions checking before we - * actually start allocating stuff - */ - if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) { - if (clone_flags & (CLONE_THREAD|CLONE_PARENT)) - return -EINVAL; - } - - /* * Determine whether and which event to report to ptracer. When * called from kernel_thread or CLONE_UNTRACED is explicitly * requested, no event is reported; otherwise, report if the event -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/