Date: Wed, 27 Aug 2014 18:54:32 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, John Baldwin <jhb@freebsd.org>, Mateusz Guzik <mjg@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org, John-Mark Gurney <jmg@funkthat.com> Subject: Re: svn commit: r270444 - in head/sys: kern sys Message-ID: <20140827165432.GA28581@dft-labs.eu> In-Reply-To: <20140826215522.GG2737@kib.kiev.ua> References: <201408240904.s7O949sI083660@svn.freebsd.org> <201408261509.26815.jhb@freebsd.org> <20140826193210.GL71691@funkthat.com> <201408261723.10854.jhb@freebsd.org> <20140826215522.GG2737@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Aug 27, 2014 at 12:55:22AM +0300, Konstantin Belousov wrote: > On Tue, Aug 26, 2014 at 05:23:10PM -0400, John Baldwin wrote: > > On Tuesday, August 26, 2014 3:32:10 pm John-Mark Gurney wrote: > > > John Baldwin wrote this message on Tue, Aug 26, 2014 at 15:09 -0400: > > > > On Monday, August 25, 2014 6:30:34 pm John-Mark Gurney wrote: > > > > > John Baldwin wrote this message on Mon, Aug 25, 2014 at 13:35 -0400: > > > > > > On Monday, August 25, 2014 07:02:41 PM Mateusz Guzik wrote: > > > > > > > On Mon, Aug 25, 2014 at 10:23:19AM -0400, John Baldwin wrote: > > > > > > > > On Sunday, August 24, 2014 09:04:09 AM Mateusz Guzik wrote: > > > > > > > > > Author: mjg > > > > > > > > > Date: Sun Aug 24 09:04:09 2014 > > > > > > > > > New Revision: 270444 > > > > > > > > > URL: http://svnweb.freebsd.org/changeset/base/270444 > > > > > > > > > > > > > > > > > > Log: > > > > > > > > > Fix getppid for traced processes. > > > > > > > > > > > > > > > > > > Traced processes always have the tracer set as the parent. > > > > > > > > > Utilize proc_realparent to obtain the right process when needed. > > > > > > > > > > > > > > > > Are you sure this won't break things? I know of several applications that > > > > > > > > expect a debugger to be the parent when attached and change behavior as a > > > > > > > > result (e.g. inserting a breakpoint on an assertion failure rather than > > > > > > > > generating a core). > Shouldn't such applications use a breakpoint instruction like INT3 > unconditionally then ? Detection of the attached debugger is inherently > racy, the debugger might have detached after the test. This, and the > fact that default action for the SIGTRAP is coredumping. > > > > > > > > > > > > > > > Well, this is what linux and solaris do. > > > > > > > > > > > > Interesting. > > > > > > > > > > > > > I don't feel strongly about this change. If you really want I'm happy to > > > > > > > revert. > > > > > > > > > > > > In general I'd like to someday have the debugger-debuggee relationship not > > > > > > override parent-child and this is a step in that direction. However, this > > > > > > will break existing applications, so this needs to be clearly documented in > > > > > > the release notes. In addition, we should probably advertise how a process > > > > > > can correctly determine if it is being run under a debugger (right now you can > > > > > > do 'getppid()' and use strcmp or strstr on the p_comm of that pid so you can > > > > > > do different things for "gdb" vs "gcore", etc. so just checking P_TRACED from > > > > > > kinfo_proc wouldn't be equivalent in functionality) > > > > > > > > > > But what about when you attach gdb to a running process... That > > > > > doesn't magicly make the now debugged process a child of gdb does it? > > > > > > > > % cat hello.c > > > > #include <stdio.h> > > > > > > > > int > > > > main() > > > > { > > > > printf("hello world\n"); > > > > (void)getchar(); > > > > return (0); > > > > } > > > > % cc -g hello.c -o hello > > > > % ./hello > > > > hello world > > > > load: 9.81 cmd: hello 42599 [ttyin] 1.67r 0.00u 0.00s 0% 1056k > > > > > > > > < different window > > > > > > > > > % ps -O ppid -p `pgrep hello` > > > > PID PPID TT STAT TIME COMMAND > > > > 42599 5340 16 I+ 0:00.00 ./hello > > > > % gdb hello `pgrep hello` > > > > GNU gdb 6.1.1 [FreeBSD] > > > > ... > > > > (gdb) <hit Ctrl-Z> > > > > Suspended > > > > % ps -O ppid -p `pgrep hello` > > > > PID PPID TT STAT TIME COMMAND > > > > 42599 45079 16 TX+ 0:00.00 ./hello > > > > > > Wow, learn something new every day... > > > > > > But doesn't that break apps that use getppid to signal their parent > > > that forked them? > > > > Until mjg@'s commit, yes. It's been that way in FreeBSD at least for > > as long as I can remember. Certainly back to 4.x. > > The ps(1) trick continues to work after the commit, since kern_proc > sysctl directly accesses p_pptr to fill ki_ppid. I simply forgot about > it during the review. > Fixing it requires taking proctree_lock when it is otherwise not needed, which is somewhat unfortunate. > Anyway, checking the parent pid is definitely not the right way to > see if the process is under ptrace debugging. What if the parent > is the debugger ? The p_flag AKA ki_flag P_TRACED bit seems to be > the correct indicator. It was stated they want to do something based on the name of the tracer. Whether that makes sense or not is unclear for me. Either way, having a way of checking who traces is a nice thing to retain. So how about the following: diff --git a/bin/ps/keyword.c b/bin/ps/keyword.c index 3a0c323..38a9934 100644 --- a/bin/ps/keyword.c +++ b/bin/ps/keyword.c @@ -157,6 +157,7 @@ static VAR var[] = { {"tdnam", "TDNAM", NULL, LJUST, tdnam, 0, CHAR, NULL, 0}, {"time", "TIME", NULL, USER, cputime, 0, CHAR, NULL, 0}, {"tpgid", "TPGID", NULL, 0, kvar, KOFF(ki_tpgid), UINT, PIDFMT, 0}, + {"tracer", "TRACER", NULL, 0, kvar, KOFF(ki_tracer), UINT, PIDFMT, 0}, {"tsid", "TSID", NULL, 0, kvar, KOFF(ki_tsid), UINT, PIDFMT, 0}, {"tsiz", "TSIZ", NULL, 0, kvar, KOFF(ki_tsize), PGTOK, "ld", 0}, {"tt", "TT ", NULL, 0, tname, 0, CHAR, NULL, 0}, diff --git a/bin/ps/ps.1 b/bin/ps/ps.1 index d8e56fb..294ecf9 100644 --- a/bin/ps/ps.1 +++ b/bin/ps/ps.1 @@ -29,7 +29,7 @@ .\" @(#)ps.1 8.3 (Berkeley) 4/18/94 .\" $FreeBSD$ .\" -.Dd August 7, 2014 +.Dd August 27, 2014 .Dt PS 1 .Os .Sh NAME @@ -665,6 +665,8 @@ accumulated CPU time, user + system (alias .Cm cputime ) .It Cm tpgid control terminal process group ID +.It Cm tracer +tracer process ID .\".It Cm trss .\"text resident set size (in Kbytes) .It Cm tsid diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c index 6689186..530969a 100644 --- a/sys/kern/kern_proc.c +++ b/sys/kern/kern_proc.c @@ -791,6 +791,8 @@ fill_kinfo_proc_only(struct proc *p, struct kinfo_proc *kp) struct ucred *cred; struct sigacts *ps; + /* For proc_realparent. */ + sx_assert(&proctree_lock, SX_LOCKED); PROC_LOCK_ASSERT(p, MA_OWNED); bzero(kp, sizeof(*kp)); @@ -920,7 +922,9 @@ fill_kinfo_proc_only(struct proc *p, struct kinfo_proc *kp) kp->ki_acflag = p->p_acflag; kp->ki_lock = p->p_lock; if (p->p_pptr) - kp->ki_ppid = p->p_pptr->p_pid; + kp->ki_ppid = proc_realparent(p)->p_pid; + if (p->p_flag & P_TRACED) + kp->ki_tracer = p->p_pptr->p_pid; } /* @@ -1287,10 +1291,11 @@ sysctl_kern_proc(SYSCTL_HANDLER_ARGS) error = sysctl_wire_old_buffer(req, 0); if (error) return (error); + sx_slock(&proctree_lock); error = pget((pid_t)name[0], PGET_CANSEE, &p); - if (error != 0) - return (error); - error = sysctl_out_proc(p, req, flags, 0); + if (error == 0) + error = sysctl_out_proc(p, req, flags, 0); + sx_sunlock(&proctree_lock); return (error); } @@ -1318,6 +1323,7 @@ sysctl_kern_proc(SYSCTL_HANDLER_ARGS) error = sysctl_wire_old_buffer(req, 0); if (error != 0) return (error); + sx_slock(&proctree_lock); sx_slock(&allproc_lock); for (doingzomb=0 ; doingzomb < 2 ; doingzomb++) { if (!doingzomb) @@ -1422,11 +1428,13 @@ sysctl_kern_proc(SYSCTL_HANDLER_ARGS) error = sysctl_out_proc(p, req, flags, doingzomb); if (error) { sx_sunlock(&allproc_lock); + sx_sunlock(&proctree_lock); return (error); } } } sx_sunlock(&allproc_lock); + sx_sunlock(&proctree_lock); return (0); } diff --git a/sys/sys/user.h b/sys/sys/user.h index f7b18df..6775ff7 100644 --- a/sys/sys/user.h +++ b/sys/sys/user.h @@ -84,7 +84,7 @@ * it in two places: function fill_kinfo_proc in sys/kern/kern_proc.c and * function kvm_proclist in lib/libkvm/kvm_proc.c . */ -#define KI_NSPARE_INT 7 +#define KI_NSPARE_INT 6 #define KI_NSPARE_LONG 12 #define KI_NSPARE_PTR 6 @@ -187,6 +187,7 @@ struct kinfo_proc { */ char ki_sparestrings[50]; /* spare string space */ int ki_spareints[KI_NSPARE_INT]; /* spare room for growth */ + int ki_tracer; /* Pid of tracing process */ int ki_flag2; /* P2_* flags */ int ki_fibnum; /* Default FIB number */ u_int ki_cr_flags; /* Credential flags */ -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140827165432.GA28581>