[DTrace-devel] [PATCH 09/15] provider: Implement a rawtp provider

[Nick Alcock]

On 23 Feb 2023, Kris Van Hees outgrape:

> On Thu, Feb 23, 2023 at 01:01:38PM +0000, Nick Alcock wrote:
>> On 23 Feb 2023, Kris Van Hees via DTrace-devel outgrape:
>> > +/*
>> > + * The PROBE_LIST file lists all tracepoints in a <group>:<name> format.
>> > + * We need to ignore these groups:
>> > + *   - GROUP_FMT (created by DTrace processes)
>> > + *   - kprobes and uprobes
>> > + *   - syscalls (handled by a different provider)
>> > + *   - pid and usdt probes (ditto)
>> > + */
>> 
>> What if there are other providers of tracepoints on the system? We
>> should really ignore all uprobes created by anyone that isn't us, but
>> just as we named ours differently, so too can they... I don't see an
>> obvious way around this, wich presuambly means you could set up a pair
>> of warring tracers each madly tracing the other one :)
>
> The challenge is that there is no way that I have found to determine the type
> of events listed in available_events.  So, we don't actually know whether
> something is an actual tracepoint aside from trying to filter out anything
> that we know is definitely not a tracepoint.
>
> Obviously, when we try to use something as a raw tracepoint it will simply
> fail if it isn't an actual tracepoint.  But that is about it.
>
> In terms of other probes... if a provider can attach to a probe that some
> other tracer created... oh well, enjoy!  No harm done.

Yeah that's what I suspected. It might edven be useful! :)

>> > +			if (sscanf(buf, GROUP_SFMT, &dummy, &str) == 2) {
>> 
>> Not entirely sure that the use of #defines for format strings here (and
>> in dt_prov_sdt.c) is buying us anything other than potential future
>> bugs: it's bad enough with printf but used with scanf it's just
>> terrifying. I'm shuddering inwardly at the idea of separating format
>> strings and format arguments like this. Maybe the whole sscanf should be
>> bundled up in a shared function in the future instead...
>
> You introduced that with the usdt work, so I will leave it to you to come
> up with something better in the future :) :) :)

Oh, ok! Only now does it occur to me that it's a bad idea... I'll think
about it.

>> > +		rtp_fd = dt_bpf_raw_tracepoint_open(prp->desc->prb, bpf_fd);
>> > +		close(bpf_fd);
>> > +		if (rtp_fd == -1)
>> > +			continue;
>> > +		close(rtp_fd);
>> 
>> ... I suppose we could at least check for EPERM? It's a shame there's no
>> immediate way to get a notification that this specifically got a
>> verifier failure, let alone what the failure was.
>
> I guess, but really, what is the point?  Unfortunately, the BPF syscall is
> really bad at being descriptive in its errno use.  You can get EPERM for many
> things, and in the end, there are 2 instructions: an indirect load from the
> BPF context, and an exit.  Any failure is sufficient to know that we cannot
> access the value.
>
> In the end, I really do not care what the errno is (and the kernel code could
> change and mess it up).  Failure is failure.

Yeah. We can't do anything different in different cases really.

>> > +		break;
>> > +	}
>> > +
>> > +	if (argc == 0)
>> > +		goto done;
>> 
>> ... so if we can't find any number of valid args, we assume it zero and
>> silently keep going? We should probably also check for success in the
>> the zero-args case, and if that still fails, raise an actual error more
>> loudly, since something else is clearly wrong.
>
> Err....  how can one distinguish between not finding any number of arguments
> and zero arguments.  No arguments == zero arguments == not finding any number
> of arguments.  You cannot test zero args because there is nothing to access,
> right?

You can test zero arguments by simply not doing the load at all and
making sure the simple BPF_RETURN() case works. If something is really
wrong and that fails, that's worth noting... you'd think nothing else in
dtrace would work if something that simple failed, but this is the first
and so far only place we're using raw tracepoints: if they fail and
nothing else does, right now we get no warning of that at all.

>> > +	argv = dt_zalloc(dtp, argc * sizeof(dt_argdesc_t));
>> > +
>> > +	for (i = 0; i < argc; i++) {
>> > +		argv[i].mapping = i;
>> > +		argv[i].native = "uint64_t";
>> > +		argv[i].xlate = NULL;
>> > +	}
>> > +
>> > +done:
>> > +        *argcp = argc;
>> > +        *argvp = argv;
>> > +
>> > +        return 0;
>> > +}
>> 
>> (There really should be at least some error case here.)
>
> What do you mean with 'error case'?  Either we can access arguments or we
> cannot.  What error case are you thinking of?

We get an error even when we try to not access anything, just a
BPF_RETURN(), likely because raw tracepoints are somehow busted :/

Not a *serious* problem but trivial to fix, I suspect.

-- 
NULL && (void)