dtrace discuss - Jan 2008 - 6593259: libdtrace should prefer pid probes to breakpoints

I''ve been looking at this bug.  What I think needs to be done is the
following:

- In dt_proc_control(), create a separate dtrace handle with the
destructive option set.
- In dt_proc_bp_create() (or the equivalent-but-renamed function), do
something similar to the following instead of creating a breakpoint
(where dtp is this new handle):

                sprintf(dprog, "pid%d::-:%x {stop();}",
(int)dpr->dpr_pid,
                    dbp->dbp_addr);

                if ((pgp = dtrace_program_strcompile(dtp, dprog,
                    DTRACE_PROBESPEC_NAME, DTRACE_C_NOLIBS, 0, NULL))
                    == NULL) {
                        dt_dprintf("pid %d: failed to compile pid
breakpoint "
                            "at 0x%x\n", (int)dpr->dpr_pid,
dbp->dbp_addr);
                } else if (dtrace_program_exec(dtp, pgp, NULL)
                    == -1) {
                        dt_dprintf("pid %d: failed to create pid breakpoint
"
                            "at 0x%x\n", (int)dpr->dpr_pid,
dbp->dbp_addr);
                } else {
                        dbp->dbp_active = B_TRUE;
                }

- In the dt_proc_bpmatch() equivalent, match exactly as we currently
do, by comparing the current PC with dbp->dbp_addr (but not executing
the breakpoint-replaced instruction).
- In dt_proc_control(), call the dt_proc_bpmatch() equivalent similar
to the following ("pbp" for "pid breakpoint", for want of a
better
name):

                case PS_STOP:
                        psp = &Pstatus(P)->pr_lwp;
                        if (psp->pr_why == PR_REQUESTED) {
                                if (dt_proc_pbpmatch(pbp_dtp, dpr) == -1) {
                                        dt_proc_waitrun(dpr);
                                        (void) pthread_mutex_unlock(
                                            &dpr->dpr_lock);
                                        continue;
                                }
                        }

(There''s still the question of what to do with dt_proc_bpdestroy(),
dt_proc_bpenable() and dt_proc_disable().)

I''ve tried the above, and I see the probes being created, but these
probes never fire.  What''s more, the probes in the D script
I''m
running are never created, although I believe that''s just because the
RD_DLACTIVITY probe is never firing, thus
dt_pid_create_probes_module() is never called.

Any ideas?  I''ve put a copy of dt_proc.[ch] and DTRACE_DEBUG output at
http://cr.opensolaris.org/~cmynhier/6593259/ if you want to look at
it.

Thanks,
Chad

On Sun, Jan 06, 2008 at 08:37:32PM -0500, Chad Mynhier
wrote:
> I''ve been looking at this bug.  What I think needs to be done is
the following:
[snip]

I think you''ve got most of that right. There are some issues such as
the fact
that the stop() action will result in the thread being stopped on the
subsequent instruction. But I think you''re mostly there.
> (There''s still the question of what to do with
dt_proc_bpdestroy(),
> dt_proc_bpenable() and dt_proc_disable().)
It should no longer be necessary to enable and disable breakpoints since
this was really only done to get out of the way of the disassembler -- this
will all be taken care of by the existing mechanism.
> I''ve tried the above, and I see the probes being created, but
these
> probes never fire.  What''s more, the probes in the D script
I''m
> running are never created, although I believe that''s just because
the
> RD_DLACTIVITY probe is never firing, thus
> dt_pid_create_probes_module() is never called.
Is the instruction getting modified?
> Any ideas?  I''ve put a copy of dt_proc.[ch] and DTRACE_DEBUG
output at
> http://cr.opensolaris.org/~cmynhier/6593259/ if you want to look at
> it.
I think you''re missing a call to dtrace_go() at the very least. You
could
probably do this at the end of dt_proc_attach().

Adam

-- 
Adam Leventhal, FishWorks                        http://blogs.sun.com/ahl

I forgot to mention that there''s a rather large caveat here. If a user
has the dtrace_user or dtrace_kernel privilege, but _not_ the dtrace_proc
privilege he won''t be able to use pid provider probes.

I think the two options are to either support both breakpoints in libdtrace
as a fallback position, to work on getting dtrace_proc in the list of default
(basic) privileges, or give up on the bug for now.

Adam

On Sun, Jan 06, 2008 at 08:37:32PM -0500, Chad Mynhier
wrote:
> I''ve been looking at this bug.  What I think needs to be done is
the following:
> 
> - In dt_proc_control(), create a separate dtrace handle with the
> destructive option set.
> - In dt_proc_bp_create() (or the equivalent-but-renamed function), do
> something similar to the following instead of creating a breakpoint
> (where dtp is this new handle):
> 
>                 sprintf(dprog, "pid%d::-:%x {stop();}",
(int)dpr->dpr_pid,
>                     dbp->dbp_addr);
> 
>                 if ((pgp = dtrace_program_strcompile(dtp, dprog,
>                     DTRACE_PROBESPEC_NAME, DTRACE_C_NOLIBS, 0, NULL))
>                     == NULL) {
>                         dt_dprintf("pid %d: failed to compile pid
breakpoint "
>                             "at 0x%x\n", (int)dpr->dpr_pid,
dbp->dbp_addr);
>                 } else if (dtrace_program_exec(dtp, pgp, NULL)
>                     == -1) {
>                         dt_dprintf("pid %d: failed to create pid
breakpoint "
>                             "at 0x%x\n", (int)dpr->dpr_pid,
dbp->dbp_addr);
>                 } else {
>                         dbp->dbp_active = B_TRUE;
>                 }
> 
> - In the dt_proc_bpmatch() equivalent, match exactly as we currently
> do, by comparing the current PC with dbp->dbp_addr (but not executing
> the breakpoint-replaced instruction).
> - In dt_proc_control(), call the dt_proc_bpmatch() equivalent similar
> to the following ("pbp" for "pid breakpoint", for want
of a better
> name):
> 
>                 case PS_STOP:
>                         psp = &Pstatus(P)->pr_lwp;
>                         if (psp->pr_why == PR_REQUESTED) {
>                                 if (dt_proc_pbpmatch(pbp_dtp, dpr) == -1) {
>                                         dt_proc_waitrun(dpr);
>                                         (void) pthread_mutex_unlock(
>                                             &dpr->dpr_lock);
>                                         continue;
>                                 }
>                         }
> 
> (There''s still the question of what to do with
dt_proc_bpdestroy(),
> dt_proc_bpenable() and dt_proc_disable().)
> 
> I''ve tried the above, and I see the probes being created, but
these
> probes never fire.  What''s more, the probes in the D script
I''m
> running are never created, although I believe that''s just because
the
> RD_DLACTIVITY probe is never firing, thus
> dt_pid_create_probes_module() is never called.
> 
> Any ideas?  I''ve put a copy of dt_proc.[ch] and DTRACE_DEBUG
output at
> http://cr.opensolaris.org/~cmynhier/6593259/ if you want to look at
> it.
> 
> Thanks,
> Chad
> _______________________________________________
> dtrace-discuss mailing list
> dtrace-discuss at opensolaris.org
-- 
Adam Leventhal, FishWorks                        http://blogs.sun.com/ahl

On Jan 9, 2008 1:26 AM, Adam Leventhal <ahl at eng.sun.com>
wrote:
> I forgot to mention that there''s a rather large caveat here. If a
user
> has the dtrace_user or dtrace_kernel privilege, but _not_ the dtrace_proc
> privilege he won''t be able to use pid provider probes.
>
> I think the two options are to either support both breakpoints in libdtrace
> as a fallback position, to work on getting dtrace_proc in the list of
default
> (basic) privileges, or give up on the bug for now.
Yeah, that''s fairly major.  I''d prefer to fix this bug and get
the
appropriate privileges into the set of basic privileges.

Thanks,
Chad

On Wed 09 Jan 2008 at 09:19PM, Chad Mynhier wrote:
> On Jan 9, 2008 1:26 AM, Adam Leventhal <ahl at eng.sun.com> wrote:
> > I forgot to mention that there''s a rather large caveat here.
If a user
> > has the dtrace_user or dtrace_kernel privilege, but _not_ the
dtrace_proc
> > privilege he won''t be able to use pid provider probes.
> >
> > I think the two options are to either support both breakpoints in
libdtrace
> > as a fallback position, to work on getting dtrace_proc in the list of
default
> > (basic) privileges, or give up on the bug for now.
> 
> Yeah, that''s fairly major.  I''d prefer to fix this bug
and get the
> appropriate privileges into the set of basic privileges.
I think this will have a potential knock-on effect for zones, since
dtrace_proc isn''t in the set a zone (even a zone root user) has by
default.  We could change that, although the received wisdom up until
now has been that zones should not be granted dtrace_proc or dtrace_user
by default.

        -dp

-- 
Daniel Price - Solaris Kernel Engineering - dp at eng.sun.com - blogs.sun.com/dp

On Jan 9, 2008 10:27 PM, Dan Price <dp at eng.sun.com>
wrote:
>
> On Wed 09 Jan 2008 at 09:19PM, Chad Mynhier wrote:
> > On Jan 9, 2008 1:26 AM, Adam Leventhal <ahl at eng.sun.com>
wrote:
> > >
> > > I think the two options are to either support both breakpoints in
libdtrace
> > > as a fallback position, to work on getting dtrace_proc in the
list of default
> > > (basic) privileges, or give up on the bug for now.
> >
> > Yeah, that''s fairly major.  I''d prefer to fix this
bug and get the
> > appropriate privileges into the set of basic privileges.
>
> I think this will have a potential knock-on effect for zones, since
> dtrace_proc isn''t in the set a zone (even a zone root user) has by
> default.  We could change that, although the received wisdom up until
> now has been that zones should not be granted dtrace_proc or dtrace_user
> by default.
Hey, Dan,

Thanks for pointing this out.  I''m curious:  are these not part of the
default privileges for zones because there are known issues with
violating the fault boundary ceated by zones, or is it just that
nobody has had the time to fully look at the issue?   (I can see
2006/124 but not 2002/174 or 2002/188.  Do you know if the answer is
somewhere else I can see?)  I guess the real question is, would I be
tilting at windmills if I wanted to get this changed?

Chad

Sorry for the delay in the reply.

On Jan 7, 2008 1:27 PM, Adam Leventhal <ahl at eng.sun.com>
wrote:
> On Sun, Jan 06, 2008 at 08:37:32PM -0500, Chad Mynhier wrote:
> > I''ve been looking at this bug.  What I think needs to be done
is the following:
>
> [snip]
>
> > I''ve tried the above, and I see the probes being created, but
these
> > probes never fire.
>
> Is the instruction getting modified?
>
> I think you''re missing a call to dtrace_go() at the very least.
You could
> probably do this at the end of dt_proc_attach().
I verified that the instruction was getting modifed, and adding the
call to dtrace_go() made things work.

And this solution passes the test suite, so it looks good.

Chad