From: Wang C. <wan...@cn...> - 2010-09-07 07:49:11
|
If clone is called with flag CLONE_UNTRACED, I think we should not set CLONE_PTRACE flag in its arguments. * defs.h [LINUX]: New flag to indicate that the clone is called with CLONE_UNTRACED, and we do not setbpt on it. * util.c [LINUX]: Define CLONE_UNTRACED. [LINUX] (setbpt): Check it. * process.c [LINUX] (internal_fork): Check for TCB_CLONE_UNTRACED. Signed-off-by: Wang Chao <wan...@cn...> --- defs.h | 1 + process.c | 7 +++++++ util.c | 7 +++++++ 3 files changed, 15 insertions(+), 0 deletions(-) diff --git a/defs.h b/defs.h index 4b9c29e..cee8551 100644 --- a/defs.h +++ b/defs.h @@ -383,6 +383,7 @@ struct tcb { # endif # define TCB_CLONE_THREAD 010000 /* CLONE_THREAD set in creating syscall */ # define TCB_GROUP_EXITING 020000 /* TCB_EXITING was exit_group, not _exit */ +# define TCB_CLONE_UNTRACED 040000 /* clone a new child with CLONE_UNTRACED */ # include <sys/syscall.h> # ifndef __NR_exit_group # /* Hack: Most headers around are too old to have __NR_exit_group. */ diff --git a/process.c b/process.c index 8af960a..f3f0901 100644 --- a/process.c +++ b/process.c @@ -809,6 +809,13 @@ internal_fork(struct tcb *tcp) if (!(tcp->flags & TCB_FOLLOWFORK)) return 0; + if (tcp->flags & TCB_CLONE_UNTRACED) { + tcp->flags &= ~TCB_CLONE_UNTRACED; + if (!(tcp->nchildren > 0)) + tcp->flags &= ~TCB_FOLLOWFORK; + return 0; + } + bpt = tcp->flags & TCB_BPTSET; if (syserror(tcp)) { diff --git a/util.c b/util.c index 07cbfde..380c16d 100644 --- a/util.c +++ b/util.c @@ -1271,6 +1271,9 @@ printcall(struct tcb *tcp) # ifndef CLONE_STOPPED # define CLONE_STOPPED 0x02000000 # endif +# ifndef CLONE_UNTRACED +# define CLONE_UNTRACED 0x00800000 +# endif # ifdef IA64 @@ -1537,6 +1540,10 @@ setbpt(struct tcb *tcp) clear also CLONE_VM but only in the CLONE_VFORK case as otherwise we would break pthread_create. */ + if (tcp->u_arg[arg0_index] & CLONE_UNTRACED) { + tcp->flags |= TCB_CLONE_UNTRACED; + return 0; + } if ((arg_setup (tcp, &state) < 0 || set_arg0 (tcp, &state, (tcp->u_arg[arg0_index] | CLONE_PTRACE) -- 1.6.5.2 |
From: Roland M. <ro...@re...> - 2010-09-08 01:06:18
|
> If clone is called with flag CLONE_UNTRACED, I think we should > not set CLONE_PTRACE flag in its arguments. I can guess what your rationale for this might be, but I think you should give it explicitly. On its face, there is no good reason why any one clone call ought to be treated different from another, in current kernels. In practice, nobody ever uses CLONE_UNTRACED today. AFAIK, it was only ever used by some old versions of linuxthreads (which has been obsolete for ages), and even those only wanted it for some ancient kernel versions where the ptrace wait interference was even worse than it is on non-ancient kernels. I imagine that your motivation is to make strace's -f behavior today match what it would do if implemented via PTRACE_O_TRACECLONE, where we'd give up the option of tracing CLONE_UNTRACED children. If that's so, then just be clear about it. Now, to the change itself. I don't see why you've added a flag and everything. What doesn't work about just doing this: diff --git a/process.c b/process.c index 59d7ba0..ebba77b 100644 --- a/process.c +++ b/process.c @@ -796,7 +796,7 @@ int internal_fork(struct tcb *tcp) { if (entering(tcp)) { - if (!followfork) + if (!followfork || (tcp->u_arg[ARG_FLAGS] & CLONE_UNTRACED)) return 0; fork_tcb(tcp); if (setbpt(tcp) < 0) ? Thanks, Roland |
From: Wang C. <wan...@cn...> - 2010-09-08 03:56:00
|
Sent on 2010-9-8 9:06, Roland McGrath wrote: > On its face, there is no good reason why any one clone call ought to be > treated different from another, in current kernels. In practice, nobody > ever uses CLONE_UNTRACED today. AFAIK, it was only ever used by some > old versions of linuxthreads (which has been obsolete for ages), and > even those only wanted it for some ancient kernel versions where the > ptrace wait interference was even worse than it is on non-ancient kernels. > > I imagine that your motivation is to make strace's -f behavior today > match what it would do if implemented via PTRACE_O_TRACECLONE, where > we'd give up the option of tracing CLONE_UNTRACED children. If that's > so, then just be clear about it. Yes, so it is. Actually, I am not sure whether there are someone or some librarys using such flag. But I think since this flag still exists in kernel, and maybe will not disappear in the near future, keep the same logic with kernel may be a good idea. > > Now, to the change itself. > > I don't see why you've added a flag and everything. > What doesn't work about just doing this: > > diff --git a/process.c b/process.c > index 59d7ba0..ebba77b 100644 > --- a/process.c > +++ b/process.c > @@ -796,7 +796,7 @@ int > internal_fork(struct tcb *tcp) > { > if (entering(tcp)) { > - if (!followfork) > + if (!followfork || (tcp->u_arg[ARG_FLAGS] & CLONE_UNTRACED)) > return 0; > fork_tcb(tcp); > if (setbpt(tcp) < 0) > Yeah, I forgot the macro ARG_FLAGS. This change seems much simpler and it's a good idea. Also we may add this check on syscall exit. Thanks, Wang Chao |
From: Roland M. <ro...@re...> - 2010-09-08 05:25:10
|
> Yes, so it is. Actually, I am not sure whether there are someone or some > librarys using such flag. But I think since this flag still exists in > kernel, and maybe will not disappear in the near future, keep the same > logic with kernel may be a good idea. I don't object to that. > Yeah, I forgot the macro ARG_FLAGS. This change seems much simpler and > it's a good idea. Also we may add this check on syscall exit. I see no reason for any additional check. TCB_FOLLOWFORK won't be set and everything else should flow correctly from there. Thanks, Roland |
From: Wang C. <wan...@cn...> - 2010-09-08 05:52:38
|
Sent on 2010-9-8 13:24, Roland McGrath wrote: >> Yeah, I forgot the macro ARG_FLAGS. This change seems much simpler and >> it's a good idea. Also we may add this check on syscall exit. > > I see no reason for any additional check. TCB_FOLLOWFORK won't be set > and everything else should flow correctly from there. > But I'm wondering the following situation though maybe it's not common: we've already traced a process A and its children, A should have TCB_FOLLOWFORK set. And then if A calls clone with CLONE_UNTRACED, I think strace will still allocate tcb for the new child which we don't trace. I'm sorry if I don't understand correctly. Thanks, Wang Chao |
From: Roland M. <ro...@re...> - 2010-09-10 08:06:05
|
> But I'm wondering the following situation though maybe it's not common: > we've already traced a process A and its children, A should have TCB_FOLLOWFORK > set. And then if A calls clone with CLONE_UNTRACED, I think strace will still > allocate tcb for the new child which we don't trace. > > I'm sorry if I don't understand correctly. You are quite right. The setting of the TCB_FOLLOWFORK bit got moved around in a previous cleanup, and it now actually has no useful meaning at all, it's essentially always set. But it is already tested in the places we need for the related new purpose of this change. So I think the patch below would indeed do the trick. What do you think? Thanks, Roland ====== diff --git a/process.c b/process.c index 8af960a..50f5ae8 100644 --- a/process.c +++ b/process.c @@ -796,7 +796,8 @@ int internal_fork(struct tcb *tcp) { if (entering(tcp)) { - if (!followfork) + tcp->flags &= ~TCB_FOLLOWFORK; + if (!followfork || (tcp->u_arg[ARG_FLAGS] & CLONE_UNTRACED)) return 0; fork_tcb(tcp); if (setbpt(tcp) < 0) |
From: Wang C. <wan...@cn...> - 2010-09-10 08:51:42
|
Sent on 2010-9-10 16:05, Roland McGrath wrote: > > diff --git a/process.c b/process.c > index 8af960a..50f5ae8 100644 > --- a/process.c > +++ b/process.c > @@ -796,7 +796,8 @@ int > internal_fork(struct tcb *tcp) > { > if (entering(tcp)) { > - if (!followfork) > + tcp->flags &= ~TCB_FOLLOWFORK; > + if (!followfork || (tcp->u_arg[ARG_FLAGS] & CLONE_UNTRACED)) > return 0; > fork_tcb(tcp); > if (setbpt(tcp) < 0) I quite agree with the above change. Thanks, Wang Chao |
From: Roland M. <ro...@re...> - 2010-09-10 09:26:38
|
> Sent on 2010-9-10 16:05, Roland McGrath wrote: > > > > diff --git a/process.c b/process.c > > index 8af960a..50f5ae8 100644 > > --- a/process.c > > +++ b/process.c > > @@ -796,7 +796,8 @@ int > > internal_fork(struct tcb *tcp) > > { > > if (entering(tcp)) { > > - if (!followfork) > > + tcp->flags &= ~TCB_FOLLOWFORK; > > + if (!followfork || (tcp->u_arg[ARG_FLAGS] & CLONE_UNTRACED)) > > return 0; > > fork_tcb(tcp); > > if (setbpt(tcp) < 0) > > I quite agree with the above change. Does that mean you tested it? |
From: Wang C. <wan...@cn...> - 2010-09-13 03:41:49
|
Sent on 2010-9-10 17:26, Roland McGrath wrote: >> I quite agree with the above change. > > Does that mean you tested it? > Very sorry for my last reply, actually there are indeed some problems here. For syscall who has no arguments such as vfork accessing tcp->u_arg here may result an undetermined value. What we want to change only occur in clone syscall, so I think we may add one more check here, just like the patch below. What do you think? Thanks, Wang Chao ====== diff --git a/process.c b/process.c index 8af960a..d587379 100644 --- a/process.c +++ b/process.c @@ -796,8 +796,12 @@ int internal_fork(struct tcb *tcp) { if (entering(tcp)) { + tcp->flags &= ~TCB_FOLLOWFORK; if (!followfork) return 0; + if ((known_scno(tcp) == SYS_clone) && + (tcp->u_arg[ARG_FLAGS] & CLONE_UNTRACED)) + return 0; fork_tcb(tcp); if (setbpt(tcp) < 0) return 0; |
From: Roland M. <ro...@re...> - 2010-09-14 02:29:27
|
> Very sorry for my last reply, actually there are indeed some > problems here. For syscall who has no arguments such as vfork > accessing tcp->u_arg here may result an undetermined value. Oh, that makes sense. I almost thought this was also a problem for the existing use of ARG_FLAGS in internal_fork. But, actually that code only happens in the exiting case, where setbpt already ran in the entering case and changed the registers. > What we want to change only occur in clone syscall, so I think > we may add one more check here, just like the patch below. > > What do you think? On ia64, it's clone2. And there might be others later. It might be better to test sysent[tcp->scno].sys_func == sys_clone. Thanks, Roland |
From: Wang C. <wan...@cn...> - 2010-09-15 05:20:39
|
If clone is called with flag CLONE_UNTRACED, I think we should not set CLONE_PTRACE flag in its arguments. * process.c [LINUX] (internal_fork): If the syscall is clone and CLONE_UNTRACED flag is set in its arguments, do not trace this new child. Signed-off-by: Wang Chao <wan...@cn...> --- process.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/process.c b/process.c index 8999fc6..851297c 100644 --- a/process.c +++ b/process.c @@ -796,8 +796,12 @@ int internal_fork(struct tcb *tcp) { if (entering(tcp)) { + tcp->flags &= ~TCB_FOLLOWFORK; if (!followfork) return 0; + if ((sysent[tcp->scno].sys_func == sys_clone) && + (tcp->u_arg[ARG_FLAGS] & CLONE_UNTRACED)) + return 0; fork_tcb(tcp); if (setbpt(tcp) < 0) return 0; -- 1.6.5.2 |
From: Wang C. <wan...@cn...> - 2010-09-16 03:19:39
|
If clone is called with flag CLONE_UNTRACED, to be consistent with option PTRACE_O_TRACECLONE, we should not set CLONE_PTRACE flag on its arguments. * process.c [LINUX] (internal_fork): Check the syscall and arguments. Signed-off-by: Wang Chao <wan...@cn...> --- process.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-) diff --git a/process.c b/process.c index 8999fc6..f246bcb 100644 --- a/process.c +++ b/process.c @@ -796,8 +796,16 @@ int internal_fork(struct tcb *tcp) { if (entering(tcp)) { + tcp->flags &= ~TCB_FOLLOWFORK; if (!followfork) return 0; + /* In occasion of using PTRACE_O_TRACECLONE, we won't see the + * new child if clone is called with flag CLONE_UNTRACED, so + * we keep the same logic with that option and dont't trace it. + */ + if ((sysent[tcp->scno].sys_func == sys_clone) && + (tcp->u_arg[ARG_FLAGS] & CLONE_UNTRACED)) + return 0; fork_tcb(tcp); if (setbpt(tcp) < 0) return 0; -- 1.6.5.2 |
From: Roland M. <ro...@re...> - 2010-09-15 06:10:10
|
> If clone is called with flag CLONE_UNTRACED, I think we should > not set CLONE_PTRACE flag in its arguments. Please add a comment in the code about why this is the desireable behavior. AFAIK the only reason is something like, "We won't see this clone child at all if we rely on PTRACE_O_TRACECLONE, so be consistent by never tracing it." Otherwise, this is fine with me, assuming you have tested it. Thanks, Roland |
From: Roland M. <ro...@re...> - 2010-09-17 19:16:57
|
> + /* In occasion of using PTRACE_O_TRACECLONE, we won't see the The comment format used in strace is: /* * Comment. */ > + * we keep the same logic with that option and dont't trace it. Typo there in "don't". Thanks, Roland |