DTrace: Add jailname/jid builtins
ClosedPublic

Authored by domagoj.stolfa_gmail.com on Thu, Jan 11, 10:47 PM.

Details

Summary

This diff aims to add a jailname and jid builtin variables to support a stable API to get access to jail details. Currently, tracing jails is possible via use of another builtin variable, curthread, but this change makes it more reliable and easier to write scripts across FreeBSD versions.

zonename was also now added to FreeBSD and it simply falls through to jailname as a compatibility mechanism. The version for the variables is 1.13, as suggested by markj@.

Sponsored by: DARPA, AFRL

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dteske added a subscriber: dteske.EditedThu, Jan 11, 11:47 PM

Tracing on a per-jail basis using a predicate was already possible (example below):

someprobe /curthread->td_proc->p_ucred->cr_prison->pr_id == $1/
{
        /* some actions */
}

Tracing on a per-jail basis using a predicate was already possible (example below):

someprobe /curthread->td_proc->p_ucred->cr_prison->pr_id == $1/
{
        /* some actions */
}

That's true, however, this exposes a stable interface that people can use without following pointers in the D script and instead simply calling into a builtin variable. I'd imagine this is the same reason why you would want to have execname, errno, uid, gid and others, as they can also be accessible via other, more primitive builtins.

Tracing on a per-jail basis using a predicate was already possible (example below):

someprobe /curthread->td_proc->p_ucred->cr_prison->pr_id == $1/
{
        /* some actions */
}

That's true, however, this exposes a stable interface that people can use without following pointers in the D script and instead simply calling into a builtin variable. I'd imagine this is the same reason why you would want to have execname, errno, uid, gid and others, as they can also be accessible via other, more primitive builtins.

Don't get me wrong, I like the analog to Solaris's zonename and thus think jailname and jid are good additions.

I just wanted to point out that curthread already gives us access to far more details than the few given at the top-level.

Programs like "dwatch" (reviews.freebsd.org/D10006) make it possible to access not-only curthread details but the details of the parent, grandparent, and ancestor process(es), and thus the ability to trace a jail is neither novel nor unique.

If you're ever looking for inspiration on other top-level variables that might be nice-to-haves without having to crawl curthread->td_proc (as dwatch does), give dwatch a trial run.

dteske requested changes to this revision.Fri, Jan 12, 1:06 PM

I just noticed that the ifdef for solaris was removed, resulting in the declaration of zonename. Was this intentional, and if-so, why?

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
537–540 ↗(On Diff #37846)

I don't think we want to expose "zonename" on FreeBSD.

This revision now requires changes to proceed.Fri, Jan 12, 1:06 PM

Tracing on a per-jail basis using a predicate was already possible (example below):

someprobe /curthread->td_proc->p_ucred->cr_prison->pr_id == $1/
{
        /* some actions */
}

That's true, however, this exposes a stable interface that people can use without following pointers in the D script and instead simply calling into a builtin variable. I'd imagine this is the same reason why you would want to have execname, errno, uid, gid and others, as they can also be accessible via other, more primitive builtins.

Don't get me wrong, I like the analog to Solaris's zonename and thus think jailname and jid are good additions.

I just wanted to point out that curthread already gives us access to far more details than the few given at the top-level.

Programs like "dwatch" (reviews.freebsd.org/D10006) make it possible to access not-only curthread details but the details of the parent, grandparent, and ancestor process(es), and thus the ability to trace a jail is neither novel nor unique.

If you're ever looking for inspiration on other top-level variables that might be nice-to-haves without having to crawl curthread->td_proc (as dwatch does), give dwatch a trial run.

Oh no -- I understand what you meant :). Perhaps my wording in the initial patch was wrong for what I was actually trying to express. I'll fix it to reflect that this does not add the ability to trace jails, rather makes it simpler and more reliable across FreeBSD versions.

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
537–540 ↗(On Diff #37846)

It's a bit unhelpful that the diff doesn't show the detail of DIF_VAR_JAILNAME being right under DIF_VAR_ZONENAME and falls through to it. The goal is to introduce a compatibility with scripts written on Solaris/illumos and have them work if they use zonename. Presently, we just returned 0 in the kernel, and instead now we check if FreeBSD is defined and fall through to jailname.

Update the diff to add more context. No actual code was changed.

dteske accepted this revision.Fri, Jan 12, 2:43 PM

After you explained that zonename is for compatibility, making it easier to port scripts from Solaris, this seems like a good change. Thank you for you hard work. Cheers!

This revision is now accepted and ready to land.Fri, Jan 12, 2:43 PM
markj accepted this revision.Fri, Jan 12, 4:16 PM

Looks ok to me, thanks. Did you end up writing test cases for safety/ as we discussed on IRC?

cddl/contrib/opensolaris/lib/libdtrace/common/dt_open.c
322 ↗(On Diff #37866)

Jail IDs are represented internally by an int, so this ought to be an int as well. I realize that valid jail IDs are non-negative, but in general we should match the underlying type when possible.

sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
3692 ↗(On Diff #37866)

Consider adding a comment here indicating that we intend for the DIF_VAR_ZONENAME case to fall through here on FreeBSD.

Address the comments by markj@ and add tests in safety/ for jailname and jid.

This revision now requires review to proceed.Fri, Jan 12, 5:15 PM
markj accepted this revision.Fri, Jan 12, 5:37 PM

Thanks. I'll commit this later tonight.

This revision is now accepted and ready to land.Fri, Jan 12, 5:37 PM
This revision was automatically updated to reflect the committed changes.
Herald added 1 blocking reviewer(s): gnn. · View Herald TranscriptFri, Jan 12, 8:00 PM
Herald added a subscriber: imp. · View Herald Transcript