[DTrace-devel] [PATCH 02/10] dtprobed: add the DOF stash

[Nick Alcock]

On 29 Aug 2023, Kris Van Hees stated:

> As an overall comment, there are quite a few whitespace issues in this patch
> where tabs should be used and where extra spaces are present, messing up the
> alignment of statements, etc.  Plwase make sure to correct whitespace issues
> throughout patches before submitting them for review.

... I thought I did, but looking down the quoting has made some more
more instances obvious. Fixing as we go...

> More comments below...
>
> On Wed, Aug 02, 2023 at 02:26:52PM +0100, Nick Alcock via DTrace-devel wrote:
>> The DOF stash is a runtime directory (/run/dtrace/stash by default) in which
>> the dtprobed daemon keeps track of incoming DOF.  There's a comment
>> describing it at the top of dtprobed/dof_stash.c, but in brief we have two
>> directories under /run/dtrace/stash, dof/, which contains raw DOF named by
>> mapping, each entry hardlinked to a corresponding entry in
>> pid/$pid/$mapping/raw, which tracks the DOF-containing mappings on a
>> PID-by-PID basis.  We also keep track of parsed DOF in the per-PID
>
> This is a bit confusing to read.  How about first saying that there are two
> directories under /run/dtrace/stash: dof/ and pid/.  And then explain what
> each contains.
>
>> directory, and everything necessary to regenerate the parsed DOF should the
>> definition of struct dof_parsed change in future.
>
> You are storing the parsed data in binary format.  Wouldn't it make sense to
> use textual format?  While that is a bit less efficient, it adds great benefit
> for debugging purposes and it also makes it a lot less likely that you need to

I must admit I never thought of adding an extra serialization layer and
an extra parser when the whole point of this is to keep all the parsing
neatly locked away in a seccomped jail which is invoked as little as can
get away with. I thought everything was in danger of being overdesigned
*already*. Also, textual parsers in C are a classic cause of security
holes, doubly so when running as root: we'd surely need to seccomp-jail
that too (to guard against bugs in the parser, rather than to guard
against hostile input as with our current jail), and you might note that
jailed stuff even has trouble mallocing a lot, and textual parsing
usually means malloc everywhere.

But I love textual formats and I wish I couldn't immediately think of
multiple ways in which textualizing the DOF and reparsing it wouldn't be
so dreadful :/

Handling struct changes is not troublesome -- having to reparse on
restart is annoying, but a textual format would have the same problem
unless we never ever changed it (a dangerous assumption).

However, this is all moot given your next pint:

> handle struct changes.  Alternatively, you could just leave the parsing to
> DTrace itself and only store the raw DOF along with the helper info, right?

Hmm. As long as DTrace jails the parser too (which is fine, it can just
use the exact same code as the daemon does now), that dooes seem like a
*much* better approach. I don't think the daemon needs the result of
parsing at all any more: we literally just suck it in and stash it, so
DTrace could just as easily do it all itself. I'm kicking myself for not
noticing that its need for the parsed output had fallen to zero. (The
stash still needs maintenance, but only of the raw stuff and it should
be a bit simpler.)

Not a small change though. We can converge on that in the next release,
perhaps? I doubt it would take less than a week to do even if I had
nothing else on my plate.

> So, what is the benefit of parsing the data and storing it in some alternate
> binary format (since DOF is a binary format also)?  And if parsing makes sense,
> why not store it as textual data?

Mostly that I didn't notice that the jailed parser child could move into
dtrace itself now.

-- 
NULL && (void)