namespaces && fork fixes/cleanups
Subsystems (3)
cover namespaces && fork fixes/cleanups
Hello.
The discussion was a bit confusing. I decided to split the changes to simplify the documentation/review.
Eric, Andy, could you please ack/nack explicitly?
I guess it is too late for 3.11, hopefully Andrew can take these patches if they pass the review.
OTOH, 1/3 fixes the regression, so perhaps it should cc -stable.
Oleg.
kernel/fork.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-)
PATCH 1/3 pidns: fix vfork() after unshare(CLONE_NEWPID)
8382fcac "pidns: Outlaw thread creation after unshare(CLONE_NEWPID)" nacks CLONE_VM if the forking process unshared pid_ns, this obviously breaks vfork:
int main(void)
{
assert(unshare(CLONE_NEWUSER | CLONE_NEWPID) == 0);
assert(vfork() >= 0);
_exit(0);
return 0;
}fails without this patch.
Change this check to use CLONE_SIGHAND instead. This also forbids CLONE_THREAD automatically, and this is what the comment implies.
We could probably even drop CLONE_SIGHAND and use CLONE_THREAD, but it would be safer to not do this. The current check denies CLONE_SIGHAND implicitely and there is no reason to change this.
--- kernel/fork.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
kernel/fork.c +3 -3
@@ -1173,10 +1173,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,return ERR_PTR (-EINVAL );/* -* If the new process will be in a different pid namespace -* don't allow the creation of threads. +* If the new process will be in a different pid namespace don't +* allow the creation of threads, or share the signal handlers. */ -if ((clone_flags & (CLONE_VM |CLONE_NEWPID )) && +if ((clone_flags & (CLONE_SIGHAND |CLONE_NEWPID )) && (task_active_pid_ns (current) != current->nsproxy->pid_ns))return ERR_PTR (-EINVAL );
Signature · 1 line
1.5.5.1
Re: [PATCH 1/3] pidns: fix vfork() after unshare(CLONE_NEWPID)
Quoted 12 lines · Oleg Nesterov
8382fcac"pidns: Outlaw thread creation afterunshare(CLONE_NEWPID)" nacksCLONE_VMif the forking process unsharedpid_ns, this obviously breaks vfork:int main(void) { assert(unshare(CLONE_NEWUSER | CLONE_NEWPID) == 0); assert(vfork() >= 0); _exit(0); return 0; }
Quoted 8 lines
fails without this patch.
Change this check to use
CLONE_SIGHANDinstead. This also forbidsCLONE_THREADautomatically, and this is what the comment implies.We could probably even drop
CLONE_SIGHANDand useCLONE_THREAD, but it would be safer to not do this. The current check deniesCLONE_SIGHANDimplicitely and there is no reason to change this.
although...
Quoted 12 lines
Reported-by Colin Walters <[email protected]>Signed-off-by Oleg Nesterov <[email protected]>--- kernel/fork.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)kernel/fork.c +0
-0
kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1177
Quoted 7 lines
kernel/fork.c +2
-2@@ -1173,10 +1173,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,return ERR_PTR (-EINVAL );/* -* If the new process will be in a different pid namespace -* don't allow the creation of threads. +* If the new process will be in a different pid namespace don't +* allow the creation of threads, or share the signal handlers.
...how about "If the new process will be in a different pid namespace, don't allow it to share a thread group or signal handlers with the parent"?
--Andy
Re: [PATCH 1/3] pidns: fix vfork() after unshare(CLONE_NEWPID)
Quoted 16 lines · Andy Lutomirski
Quoted 14 lines · Oleg Nesterov
8382fcac"pidns: Outlaw thread creation afterunshare(CLONE_NEWPID)" nacksCLONE_VMif the forking process unsharedpid_ns, this obviously breaks vfork:int main(void) { assert(unshare(CLONE_NEWUSER | CLONE_NEWPID) == 0); assert(vfork() >= 0); _exit(0); return 0; }fails without this patch.
Quoted 8 lines
Quoted 6 lines
Change this check to use
CLONE_SIGHANDinstead. This also forbidsCLONE_THREADautomatically, and this is what the comment implies.We could probably even drop
CLONE_SIGHANDand useCLONE_THREAD, but it would be safer to not do this. The current check deniesCLONE_SIGHANDimplicitely and there is no reason to change this.Acked-by Andy Lutomirski <[email protected]>
Thanks Andy for taking a look.
Quoted 5 lines
Quoted 5 lines
kernel/fork.c +0
-0@@ -1173,10 +1173,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,return ERR_PTR (-EINVAL );
Quoted 9 lines
kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1177
Quoted 5 lines
kernel/fork.c +2
-2/* -* If the new process will be in a different pid namespace -* don't allow the creation of threads. +* If the new process will be in a different pid namespace don't +* allow the creation of threads, or share the signal handlers. ...how about "If the new process will be in a different pid namespace, don't allow it to share a thread group or signal handlers with the parent"?
Agreed... But in this case I do not how 3/3 should explain CLONE_PARENT, it adds "or share the parent" into this comment.
OK, I'll wait for other comments, then try to update this comment somehow and send v2.
Perhaps "... with the forking task" would be fine?
Thanks.
Oleg.
PATCH 2/3 pidns: kill the unnecessary CLONE_NEWPID in copy_process()
In reply to Oleg Nesterov8382fcac "pidns: Outlaw thread creation after unshare(CLONE_NEWPID)" nacks CLONE_NEWPID if the forking process unshared pid_ns. This is correct but unnecessary, copy_pid_ns() does the same check.
Test-case (needs root or CLONE_NEWUSER should be added):
static int child(void *arg)
{
return 0;
}
static char stack[16 * 1024];
int main(void)
{
pid_t pid;
assert(unshare(CLONE_NEWPID) == 0);
pid = clone(child, stack + sizeof(stack) / 2,
CLONE_NEWPID | SIGCHLD, NULL);
assert(pid < 0 && errno == EINVAL);
return 0;
}clone(CLONE_NEWPID) correctly fails with or without this change.
--- kernel/fork.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/fork.c +1 -1
@@ -1176,7 +1176,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,* If the new process will be in a different pid namespace don't * allow the creation of threads, or share the signal handlers. */ -if ((clone_flags & (CLONE_SIGHAND |CLONE_NEWPID )) && +if ((clone_flags &CLONE_SIGHAND ) && (task_active_pid_ns (current) != current->nsproxy->pid_ns))return ERR_PTR (-EINVAL );
Signature · 1 line
1.5.5.1
Re: [PATCH 2/3] pidns: kill the unnecessary CLONE_NEWPID in copy_process()
Quoted 26 lines · Oleg Nesterov
8382fcac"pidns: Outlaw thread creation afterunshare(CLONE_NEWPID)" nacksCLONE_NEWPIDif the forking process unsharedpid_ns. This is correct but unnecessary,copy_pid_ns()does the same check.Test-case (needs root or
CLONE_NEWUSERshould be added):static int child(void *arg) { return 0; } static char stack[16 * 1024]; int main(void) { pid_t pid; assert(unshare(CLONE_NEWPID) == 0); pid = clone(child, stack + sizeof(stack) / 2, CLONE_NEWPID | SIGCHLD, NULL); assert(pid < 0 && errno == EINVAL); return 0; }
Quoted 1 line
clone(CLONE_NEWPID)correctly fails with or without this change.
Quoted 22 lines
Signed-off-by Oleg Nesterov <[email protected]>--- kernel/fork.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)kernel/fork.c +1
-2@@ -1176,7 +1176,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,* If the new process will be in a different pid namespace don't * allow the creation of threads, or share the signal handlers. */ -if ((clone_flags & (CLONE_SIGHAND |CLONE_NEWPID )) && +if ((clone_flags &CLONE_SIGHAND ) && (task_active_pid_ns (current) != current->nsproxy->pid_ns))return ERR_PTR (-EINVAL ); --1.5.5.1
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
PATCH 3/3 fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks
In reply to Oleg Nesterovdo_fork() denies CLONE_THREAD | CLONE_PARENT if NEWUSER | NEWPID.
Then later copy_process() denies CLONE_SIGHAND if the new process will be in a different pid namespace (task_active_pid_ns() doesn't match current->nsproxy->pid_ns).
This looks confusing and inconsistent. CLONE_NEWPID is very similar to the case when ->pid_ns was already unshared, we want the same restrictions so copy_process() should also nack CLONE_PARENT.
And it would be better to deny CLONE_NEWUSER && CLONE_SIGHAND as well just for consistency.
Kill the "CLONE_NEWUSER | CLONE_NEWPID" check in do_fork() and change copy_process() to the same check along with nsproxy->pid_ns we already have.
--- kernel/fork.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-)
kernel/fork.c +8 -14
@@ -1173,12 +1173,15 @@ static struct task_struct *copy_process(unsigned long clone_flags,return ERR_PTR (-EINVAL );/* -* If the new process will be in a different pid namespace don't -* allow the creation of threads, or share the signal handlers. +* If the new process will be in a different pid or user namespace +* don't allow the creation of threads, or share the signal handlers, +* or share the parent. */ -if ((clone_flags &CLONE_SIGHAND ) && - (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 ERR_PTR (-EINVAL ); + } retval =security_task_create (clone_flags);if (retval)
@@ -1575,15 +1578,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
Signature · 1 line
1.5.5.1
Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks
Quoted 10 lines · Oleg Nesterov
do_fork()deniesCLONE_THREAD|CLONE_PARENTif NEWUSER | NEWPID.Then later
copy_process()deniesCLONE_SIGHANDif the new process will be in a different pid namespace (task_active_pid_ns()doesn't matchcurrent->nsproxy->pid_ns).This looks confusing and inconsistent.
CLONE_NEWPIDis very similar to the case when ->pid_ns was already unshared, we want the same restrictions socopy_process()should also nackCLONE_PARENT.
Quoted 6 lines
And it would be better to deny
CLONE_NEWUSER&&CLONE_SIGHANDas well just for consistency.Kill the "
CLONE_NEWUSER|CLONE_NEWPID" check indo_fork()and changecopy_process()to the same check along withnsproxy->pid_nswe already have.
Did the old code actually prevent clone(CLONE_PARENT | CLONE_NEWPID)? The new code explicitly does, and that looks like a good thing.
Quoted 50 lines
Signed-off-by Oleg Nesterov <[email protected]>--- kernel/fork.c | 22 ++++++++-------------- 1 files changed, 8 insertions(+), 14 deletions(-)kernel/fork.c +8
-15@@ -1173,12 +1173,15 @@ static struct task_struct *copy_process(unsigned long clone_flags,return ERR_PTR (-EINVAL );/* -* If the new process will be in a different pid namespace don't -* allow the creation of threads, or share the signal handlers. +* If the new process will be in a different pid or user namespace +* don't allow the creation of threads, or share the signal handlers, +* or share the parent. */ -if ((clone_flags &CLONE_SIGHAND ) && - (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 ERR_PTR (-EINVAL ); + } retval =security_task_create (clone_flags);if (retval)@@ -1575,15 +1578,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 -- 1.5.5.1
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks
Quoted 15 lines · Andy Lutomirski
Quoted 13 lines · Oleg Nesterov
do_fork()deniesCLONE_THREAD|CLONE_PARENTif NEWUSER | NEWPID.Then later
copy_process()deniesCLONE_SIGHANDif the new process will be in a different pid namespace (task_active_pid_ns()doesn't matchcurrent->nsproxy->pid_ns).This looks confusing and inconsistent.
CLONE_NEWPIDis very similar to the case when ->pid_ns was already unshared, we want the same restrictions socopy_process()should also nackCLONE_PARENT.And it would be better to deny
CLONE_NEWUSER&&CLONE_SIGHANDas well just for consistency.
Quoted 6 lines
Quoted 3 lines
Kill the "
CLONE_NEWUSER|CLONE_NEWPID" check indo_fork()and changecopy_process()to the same check along withnsproxy->pid_nswe already have.Did the old code actually prevent
clone(CLONE_PARENT | CLONE_NEWPID)? The new code explicitly does, and that looks like a good thing.
Yes. Before this patch do_fork() did:
if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
return -EINVAL;
}however, let me repeat, CLONE_PARENT after unshare(CLONE_NEWPID) was allowed. With this patch CLONE_PARENT is nacked in both cases.
Oleg.
Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks
Quoted 26 lines · Oleg Nesterov
Quoted 15 lines · Andy Lutomirski
Quoted 13 lines · Oleg Nesterov
do_fork()deniesCLONE_THREAD|CLONE_PARENTif NEWUSER | NEWPID.Then later
copy_process()deniesCLONE_SIGHANDif the new process will be in a different pid namespace (task_active_pid_ns()doesn't matchcurrent->nsproxy->pid_ns).This looks confusing and inconsistent.
CLONE_NEWPIDis very similar to the case when ->pid_ns was already unshared, we want the same restrictions socopy_process()should also nackCLONE_PARENT.And it would be better to deny
CLONE_NEWUSER&&CLONE_SIGHANDas well just for consistency.Quoted 6 lines
Quoted 3 lines
Kill the "
CLONE_NEWUSER|CLONE_NEWPID" check indo_fork()and changecopy_process()to the same check along withnsproxy->pid_nswe already have.Did the old code actually prevent
clone(CLONE_PARENT | CLONE_NEWPID)? The new code explicitly does, and that looks like a good thing.Yes. Before this patch
do_fork()did:
Quoted 8 lines
if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) { if (clone_flags & (CLONE_THREAD|CLONE_PARENT)) return -EINVAL; }however, let me repeat,
CLONE_PARENTafterunshare(CLONE_NEWPID)was allowed. With this patchCLONE_PARENTis nacked in both cases.
Is this -stable-worthy?
--Andy
Quoted 2 lines
Oleg.
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks
Quoted 10 lines · Andy Lutomirski
Quoted 8 lines · Oleg Nesterov
Yes. Before this patch
do_fork()did:if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) { if (clone_flags & (CLONE_THREAD|CLONE_PARENT)) return -EINVAL; }
Quoted 4 lines
Quoted 2 lines
however, let me repeat,
CLONE_PARENTafterunshare(CLONE_NEWPID)was allowed. With this patchCLONE_PARENTis nacked in both cases.Is this -stable-worthy?
Honestly, I do not know. I do not want to abuse -stable, and I will sleep better if this patch won't go into the stable trees ;)
OTOH, I think that at least 1/3 is probably -stable material... Since I am going to send v2, I would not mind to add [email protected] if both you and Eric agree.
Oleg.
Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks
Quoted 14 lines · Oleg Nesterov
Quoted 13 lines · Andy Lutomirski
Quoted 10 lines · Oleg Nesterov
Yes. Before this patch
do_fork()did:if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) { if (clone_flags & (CLONE_THREAD|CLONE_PARENT)) return -EINVAL; }however, let me repeat,
CLONE_PARENTafterunshare(CLONE_NEWPID)was allowed. With this patchCLONE_PARENTis nacked in both cases.
Quoted 8 lines
Quoted 1 line
Is this -stable-worthy?
Honestly, I do not know. I do not want to abuse -stable, and I will sleep better if this patch won't go into the stable trees ;)
OTOH, I think that at least 1/3 is probably -stable material... Since I am going to send v2, I would not mind to add [email protected] if both you and Eric agree.
This may allow creation of a process with tgid and pid in different pid namespaces. If so, I have no idea what the consequences would be.
Quoted 3 lines
Oleg.
Signature · 2 lines
Andy Lutomirski AMA Capital Management, LLC
Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks
Quoted 21 lines · Andy Lutomirski
Quoted 19 lines · Oleg Nesterov
Quoted 10 lines · Andy Lutomirski
Quoted 8 lines · Oleg Nesterov
Yes. Before this patch
do_fork()did:if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) { if (clone_flags & (CLONE_THREAD|CLONE_PARENT)) return -EINVAL; }Quoted 4 lines
Quoted 2 lines
however, let me repeat,
CLONE_PARENTafterunshare(CLONE_NEWPID)was allowed. With this patchCLONE_PARENTis nacked in both cases.Is this -stable-worthy?
Honestly, I do not know. I do not want to abuse -stable, and I will sleep better if this patch won't go into the stable trees ;)
Quoted 6 lines
Quoted 3 lines
OTOH, I think that at least 1/3 is probably -stable material... Since I am going to send v2, I would not mind to add [email protected] if both you and Eric agree.
This may allow creation of a process with tgid and pid in different pid namespaces. If so, I have no idea what the consequences would be.
and share the parent with the creator.
Not good. But probably not too bad, one can abuse ->pidns_install() anyway, create a child in another ns, exit. Like it or not but pid_ns is "special" and you even sent the patch to reflect this sad^W fact.
Anyway. The main point of this patch is the consistency (plus imho it cleanups/simplifies the code). Both CLONE_NEWPID and "task_active_pid_ns() != pid_ns" create a task in another namespace, we should use the same restrictions.
And you seem to agree with this change, can I take it as your ack ? I am going to preserve your acks in 1-2 and resend.
Oleg.
Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks
Quoted 35 lines · Oleg Nesterov
Quoted 21 lines · Andy Lutomirski
Quoted 19 lines · Oleg Nesterov
Quoted 10 lines · Andy Lutomirski
Quoted 8 lines · Oleg Nesterov
Yes. Before this patch
do_fork()did:if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) { if (clone_flags & (CLONE_THREAD|CLONE_PARENT)) return -EINVAL; }Quoted 4 lines
Quoted 2 lines
however, let me repeat,
CLONE_PARENTafterunshare(CLONE_NEWPID)was allowed. With this patchCLONE_PARENTis nacked in both cases.Is this -stable-worthy?
Honestly, I do not know. I do not want to abuse -stable, and I will sleep better if this patch won't go into the stable trees ;)
Quoted 6 lines
Quoted 3 lines
OTOH, I think that at least 1/3 is probably -stable material... Since I am going to send v2, I would not mind to add [email protected] if both you and Eric agree.
This may allow creation of a process with tgid and pid in different pid namespaces. If so, I have no idea what the consequences would be.
and share the parent with the creator.
Not good. But probably not too bad, one can abuse ->pidns_install() anyway, create a child in another ns, exit. Like it or not but
pid_nsis "special" and you even sent the patch to reflect this sad^W fact.
Quoted 8 lines
Anyway. The main point of this patch is the consistency (plus imho it cleanups/simplifies the code). Both
CLONE_NEWPIDand "task_active_pid_ns()!=pid_ns" create a task in another namespace, we should use the same restrictions.And you seem to agree with this change, can I take it as your ack ? I am going to preserve your acks in 1-2 and resend.
Yes. These patches probably want Eric's ack, too, though -- it's his code.
--Andy