[DTrace-devel] [PATCH v3 06/21] dtprobed: add the DOF stash

[Nick Alcock]

On 14 Feb 2024, Kris Van Hees outgrape:

> On Mon, Feb 12, 2024 at 10:21:35AM -0500, Kris Van Hees via DTrace-devel wrote:
>> > +/*
>> > + * Free all the accumulated parsed DOF in the passed list.
>> > + */
>> > +void
>> > +dof_stash_flush(dt_list_t *accum)
>
> That is a bit of an unusual function name for a function that frees a list.
> When I see *flush I usually think of forcinfg things to be written out to
> stable storage or something like that.  Maybe dof_stash_free() would be
> better?

Yeah, that's a terrible name! Even the *comment* says what it should be
called.

Process-wide things like this are usually named _init and _fini in
DTrace, but in this case _init actually doesn't have an inverse (there
is no _fini), and this function actually undoes a bunch of
dof_stash_push_parsed(). This is what I thought _flush would be a good
inverse of. Since that's actually a dreadful name I changed this to
dof_stash_free().

(This causes minor modifications to the calls in a later commit, too.)

>> > +/*
>> > + * Write out a piece of raw DOF.  Returns the length of the file written,
>> > + * or 0 if none was needed (or -1 on error).
>> > + */
>> > +static int
>> > +dof_stash_write(int dirfd, const char *name, const void *buf, size_t size)
>
> For consistency this perhaps would be better named dof_stash_write_raw() ?

Ack, changed. (Originally it wrote out both...)

> Is there a point to passing in dirfd given that the only caller simply passes
> in the dof_dir global variable?

Only that it's passing in a filename too, and I do think it's neater to
pass in a path and name in that case (even if the path is a dirfd, much
like the *at() functions), rather than passing in a name and picking up
the path from somewhere else.

This is marginal though :)

>> > +{
>> > +	struct stat s;
>> > +	int fd;
>> > +
>> > +        /*
>> > +	 * Sanity check: if the DOF already exists but is not the same size
>> > +	 * as the DOF we already have, complain, and replace it and its parsed
>> > +	 * equivalent.  If it does exist, there's no need to write it out.
>
> This comment seems to imply that this function replaces the raw DOF and its
> parsed equivalent.  But the code in this function does not actually do that.
> So, I assume the comment needs to be updated to reflect reality?

Ugh, the comment was copied from its caller back in the day when the
caller also wrote the parsed DOF out (when I thought parsed DOF was
per-mapping, not per-mapping/per-PID) and not adequately adjusted.
Fixed.

>> > +	 *
>> > +	 * If we can't even unlink it or write it out, we give up -- the stash
>> > +	 * has failed and we won't be able to do anything it implies.
>> > +	 *
>> > +	 * (This is only a crude check -- obviously distinct raw DOF could be
>> > +	 * the same size by pure chance.)
>
> I fear that the check is so crude that it may not catch that many cases.  E.g.
> when I recompile an executable or library but do not change the probes, it is
> very likely that the offset location of the probes will change, but the DOF
> size will not because it is based on statically sized data items.  And so there
> is a higher chance that two distinct DOF versions will have the same size.
>
> Something like a checksum may be needed here.

As the caller notes, this is actually very hard to hit, hard enough that
I have no idea how to test it without a custom pathological filesystem:
you need to kill -9 a process with a mapping with DOF in it, unlink the
mapping's ELF file, then create a new one *with the same inode number*
for which the DOF actually has the same size.

Does that seem remotely probable? With inode numbers now chosen from a
2^32-wide space and rising to 2^64 on most filesystems (and on most new
ones, like btrfs and xfs, sparsely allocated within that range) this
feels like something it is just not worth worrying about, and certainly
not spending time in a single-threaded process doing checksums.

I suppose the one alternative is that you could *edit the ELF file* to
modify the DOF, which surely nothing should ever do, since it's a recipe
for crashes of shared libraries and -ETXTBSY of main programs? if
something does it -- say, an attacker -- the consequence of this is that
the new probably-malicious DOF might be ignored for a while.

I was more concerned that you'd tell me that this was pointless because
it's so hard to hit, but I thought I should do *something*, and this at
least is simple enough that I don't need to worry too much about it
being nearly impossible to test.

>> > +	return size + sizeof(uint64_t);
>> > +
>> > +  err:
>
> Goto labels are typically at the left marging in our code.

cc-mode strikes again!

... presumably this only applies to goto labels at the top level in a
function. Ones at a lower level (and dof_stash does have a few) would
look *really ugly* like that.

Hm, can I teach cc-mode how to do this...

... yep! for reference for the zero other of us who are Emacs users:

(defun c-lineup-top-level-labels-at-0 (_langelem)
  "Line up labels at the top level at column 0."
  (save-excursion
    (back-to-indentation)
    (if (< (current-column) c-basic-offset)
	'-
      0)))

(c-add-style "dtrace" '("linux" (c-offsets-alist . ((label . c-lineup-top-level-labels-at-0)))))