samples: make pidfd-metadata fail gracefully on older kernels

Initialize pidfd to an invalid descriptor, to fail gracefully on those kernels that do not implement CLONE_PIDFD and leave pidfd unchanged.

Signed-off-by Dmitry V. Levin <[email protected]>

---
 samples/pidfd/pidfd-metadata.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

samples/pidfd/pidfd-metadata.c +0 -0

index 14b454448429..ff109fdac3a5 100644

Quoted 15 lines · Dmitry V. Levin

Initialize pidfd to an invalid descriptor, to fail gracefully on those kernels that do not implement CLONE_PIDFD and leave pidfd unchanged.

Signed-off-by Dmitry V. Levin <[email protected]>

---
 samples/pidfd/pidfd-metadata.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

samples/pidfd/pidfd-metadata.c +0 -0

index 14b454448429..ff109fdac3a5 100644
@@ -83,7 +83,7 @@ static int pidfd_metadata_fd(pid_t pid, int pidfd)
 

samples/pidfd/pidfd-metadata.c static int pidfd_metadata_fd(pid_t pid, int pidfd) · line 86

Quoted 4 lines

samples/pidfd/pidfd-metadata.c +1 -1

 int main(int argc, char *argv[])
 {
-	int pidfd = 0, ret = EXIT_FAILURE;
+	int pidfd = -1, ret = EXIT_FAILURE;

Hm, that currently won't work since we added a check in fork.c for pidfd == 0. If it isn't you'll get EINVAL.

This was done to ensure that
we can potentially extend CLONE_PIDFD by passing in flags through the return argument.
However, I find this increasingly unlikely. Especially since the interface would be horrendous and an absolute last resort. If clone3() gets merged for 5.3 (currently in linux-next) we also have no real need anymore to extend legacy clone() this way. So either wait until (if) we merge clone3() where the check I mentioned is gone anyway, or remove the pidfd == 0 check from fork.c in a preliminary patch. Thoughts?

Cc'ed more people as the issue is not just with the example but with the interface itself.

Quoted 16 lines · Christian Brauner

Quoted 15 lines · Dmitry V. Levin

Initialize pidfd to an invalid descriptor, to fail gracefully on those kernels that do not implement CLONE_PIDFD and leave pidfd unchanged.

Signed-off-by Dmitry V. Levin <[email protected]>

---
 samples/pidfd/pidfd-metadata.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

samples/pidfd/pidfd-metadata.c +0 -0

index 14b454448429..ff109fdac3a5 100644
@@ -83,7 +83,7 @@ static int pidfd_metadata_fd(pid_t pid, int pidfd)
 

Quoted 7 lines

samples/pidfd/pidfd-metadata.c static int pidfd_metadata_fd(pid_t pid, int pidfd) · line 86

Quoted 4 lines

samples/pidfd/pidfd-metadata.c +1 -1

 int main(int argc, char *argv[])
 {
-	int pidfd = 0, ret = EXIT_FAILURE;
+	int pidfd = -1, ret = EXIT_FAILURE;

Hm, that currently won't work since we added a check in fork.c for pidfd == 0. If it isn't you'll get EINVAL.

Sorry, I must've missed that check. But this makes things even worse.

Quoted 10 lines

This was done to ensure that
we can potentially extend CLONE_PIDFD by passing in flags through the return argument.
However, I find this increasingly unlikely. Especially since the interface would be horrendous and an absolute last resort. If clone3() gets merged for 5.3 (currently in linux-next) we also have no real need anymore to extend legacy clone() this way. So either wait until (if) we merge clone3() where the check I mentioned is gone anyway, or remove the pidfd == 0 check from fork.c in a preliminary patch. Thoughts?

Userspace needs a reliable way to tell whether CLONE_PIDFD is supported by the kernel or not.

If CLONE_PIDFD is not supported, then pidfd remains unchanged.

If CLONE_PIDFD is supported and fd 0 is closed, then mandatory pidfd == 0 also remains unchanged, which effectively means that userspace must ensure that fd 0 is not closed when invoking CLONE_PIDFD. This is ugly.

If we can assume that clone(CLONE_PIDFD) is not going to be extended, then I'm for removing the pidfd == 0 check along with recommending userspace to initialize pidfd with -1.

Quoted 41 lines · Dmitry V. Levin

Cc'ed more people as the issue is not just with the example but with the interface itself.

Quoted 16 lines · Christian Brauner

Quoted 15 lines · Dmitry V. Levin

Initialize pidfd to an invalid descriptor, to fail gracefully on those kernels that do not implement CLONE_PIDFD and leave pidfd unchanged.

Signed-off-by Dmitry V. Levin <[email protected]>

---
 samples/pidfd/pidfd-metadata.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

samples/pidfd/pidfd-metadata.c +0 -0

index 14b454448429..ff109fdac3a5 100644
@@ -83,7 +83,7 @@ static int pidfd_metadata_fd(pid_t pid, int pidfd)
 

Quoted 7 lines

samples/pidfd/pidfd-metadata.c static int pidfd_metadata_fd(pid_t pid, int pidfd) · line 86

Quoted 4 lines

samples/pidfd/pidfd-metadata.c +1 -1

 int main(int argc, char *argv[])
 {
-	int pidfd = 0, ret = EXIT_FAILURE;
+	int pidfd = -1, ret = EXIT_FAILURE;

Hm, that currently won't work since we added a check in fork.c for pidfd == 0. If it isn't you'll get EINVAL.

Sorry, I must've missed that check. But this makes things even worse.

Quoted 10 lines

This was done to ensure that
we can potentially extend CLONE_PIDFD by passing in flags through the return argument.
However, I find this increasingly unlikely. Especially since the interface would be horrendous and an absolute last resort. If clone3() gets merged for 5.3 (currently in linux-next) we also have no real need anymore to extend legacy clone() this way. So either wait until (if) we merge clone3() where the check I mentioned is gone anyway, or remove the pidfd == 0 check from fork.c in a preliminary patch. Thoughts?

Quoted 2 lines

Userspace needs a reliable way to tell whether CLONE_PIDFD is supported by the kernel or not.

Right, that's the general problem with legacy clone(): it ignores unknown flags... clone3() will EINVAL you if you pass any flag it doesn't know about.

For legacy clone you can pass

(CLONE_PIDFD | CLONE_DETACHED)

on all relevant kernels >= 2.6.2. CLONE_DETACHED will be silently ignored by the kernel if specified in flags. But if you specify both CLONE_PIDFD and CLONE_DETACHED on a kernel that does support CLONE_PIDFD you'll get EINVALed. (We did this because we wanted to have the ability to make CLONE_DETACHED reuseable with CLONE_PIDFD.)
Does that help?

Quoted 9 lines

If CLONE_PIDFD is not supported, then pidfd remains unchanged.

If CLONE_PIDFD is supported and fd 0 is closed, then mandatory pidfd == 0 also remains unchanged, which effectively means that userspace must ensure that fd 0 is not closed when invoking CLONE_PIDFD. This is ugly.

If we can assume that clone(CLONE_PIDFD) is not going to be extended, then I'm for removing the pidfd == 0 check along with recommending userspace to initialize pidfd with -1.

Right, I'm ok with that too.

Quoted 51 lines · Christian Brauner

Quoted 41 lines · Dmitry V. Levin

Cc'ed more people as the issue is not just with the example but with the interface itself.

Quoted 16 lines · Christian Brauner

Quoted 15 lines · Dmitry V. Levin

Initialize pidfd to an invalid descriptor, to fail gracefully on those kernels that do not implement CLONE_PIDFD and leave pidfd unchanged.

Signed-off-by Dmitry V. Levin <[email protected]>

---
 samples/pidfd/pidfd-metadata.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

samples/pidfd/pidfd-metadata.c +0 -0

index 14b454448429..ff109fdac3a5 100644
@@ -83,7 +83,7 @@ static int pidfd_metadata_fd(pid_t pid, int pidfd)
 

Quoted 7 lines

samples/pidfd/pidfd-metadata.c static int pidfd_metadata_fd(pid_t pid, int pidfd) · line 86

Quoted 4 lines

samples/pidfd/pidfd-metadata.c +1 -1

 int main(int argc, char *argv[])
 {
-	int pidfd = 0, ret = EXIT_FAILURE;
+	int pidfd = -1, ret = EXIT_FAILURE;

Hm, that currently won't work since we added a check in fork.c for pidfd == 0. If it isn't you'll get EINVAL.

Sorry, I must've missed that check. But this makes things even worse.

Quoted 10 lines

This was done to ensure that
we can potentially extend CLONE_PIDFD by passing in flags through the return argument.
However, I find this increasingly unlikely. Especially since the interface would be horrendous and an absolute last resort. If clone3() gets merged for 5.3 (currently in linux-next) we also have no real need anymore to extend legacy clone() this way. So either wait until (if) we merge clone3() where the check I mentioned is gone anyway, or remove the pidfd == 0 check from fork.c in a preliminary patch. Thoughts?

Quoted 2 lines

Userspace needs a reliable way to tell whether CLONE_PIDFD is supported by the kernel or not.

Right, that's the general problem with legacy clone(): it ignores unknown flags... clone3() will EINVAL you if you pass any flag it doesn't know about.

For legacy clone you can pass

Quoted 8 lines

(CLONE_PIDFD | CLONE_DETACHED)

on all relevant kernels >= 2.6.2. CLONE_DETACHED will be silently ignored by the kernel if specified in flags. But if you specify both CLONE_PIDFD and CLONE_DETACHED on a kernel that does support CLONE_PIDFD you'll get EINVALed. (We did this because we wanted to have the ability to make CLONE_DETACHED reuseable with CLONE_PIDFD.)
Does that help?

Yes, this is feasible, but the cost is extra syscall for new kernels and more complicated userspace code, so...

Quoted 7 lines

Quoted 7 lines

If CLONE_PIDFD is not supported, then pidfd remains unchanged.

If CLONE_PIDFD is supported and fd 0 is closed, then mandatory pidfd == 0 also remains unchanged, which effectively means that userspace must ensure that fd 0 is not closed when invoking CLONE_PIDFD. This is ugly.

Quoted 5 lines

Quoted 3 lines

If we can assume that clone(CLONE_PIDFD) is not going to be extended, then I'm for removing the pidfd == 0 check along with recommending userspace to initialize pidfd with -1.

Right, I'm ok with that too.

... I'd prefer this variant.

Initialize pidfd to an invalid descriptor, to fail gracefully on those kernels that do not implement CLONE_PIDFD and leave pidfd unchanged.

Signed-off-by Dmitry V. Levin <[email protected]>

---
 samples/pidfd/pidfd-metadata.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

samples/pidfd/pidfd-metadata.c +6 -2

index 14b454448429..c459155daf9a 100644
@@ -83,7 +83,7 @@ static int pidfd_metadata_fd(pid_t pid, int pidfd)
 
 int main(int argc, char *argv[])
 {
-	int pidfd = 0, ret = EXIT_FAILURE;
+	int pidfd = -1, ret = EXIT_FAILURE;
 	char buf[4096] = { 0 };
 	pid_t pid;
 	int procfd, statusfd;

@@ -91,7 +91,11 @@ int main(int argc, char *argv[])
 
 	pid = pidfd_clone(CLONE_PIDFD, &pidfd);
 	if (pid < 0)
-		exit(ret);
+		err(ret, "CLONE_PIDFD");
+	if (pidfd == -1) {
+		warnx("CLONE_PIDFD is not supported by the kernel");
+		goto out;
+	}
 
 	procfd = pidfd_metadata_fd(pid, pidfd);
 	close(pidfd);

Signature · 1 line

ldv

Userspace needs a cheap and reliable way to tell whether CLONE_PIDFD is supported by the kernel or not.

While older kernels without CLONE_PIDFD support just leave unchanged the value pointed by parent_tidptr, current implementation fails with EINVAL if that value is non-zero.

If CLONE_PIDFD is supported and fd 0 is closed, then mandatory pidfd == 0 pointed by parent_tidptr also remains unchanged, which effectively means that userspace must either check CLONE_PIDFD support beforehand or ensure that fd 0 is not closed when invoking CLONE_PIDFD.

The check for pidfd == 0 was introduced during v5.2 release cycle by commit b3e583825266 ("clone: add CLONE_PIDFD") to ensure that CLONE_PIDFD could be potentially extended by passing in flags through the return argument.

However, that extension would look horrendous, and with introduction of clone3 syscall in v5.3 there is no need to extend legacy clone syscall this way.

So remove the pidfd == 0 check. Userspace that needs to be portable to kernels without CLONE_PIDFD support is advised to initialize pidfd with -1 and check the pidfd value returned by CLONE_PIDFD.

Signed-off-by Dmitry V. Levin <[email protected]>

---
 kernel/fork.c | 12 ------------
 1 file changed, 12 deletions(-)

kernel/fork.c +0 -12

index 75675b9bf6df..39a3adaa4ad1 100644
@@ -1822,8 +1822,6 @@ static __latent_entropy struct task_struct *copy_process(
 	}
 
 	if (clone_flags & CLONE_PIDFD) {
-		int reserved;
-
 		/*
 		 * - CLONE_PARENT_SETTID is useless for pidfds and also
 		 *   parent_tidptr is used to return pidfds.

@@ -1834,16 +1832,6 @@ static __latent_entropy struct task_struct *copy_process(
 		if (clone_flags &
 		    (CLONE_DETACHED | CLONE_PARENT_SETTID | CLONE_THREAD))
 			return ERR_PTR(-EINVAL);
-
-		/*
-		 * Verify that parent_tidptr is sane so we can potentially
-		 * reuse it later.
-		 */
-		if (get_user(reserved, parent_tidptr))
-			return ERR_PTR(-EFAULT);
-
-		if (reserved != 0)
-			return ERR_PTR(-EINVAL);
 	}
 
 	/*

Signature · 1 line

ldv

Quoted 25 lines · Dmitry V. Levin

Userspace needs a cheap and reliable way to tell whether CLONE_PIDFD is supported by the kernel or not.

While older kernels without CLONE_PIDFD support just leave unchanged the value pointed by parent_tidptr, current implementation fails with EINVAL if that value is non-zero.

If CLONE_PIDFD is supported and fd 0 is closed, then mandatory pidfd == 0 pointed by parent_tidptr also remains unchanged, which effectively means that userspace must either check CLONE_PIDFD support beforehand or ensure that fd 0 is not closed when invoking CLONE_PIDFD.

The check for pidfd == 0 was introduced during v5.2 release cycle by commit b3e583825266 ("clone: add CLONE_PIDFD") to ensure that CLONE_PIDFD could be potentially extended by passing in flags through the return argument.

However, that extension would look horrendous, and with introduction of clone3 syscall in v5.3 there is no need to extend legacy clone syscall this way.

So remove the pidfd == 0 check. Userspace that needs to be portable to kernels without CLONE_PIDFD support is advised to initialize pidfd with -1 and check the pidfd value returned by CLONE_PIDFD.

Quoted 1 line

Signed-off-by Dmitry V. Levin <[email protected]>

Reviewed-by Christian Brauner <[email protected]>

Thank you Dmitry, queueing this up for rc7.

Quoted 30 lines · Christian Brauner

Quoted 21 lines · Dmitry V. Levin

Userspace needs a cheap and reliable way to tell whether CLONE_PIDFD is supported by the kernel or not.

While older kernels without CLONE_PIDFD support just leave unchanged the value pointed by parent_tidptr, current implementation fails with EINVAL if that value is non-zero.

If CLONE_PIDFD is supported and fd 0 is closed, then mandatory pidfd == 0 pointed by parent_tidptr also remains unchanged, which effectively means that userspace must either check CLONE_PIDFD support beforehand or ensure that fd 0 is not closed when invoking CLONE_PIDFD.

The check for pidfd == 0 was introduced during v5.2 release cycle by commit b3e583825266 ("clone: add CLONE_PIDFD") to ensure that CLONE_PIDFD could be potentially extended by passing in flags through the return argument.

However, that extension would look horrendous, and with introduction of clone3 syscall in v5.3 there is no need to extend legacy clone syscall this way.

Quoted 5 lines

So remove the pidfd == 0 check. Userspace that needs to be portable to kernels without CLONE_PIDFD support is advised to initialize pidfd with -1 and check the pidfd value returned by CLONE_PIDFD.

Signed-off-by Dmitry V. Levin <[email protected]>

Reviewed-by Christian Brauner <[email protected]>

Quoted 1 line

Thank you Dmitry, queueing this up for rc7.

This is now sitting in

https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/commit/?h=fixes&id=43754d05f235dd1b6c7f8ab9f42007770d721f10

I reformulated the commit message a bit and gave it a Fixes tag. Dmitry, if you want you can take a look and tell me if that's acceptable to you.

Quoted 33 lines · Christian Brauner

Quoted 31 lines · Christian Brauner

Quoted 21 lines · Dmitry V. Levin

Userspace needs a cheap and reliable way to tell whether CLONE_PIDFD is supported by the kernel or not.

While older kernels without CLONE_PIDFD support just leave unchanged the value pointed by parent_tidptr, current implementation fails with EINVAL if that value is non-zero.

If CLONE_PIDFD is supported and fd 0 is closed, then mandatory pidfd == 0 pointed by parent_tidptr also remains unchanged, which effectively means that userspace must either check CLONE_PIDFD support beforehand or ensure that fd 0 is not closed when invoking CLONE_PIDFD.

The check for pidfd == 0 was introduced during v5.2 release cycle by commit b3e583825266 ("clone: add CLONE_PIDFD") to ensure that CLONE_PIDFD could be potentially extended by passing in flags through the return argument.

However, that extension would look horrendous, and with introduction of clone3 syscall in v5.3 there is no need to extend legacy clone syscall this way.

Quoted 5 lines

So remove the pidfd == 0 check. Userspace that needs to be portable to kernels without CLONE_PIDFD support is advised to initialize pidfd with -1 and check the pidfd value returned by CLONE_PIDFD.

Signed-off-by Dmitry V. Levin <[email protected]>

Reviewed-by Christian Brauner <[email protected]>

Thank you Dmitry, queueing this up for rc7.

Quoted 6 lines

This is now sitting in

https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/commit/?h=fixes&id=43754d05f235dd1b6c7f8ab9f42007770d721f10

I reformulated the commit message a bit and gave it a Fixes tag. Dmitry, if you want you can take a look and tell me if that's acceptable to you.

s/Old kernel that only support/Old kernels that only support/

Out of curiosity: what makes the new flag different than say CLONE_NEWCGROUP or any new clone flag that got introduced? CLONE_NEWCGROUP too would not be detectable apart from the method I gave you above; same for other clone flags. Why are you so keen on being able to detect this flag when other flags didn't seem to matter that much.

On Sat, Jun 22, 2019 at 12:13:39AM +0200, Christian Brauner wrote:
[...]

Quoted 5 lines

Out of curiosity: what makes the new flag different than say CLONE_NEWCGROUP or any new clone flag that got introduced? CLONE_NEWCGROUP too would not be detectable apart from the method I gave you above; same for other clone flags. Why are you so keen on being able to detect this flag when other flags didn't seem to matter that much.

I wasn't following uapi changes closely enough those days. ;)