Re: [PATCH] samples: make pidfd-metadata fail gracefully on older kernels

Re: [PATCH] samples: make pidfd-metadata fail gracefully on older kernels

[Dmitry V. Levin]

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

On Thu, Jun 20, 2019 at 12:31:06PM +0200, Christian Brauner wrote:
> On Thu, Jun 20, 2019 at 06:11:44AM +0300, Dmitry V. Levin wrote:
> > 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 <ldv@altlinux.org>
> > ---
> >  samples/pidfd/pidfd-metadata.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/samples/pidfd/pidfd-metadata.c b/samples/pidfd/pidfd-metadata.c
> > index 14b454448429..ff109fdac3a5 100644
> > --- a/samples/pidfd/pidfd-metadata.c
> > +++ b/samples/pidfd/pidfd-metadata.c
> > @@ -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;
> 
> 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.

> 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.


-- 
ldv

Re: [PATCH] samples: make pidfd-metadata fail gracefully on older kernels

[Christian Brauner]

On Thu, Jun 20, 2019 at 02:00:37PM +0300, Dmitry V. Levin wrote:
> Cc'ed more people as the issue is not just with the example but
> with the interface itself.
> 
> On Thu, Jun 20, 2019 at 12:31:06PM +0200, Christian Brauner wrote:
> > On Thu, Jun 20, 2019 at 06:11:44AM +0300, Dmitry V. Levin wrote:
> > > 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 <ldv@altlinux.org>
> > > ---
> > >  samples/pidfd/pidfd-metadata.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/samples/pidfd/pidfd-metadata.c b/samples/pidfd/pidfd-metadata.c
> > > index 14b454448429..ff109fdac3a5 100644
> > > --- a/samples/pidfd/pidfd-metadata.c
> > > +++ b/samples/pidfd/pidfd-metadata.c
> > > @@ -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;
> > 
> > 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.
> 
> > 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.

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?

> 
> 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.

Thanks!
Christian

Re: [PATCH] samples: make pidfd-metadata fail gracefully on older kernels

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]

On Thu, Jun 20, 2019 at 01:10:37PM +0200, Christian Brauner wrote:
> On Thu, Jun 20, 2019 at 02:00:37PM +0300, Dmitry V. Levin wrote:
> > Cc'ed more people as the issue is not just with the example but
> > with the interface itself.
> > 
> > On Thu, Jun 20, 2019 at 12:31:06PM +0200, Christian Brauner wrote:
> > > On Thu, Jun 20, 2019 at 06:11:44AM +0300, Dmitry V. Levin wrote:
> > > > 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 <ldv@altlinux.org>
> > > > ---
> > > >  samples/pidfd/pidfd-metadata.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/samples/pidfd/pidfd-metadata.c b/samples/pidfd/pidfd-metadata.c
> > > > index 14b454448429..ff109fdac3a5 100644
> > > > --- a/samples/pidfd/pidfd-metadata.c
> > > > +++ b/samples/pidfd/pidfd-metadata.c
> > > > @@ -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;
> > > 
> > > 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.
> > 
> > > 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.
> 
> 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?

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

> > 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.

... I'd prefer this variant.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] samples: make pidfd-metadata fail gracefully on older kernels

[Christian Brauner]

On Fri, Jun 21, 2019 at 08:06:14PM +0300, Dmitry V. Levin wrote:
> On Thu, Jun 20, 2019 at 01:10:37PM +0200, Christian Brauner wrote:
> > On Thu, Jun 20, 2019 at 02:00:37PM +0300, Dmitry V. Levin wrote:
> > > Cc'ed more people as the issue is not just with the example but
> > > with the interface itself.
> > > 
> > > On Thu, Jun 20, 2019 at 12:31:06PM +0200, Christian Brauner wrote:
> > > > On Thu, Jun 20, 2019 at 06:11:44AM +0300, Dmitry V. Levin wrote:
> > > > > 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 <ldv@altlinux.org>
> > > > > ---
> > > > >  samples/pidfd/pidfd-metadata.c | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/samples/pidfd/pidfd-metadata.c b/samples/pidfd/pidfd-metadata.c
> > > > > index 14b454448429..ff109fdac3a5 100644
> > > > > --- a/samples/pidfd/pidfd-metadata.c
> > > > > +++ b/samples/pidfd/pidfd-metadata.c
> > > > > @@ -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;
> > > > 
> > > > 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.
> > > 
> > > > 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.
> > 
> > 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?
> 
> Yes, this is feasible, but the cost is extra syscall for new kernels
> and more complicated userspace code, so...

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.
(Again, mere curiosity.)

> 
> > > 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.
> 
> ... I'd prefer this variant.

Please send a patch for review.

Christian

[PATCH 1/2] CLONE_PIDFD: do not use the value pointed by parent_tidptr

[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.

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 kernel/fork.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 75675b9bf6df..39a3adaa4ad1 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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);
 	}
 
 	/*
-- 
ldv

[PATCH 2/2] samples: make pidfd-metadata fail gracefully on older kernels

[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 <ldv@altlinux.org>
---
 samples/pidfd/pidfd-metadata.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/samples/pidfd/pidfd-metadata.c b/samples/pidfd/pidfd-metadata.c
index 14b454448429..c459155daf9a 100644
--- a/samples/pidfd/pidfd-metadata.c
+++ b/samples/pidfd/pidfd-metadata.c
@@ -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);
-- 
ldv

Re: [PATCH 2/2] samples: make pidfd-metadata fail gracefully on older kernels

[Christian Brauner]

On Sun, Jun 23, 2019 at 02:28:00PM +0300, Dmitry V. Levin wrote:
> 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 <ldv@altlinux.org>

Reviewed-by: Christian Brauner <christian@brauner.io>

Thank you Dmitry, queueing this up for rc7.

> ---
>  samples/pidfd/pidfd-metadata.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/samples/pidfd/pidfd-metadata.c b/samples/pidfd/pidfd-metadata.c
> index 14b454448429..c459155daf9a 100644
> --- a/samples/pidfd/pidfd-metadata.c
> +++ b/samples/pidfd/pidfd-metadata.c
> @@ -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);
> -- 
> ldv

Re: [PATCH 1/2] CLONE_PIDFD: do not use the value pointed by parent_tidptr

[Christian Brauner]

On Sun, Jun 23, 2019 at 02:27:17PM +0300, Dmitry V. Levin wrote:
> 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 <ldv@altlinux.org>

Reviewed-by: Christian Brauner <christian@brauner.io>

Thank you Dmitry, queueing this up for rc7.

> ---
>  kernel/fork.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 75675b9bf6df..39a3adaa4ad1 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -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);
>  	}
>  
>  	/*
> -- 
> ldv

Re: [PATCH 1/2] CLONE_PIDFD: do not use the value pointed by parent_tidptr

[Christian Brauner]

On Mon, Jun 24, 2019 at 11:49:40AM +0200, Christian Brauner wrote:
> On Sun, Jun 23, 2019 at 02:27:17PM +0300, Dmitry V. Levin wrote:
> > 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 <ldv@altlinux.org>
> 
> Reviewed-by: Christian Brauner <christian@brauner.io>
> 
> 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.

Thanks!
Christian

Re: [PATCH 1/2] CLONE_PIDFD: do not use the value pointed by parent_tidptr

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 2126 bytes --]

On Mon, Jun 24, 2019 at 01:59:43PM +0200, Christian Brauner wrote:
> On Mon, Jun 24, 2019 at 11:49:40AM +0200, Christian Brauner wrote:
> > On Sun, Jun 23, 2019 at 02:27:17PM +0300, Dmitry V. Levin wrote:
> > > 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 <ldv@altlinux.org>
> > 
> > Reviewed-by: Christian Brauner <christian@brauner.io>
> > 
> > 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.

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

Besides that, fine with me.  Thanks.


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/2] CLONE_PIDFD: do not use the value pointed by parent_tidptr

[Christian Brauner]

On Mon, Jun 24, 2019 at 04:45:31PM +0300, Dmitry V. Levin wrote:
> On Mon, Jun 24, 2019 at 01:59:43PM +0200, Christian Brauner wrote:
> > On Mon, Jun 24, 2019 at 11:49:40AM +0200, Christian Brauner wrote:
> > > On Sun, Jun 23, 2019 at 02:27:17PM +0300, Dmitry V. Levin wrote:
> > > > 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 <ldv@altlinux.org>
> > > 
> > > Reviewed-by: Christian Brauner <christian@brauner.io>
> > > 
> > > 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.
> 
> s/Old kernel that only support/Old kernels that only support/

Fixed.

Thanks!
Christian

Re: [PATCH] samples: make pidfd-metadata fail gracefully on older kernels

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

On Sat, Jun 22, 2019 at 12:13:39AM +0200, Christian Brauner wrote:
[...]
> 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. ;)


-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] samples: make pidfd-metadata fail gracefully on older kernels

[Christian Brauner]

On Sun, Jun 23, 2019 at 02:32:30PM +0300, Dmitry V. Levin wrote:
> On Sat, Jun 22, 2019 at 12:13:39AM +0200, Christian Brauner wrote:
> [...]
> > 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. ;)

(Seriously, you had one job. :) I'm joking of course.)

What you want makes sense to me overall. This way userspace can decide
easier whether to manage a process through a pidfd or needs to fallback
to a pid.

Christian