On Sun, Jul 14, 2019 at 05:10:08PM +0300, Dmitry V. Levin wrote: > On Sun, Jul 14, 2019 at 02:17:25PM +0200, Christian Brauner wrote: > > On Sun, Jul 14, 2019 at 03:02:06PM +0300, Dmitry V. Levin wrote: > > > The introduction of clone3 syscall accidentally broke CLONE_PIDFD > > > support in traditional clone syscall on compat x86 and those > > > architectures that use do_fork to implement clone syscall. > > > > > > This bug was found by strace test suite. > > > > > > Link: https://strace.io/logs/strace/2019-07-12 > > > Fixes: 7f192e3cd316 ("fork: add clone3") > > > Bisected-and-tested-by: Anatoly Pugachev <mator...@gmail.com> > > > Signed-off-by: Dmitry V. Levin <l...@altlinux.org> > > > > Good catch! Thank you Dmitry. > > > > One change request below. > > > > > --- > > > arch/x86/ia32/sys_ia32.c | 1 + > > > kernel/fork.c | 1 + > > > 2 files changed, 2 insertions(+) > > > > > > diff --git a/arch/x86/ia32/sys_ia32.c b/arch/x86/ia32/sys_ia32.c > > > index 64a6c952091e..98754baf411a 100644 > > > --- a/arch/x86/ia32/sys_ia32.c > > > +++ b/arch/x86/ia32/sys_ia32.c > > > @@ -239,6 +239,7 @@ COMPAT_SYSCALL_DEFINE5(x86_clone, unsigned long, > > > clone_flags, > > > { > > > struct kernel_clone_args args = { > > > .flags = (clone_flags & ~CSIGNAL), > > > + .pidfd = parent_tidptr, > > > .child_tid = child_tidptr, > > > .parent_tid = parent_tidptr, > > > .exit_signal = (clone_flags & CSIGNAL), > > > diff --git a/kernel/fork.c b/kernel/fork.c > > > index 8f3e2d97d771..2c3cbad807b6 100644 > > > --- a/kernel/fork.c > > > +++ b/kernel/fork.c > > > @@ -2417,6 +2417,7 @@ long do_fork(unsigned long clone_flags, > > > { > > > struct kernel_clone_args args = { > > > .flags = (clone_flags & ~CSIGNAL), > > > + .pidfd = parent_tidptr, > > > .child_tid = child_tidptr, > > > .parent_tid = parent_tidptr, > > > .exit_signal = (clone_flags & CSIGNAL), > > > -- > > > > Both of these legacy clone helpers need to make CLONE_PIDFD and > > CLONE_PARENT_SETTID incompatible, i.e. could you please add a helper to > > kernel/fork.c: > > > > bool legacy_clone_args_valid(const struct kernel_clone_args *kargs) > > { > > /* clone(CLONE_PIDFD) uses parent_tidptr to return a pidfd */ > > if ((kargs->flags & CLONE_PIDFD) && (kargs->flags & > > CLONE_PARENT_SETTID)) > > return false; > > } > > > > and export it and use it in ia32 too? > > copy_process already performs the check, isn't this enough?
No it doesn't anymore. clone3() allows CLONE_PIDFD + CLONE_PARENT_SETTID since struct clone_args has a dedicated pidfd return argument. > Also, the check in sys_clone looks redundant and I was going to suggest > its removal. See above.
