[PATCH] Fix PID namespace persistence

[PATCH] Fix PID namespace persistence

[michael-dev]

From: michael-dev <michael-dev@fami-braun.de>

After unshare(...) is called, /proc/self/ns/pid does not change.
Instead, only /proc/self/ns/pid_for_children is affected. So bind-mounting /proc/self/ns/pid results in the original namespace getting bind-mounted.

Fix this by instead bind-mounting ns/pid_for_children.

Signed-off-by: Michael Braun <michael-dev@fami-braun.de>
---
 sys-utils/unshare.c | 66 ++++++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 28 deletions(-)

diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
index 8652ebdaf..c3ba18e32 100644
--- a/sys-utils/unshare.c
+++ b/sys-utils/unshare.c
@@ -63,7 +63,7 @@ static struct namespace_file {
 	{ .type = CLONE_NEWIPC,   .name = "ns/ipc"  },
 	{ .type = CLONE_NEWUTS,   .name = "ns/uts"  },
 	{ .type = CLONE_NEWNET,   .name = "ns/net"  },
-	{ .type = CLONE_NEWPID,   .name = "ns/pid"  },
+	{ .type = CLONE_NEWPID,   .name = "ns/pid_for_children"  },
 	{ .type = CLONE_NEWNS,    .name = "ns/mnt"  },
 	{ .type = CLONE_NEWTIME,  .name = "ns/time"  },
 	{ .name = NULL }
@@ -361,6 +361,7 @@ int main(int argc, char *argv[])
 	const char *procmnt = NULL;
 	const char *newroot = NULL;
 	const char *newdir = NULL;
+	pid_t pid_bind = 0;
 	pid_t pid = 0;
 	int fds[2];
 	int status;
@@ -501,13 +502,37 @@ int main(int argc, char *argv[])
 			"unsharing of a time namespace (-t)"));
 
 	if (npersists && (unshare_flags & CLONE_NEWNS))
-		bind_ns_files_from_child(&pid, fds);
+		bind_ns_files_from_child(&pid_bind, fds);
 
 	if (-1 == unshare(unshare_flags))
 		err(EXIT_FAILURE, _("unshare failed"));
 
-	if (npersists) {
-		if (pid && (unshare_flags & CLONE_NEWNS)) {
+	if (force_boottime)
+		settime(boottime, CLOCK_BOOTTIME);
+
+	if (force_monotonic)
+		settime(monotonic, CLOCK_MONOTONIC);
+
+	if (forkit) {
+		// force child forking before mountspace binding
+		// so pid_for_children is populated
+		pid = fork();
+
+		switch(pid) {
+		case -1:
+			err(EXIT_FAILURE, _("fork failed"));
+		case 0:	/* child */
+			if (pid_bind && (unshare_flags & CLONE_NEWNS))
+				close(fds[1]);
+			break;
+		default: /* parent */
+      break;
+		}
+	}
+
+	if (npersists && (pid || !forkit)) {
+		// run in parent
+		if (pid_bind && (unshare_flags & CLONE_NEWNS)) {
 			int rc;
 			char ch = PIPE_SYNC_BYTE;
 
@@ -518,7 +543,7 @@ int main(int argc, char *argv[])
 
 			/* wait for bind_ns_files_from_child() */
 			do {
-				rc = waitpid(pid, &status, 0);
+				rc = waitpid(pid_bind, &status, 0);
 				if (rc < 0) {
 					if (errno == EINTR)
 						continue;
@@ -533,29 +558,14 @@ int main(int argc, char *argv[])
 			bind_ns_files(getpid());
 	}
 
-	if (force_boottime)
-		settime(boottime, CLOCK_BOOTTIME);
-
-	if (force_monotonic)
-		settime(monotonic, CLOCK_MONOTONIC);
-
-	if (forkit) {
-		pid = fork();
-
-		switch(pid) {
-		case -1:
-			err(EXIT_FAILURE, _("fork failed"));
-		case 0:	/* child */
-			break;
-		default: /* parent */
-			if (waitpid(pid, &status, 0) == -1)
-				err(EXIT_FAILURE, _("waitpid failed"));
-			if (WIFEXITED(status))
-				return WEXITSTATUS(status);
-			else if (WIFSIGNALED(status))
-				kill(getpid(), WTERMSIG(status));
-			err(EXIT_FAILURE, _("child exit failed"));
-		}
+	if (pid) {
+		if (waitpid(pid, &status, 0) == -1)
+			err(EXIT_FAILURE, _("waitpid failed"));
+		if (WIFEXITED(status))
+			return WEXITSTATUS(status);
+		else if (WIFSIGNALED(status))
+			kill(getpid(), WTERMSIG(status));
+		err(EXIT_FAILURE, _("child exit failed"));
 	}
 
 	if (kill_child_signo != 0 && prctl(PR_SET_PDEATHSIG, kill_child_signo) < 0)
-- 
2.17.1

Re: [PATCH] Fix PID namespace persistence

[Karel Zak]

 Eric, can you review this unshare(8) patch? Please.

    Karel

On Wed, Apr 15, 2020 at 11:16:53PM +0200, michael-dev@fami-braun.de wrote:
> From: michael-dev <michael-dev@fami-braun.de>
> 
> After unshare(...) is called, /proc/self/ns/pid does not change.
> Instead, only /proc/self/ns/pid_for_children is affected. So bind-mounting /proc/self/ns/pid results in the original namespace getting bind-mounted.
> 
> Fix this by instead bind-mounting ns/pid_for_children.
> 
> Signed-off-by: Michael Braun <michael-dev@fami-braun.de>
> ---
>  sys-utils/unshare.c | 66 ++++++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
> index 8652ebdaf..c3ba18e32 100644
> --- a/sys-utils/unshare.c
> +++ b/sys-utils/unshare.c
> @@ -63,7 +63,7 @@ static struct namespace_file {
>  	{ .type = CLONE_NEWIPC,   .name = "ns/ipc"  },
>  	{ .type = CLONE_NEWUTS,   .name = "ns/uts"  },
>  	{ .type = CLONE_NEWNET,   .name = "ns/net"  },
> -	{ .type = CLONE_NEWPID,   .name = "ns/pid"  },
> +	{ .type = CLONE_NEWPID,   .name = "ns/pid_for_children"  },
>  	{ .type = CLONE_NEWNS,    .name = "ns/mnt"  },
>  	{ .type = CLONE_NEWTIME,  .name = "ns/time"  },
>  	{ .name = NULL }
> @@ -361,6 +361,7 @@ int main(int argc, char *argv[])
>  	const char *procmnt = NULL;
>  	const char *newroot = NULL;
>  	const char *newdir = NULL;
> +	pid_t pid_bind = 0;
>  	pid_t pid = 0;
>  	int fds[2];
>  	int status;
> @@ -501,13 +502,37 @@ int main(int argc, char *argv[])
>  			"unsharing of a time namespace (-t)"));
>  
>  	if (npersists && (unshare_flags & CLONE_NEWNS))
> -		bind_ns_files_from_child(&pid, fds);
> +		bind_ns_files_from_child(&pid_bind, fds);
>  
>  	if (-1 == unshare(unshare_flags))
>  		err(EXIT_FAILURE, _("unshare failed"));
>  
> -	if (npersists) {
> -		if (pid && (unshare_flags & CLONE_NEWNS)) {
> +	if (force_boottime)
> +		settime(boottime, CLOCK_BOOTTIME);
> +
> +	if (force_monotonic)
> +		settime(monotonic, CLOCK_MONOTONIC);
> +
> +	if (forkit) {
> +		// force child forking before mountspace binding
> +		// so pid_for_children is populated
> +		pid = fork();
> +
> +		switch(pid) {
> +		case -1:
> +			err(EXIT_FAILURE, _("fork failed"));
> +		case 0:	/* child */
> +			if (pid_bind && (unshare_flags & CLONE_NEWNS))
> +				close(fds[1]);
> +			break;
> +		default: /* parent */
> +      break;
> +		}
> +	}
> +
> +	if (npersists && (pid || !forkit)) {
> +		// run in parent
> +		if (pid_bind && (unshare_flags & CLONE_NEWNS)) {
>  			int rc;
>  			char ch = PIPE_SYNC_BYTE;
>  
> @@ -518,7 +543,7 @@ int main(int argc, char *argv[])
>  
>  			/* wait for bind_ns_files_from_child() */
>  			do {
> -				rc = waitpid(pid, &status, 0);
> +				rc = waitpid(pid_bind, &status, 0);
>  				if (rc < 0) {
>  					if (errno == EINTR)
>  						continue;
> @@ -533,29 +558,14 @@ int main(int argc, char *argv[])
>  			bind_ns_files(getpid());
>  	}
>  
> -	if (force_boottime)
> -		settime(boottime, CLOCK_BOOTTIME);
> -
> -	if (force_monotonic)
> -		settime(monotonic, CLOCK_MONOTONIC);
> -
> -	if (forkit) {
> -		pid = fork();
> -
> -		switch(pid) {
> -		case -1:
> -			err(EXIT_FAILURE, _("fork failed"));
> -		case 0:	/* child */
> -			break;
> -		default: /* parent */
> -			if (waitpid(pid, &status, 0) == -1)
> -				err(EXIT_FAILURE, _("waitpid failed"));
> -			if (WIFEXITED(status))
> -				return WEXITSTATUS(status);
> -			else if (WIFSIGNALED(status))
> -				kill(getpid(), WTERMSIG(status));
> -			err(EXIT_FAILURE, _("child exit failed"));
> -		}
> +	if (pid) {
> +		if (waitpid(pid, &status, 0) == -1)
> +			err(EXIT_FAILURE, _("waitpid failed"));
> +		if (WIFEXITED(status))
> +			return WEXITSTATUS(status);
> +		else if (WIFSIGNALED(status))
> +			kill(getpid(), WTERMSIG(status));
> +		err(EXIT_FAILURE, _("child exit failed"));
>  	}
>  
>  	if (kill_child_signo != 0 && prctl(PR_SET_PDEATHSIG, kill_child_signo) < 0)
> -- 
> 2.17.1
> 

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] Fix PID namespace persistence

[Eric W. Biederman]

michael-dev@fami-braun.de writes:

> From: michael-dev <michael-dev@fami-braun.de>
>
> After unshare(...) is called, /proc/self/ns/pid does not change.
> Instead, only /proc/self/ns/pid_for_children is affected. So bind-mounting /proc/self/ns/pid results in the original namespace getting bind-mounted.
>
> Fix this by instead bind-mounting ns/pid_for_children.

Why all of the extra code motion and change?

Your description sounds like only the first hunk is necessary.  Did
something else get mixed into this change?  Or are all of the hunks
necessary?

Eric


> Signed-off-by: Michael Braun <michael-dev@fami-braun.de>
> ---
>  sys-utils/unshare.c | 66 ++++++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 28 deletions(-)
>
> diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
> index 8652ebdaf..c3ba18e32 100644
> --- a/sys-utils/unshare.c
> +++ b/sys-utils/unshare.c
> @@ -63,7 +63,7 @@ static struct namespace_file {
>  	{ .type = CLONE_NEWIPC,   .name = "ns/ipc"  },
>  	{ .type = CLONE_NEWUTS,   .name = "ns/uts"  },
>  	{ .type = CLONE_NEWNET,   .name = "ns/net"  },
> -	{ .type = CLONE_NEWPID,   .name = "ns/pid"  },
> +	{ .type = CLONE_NEWPID,   .name = "ns/pid_for_children"  },
>  	{ .type = CLONE_NEWNS,    .name = "ns/mnt"  },
>  	{ .type = CLONE_NEWTIME,  .name = "ns/time"  },
>  	{ .name = NULL }
> @@ -361,6 +361,7 @@ int main(int argc, char *argv[])
>  	const char *procmnt = NULL;
>  	const char *newroot = NULL;
>  	const char *newdir = NULL;
> +	pid_t pid_bind = 0;
>  	pid_t pid = 0;
>  	int fds[2];
>  	int status;
> @@ -501,13 +502,37 @@ int main(int argc, char *argv[])
>  			"unsharing of a time namespace (-t)"));
>  
>  	if (npersists && (unshare_flags & CLONE_NEWNS))
> -		bind_ns_files_from_child(&pid, fds);
> +		bind_ns_files_from_child(&pid_bind, fds);
>  
>  	if (-1 == unshare(unshare_flags))
>  		err(EXIT_FAILURE, _("unshare failed"));
>  
> -	if (npersists) {
> -		if (pid && (unshare_flags & CLONE_NEWNS)) {
> +	if (force_boottime)
> +		settime(boottime, CLOCK_BOOTTIME);
> +
> +	if (force_monotonic)
> +		settime(monotonic, CLOCK_MONOTONIC);
> +
> +	if (forkit) {
> +		// force child forking before mountspace binding
> +		// so pid_for_children is populated
> +		pid = fork();
> +
> +		switch(pid) {
> +		case -1:
> +			err(EXIT_FAILURE, _("fork failed"));
> +		case 0:	/* child */
> +			if (pid_bind && (unshare_flags & CLONE_NEWNS))
> +				close(fds[1]);
> +			break;
> +		default: /* parent */
> +      break;
> +		}
> +	}
> +
> +	if (npersists && (pid || !forkit)) {
> +		// run in parent
> +		if (pid_bind && (unshare_flags & CLONE_NEWNS)) {
>  			int rc;
>  			char ch = PIPE_SYNC_BYTE;
>  
> @@ -518,7 +543,7 @@ int main(int argc, char *argv[])
>  
>  			/* wait for bind_ns_files_from_child() */
>  			do {
> -				rc = waitpid(pid, &status, 0);
> +				rc = waitpid(pid_bind, &status, 0);
>  				if (rc < 0) {
>  					if (errno == EINTR)
>  						continue;
> @@ -533,29 +558,14 @@ int main(int argc, char *argv[])
>  			bind_ns_files(getpid());
>  	}
>  
> -	if (force_boottime)
> -		settime(boottime, CLOCK_BOOTTIME);
> -
> -	if (force_monotonic)
> -		settime(monotonic, CLOCK_MONOTONIC);
> -
> -	if (forkit) {
> -		pid = fork();
> -
> -		switch(pid) {
> -		case -1:
> -			err(EXIT_FAILURE, _("fork failed"));
> -		case 0:	/* child */
> -			break;
> -		default: /* parent */
> -			if (waitpid(pid, &status, 0) == -1)
> -				err(EXIT_FAILURE, _("waitpid failed"));
> -			if (WIFEXITED(status))
> -				return WEXITSTATUS(status);
> -			else if (WIFSIGNALED(status))
> -				kill(getpid(), WTERMSIG(status));
> -			err(EXIT_FAILURE, _("child exit failed"));
> -		}
> +	if (pid) {
> +		if (waitpid(pid, &status, 0) == -1)
> +			err(EXIT_FAILURE, _("waitpid failed"));
> +		if (WIFEXITED(status))
> +			return WEXITSTATUS(status);
> +		else if (WIFSIGNALED(status))
> +			kill(getpid(), WTERMSIG(status));
> +		err(EXIT_FAILURE, _("child exit failed"));
>  	}
>  
>  	if (kill_child_signo != 0 && prctl(PR_SET_PDEATHSIG, kill_child_signo) < 0)

Re: [PATCH] Fix PID namespace persistence

[michael-dev]

Am 16.04.2020 22:49, schrieb ebiederm@xmission.com:
> Why all of the extra code motion and change?
> 
> Your description sounds like only the first hunk is necessary.  Did
> something else get mixed into this change?  Or are all of the hunks
> necessary?

Because after unsharing, pid_for_children is a dangling symlink.
So if (forkit) needs to be before /* wait for bind_ns_files_from_child() 
*/.
Moving this results in an overlapping use of the pid variable, so that 
gets renamed.
Additionally, we cannot wait for the forkit child (bash) to exit before 
triggering and waiting for the bind-mounting child to bind-mount the 
persistent namespace. So forking and waiting for the forkit child (bash) 
needs to become splitted.

Regards,
Michael

Re: [PATCH] Fix PID namespace persistence

[Michael Kerrisk]

On Sun, Apr 19, 2020 at 1:06 AM michael-dev <michael-dev@fami-braun.de> wrote:
>
> Am 16.04.2020 22:49, schrieb ebiederm@xmission.com:
> > Why all of the extra code motion and change?
> >
> > Your description sounds like only the first hunk is necessary.  Did
> > something else get mixed into this change?  Or are all of the hunks
> > necessary?
>
> Because after unsharing, pid_for_children is a dangling symlink.
> So if (forkit) needs to be before /* wait for bind_ns_files_from_child()
> */.
> Moving this results in an overlapping use of the pid variable, so that
> gets renamed.
> Additionally, we cannot wait for the forkit child (bash) to exit before
> triggering and waiting for the bind-mounting child to bind-mount the
> persistent namespace. So forking and waiting for the forkit child (bash)
> needs to become splitted.

@Michael: I confim theproblem, and thanks for this fix.

@Zak: an analogous fix is required for time namespaces.

Cheers,

Michael


-- 
Michael Kerrisk Linux man-pages maintainer;
http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface", http://blog.man7.org/

Re: [PATCH] Fix PID namespace persistence

[Karel Zak]

On Fri, Apr 24, 2020 at 11:21:15AM +0200, Michael Kerrisk wrote:
> On Sun, Apr 19, 2020 at 1:06 AM michael-dev <michael-dev@fami-braun.de> wrote:
> >
> > Am 16.04.2020 22:49, schrieb ebiederm@xmission.com:
> > > Why all of the extra code motion and change?
> > >
> > > Your description sounds like only the first hunk is necessary.  Did
> > > something else get mixed into this change?  Or are all of the hunks
> > > necessary?
> >
> > Because after unsharing, pid_for_children is a dangling symlink.
> > So if (forkit) needs to be before /* wait for bind_ns_files_from_child()
> > */.
> > Moving this results in an overlapping use of the pid variable, so that
> > gets renamed.
> > Additionally, we cannot wait for the forkit child (bash) to exit before
> > triggering and waiting for the bind-mounting child to bind-mount the
> > persistent namespace. So forking and waiting for the forkit child (bash)
> > needs to become splitted.
> 
> @Michael: I confim theproblem, and thanks for this fix.
> 
> @Zak: an analogous fix is required for time namespaces.

OK, I'll play with it later this week. Sorry for the delay, I'm busy
with other things.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] Fix PID namespace persistence

[Michael Kerrisk (man-pages)]

[I should have CCed Adrian, so that he is aware of the issue also,
having added Time Namespaces; Adrian, for context, see
https://lore.kernel.org/util-linux/20200427093346.tcoujcasylusejfq@ws.net.home/T/#t
]

On Mon, 27 Apr 2020 at 11:33, Karel Zak <kzak@redhat.com> wrote:
>
> On Fri, Apr 24, 2020 at 11:21:15AM +0200, Michael Kerrisk wrote:
> > On Sun, Apr 19, 2020 at 1:06 AM michael-dev <michael-dev@fami-braun.de> wrote:
> > >
> > > Am 16.04.2020 22:49, schrieb ebiederm@xmission.com:
> > > > Why all of the extra code motion and change?
> > > >
> > > > Your description sounds like only the first hunk is necessary.  Did
> > > > something else get mixed into this change?  Or are all of the hunks
> > > > necessary?
> > >
> > > Because after unsharing, pid_for_children is a dangling symlink.
> > > So if (forkit) needs to be before /* wait for bind_ns_files_from_child()
> > > */.
> > > Moving this results in an overlapping use of the pid variable, so that
> > > gets renamed.
> > > Additionally, we cannot wait for the forkit child (bash) to exit before
> > > triggering and waiting for the bind-mounting child to bind-mount the
> > > persistent namespace. So forking and waiting for the forkit child (bash)
> > > needs to become splitted.
> >
> > @Michael: I confim theproblem, and thanks for this fix.
> >
> > @Zak: an analogous fix is required for time namespaces.
>
> OK, I'll play with it later this week. Sorry for the delay, I'm busy
> with other things.

Thanks, Karel!

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] Fix PID namespace persistence

[Karel Zak]

On Wed, Apr 15, 2020 at 11:16:53PM +0200, michael-dev@fami-braun.de wrote:
>  sys-utils/unshare.c | 66 ++++++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 28 deletions(-)

Applied. I did small changes to the patch, see:
https://github.com/karelzak/util-linux/commit/0d5260b66c5581c8a5855a5f49e298e48e8baf82


On Fri, Apr 24, 2020 at 11:21:15AM +0200, Michael Kerrisk wrote:
> @Zak: an analogous fix is required for time namespaces.

A have added ns/time_for_children to the commit.

Thanks guys!

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com