[DTrace-devel] [PATCH 1/2] Clean up error reporting in the BPF progran load process

[Kris Van Hees]

On Mon, Sep 12, 2022 at 03:28:32PM -0400, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

Thanks.

> Is the "correct" behavior documented anywhere?  If not, how are we to judge
> who is misusing what?

Documented?  Not really.  But the code gives a pretty good clue abuot what is
expected.  The dtrace_go() function expects a -1 return value upon failure and
the caller reports the error based on the error that has been set using
dt_set_error() or related functions.  That is how the interaction between
libdtrace and the command line utility works.

> Does any test show any problem?

No, but issues in code (e.g. the pid provider creating a DTrace probe for a
function that you cannot put a uprobe on) have revealed that there was an issue
here.

> It appears that when attach() encounters an error, it sometimes returns:
> --  -ENOENT, or
> --  -1, or
> --  dt_set_errno(dtp, errno)
> To be more cohesive, should it (well, its multiple implementations) just
> return -1?  Its behavior might as well be coherent.

Yes, that is something we should clean up a bit more.  Seems like a good
topic for a future patch.

> I know this is outside the scope of this patch, but dt_tp_attach() has:
>         if (tpp->event_id == -1)
>                 return 0;
> I'm rusty on this stuff, but why is event_id==-1 okay?

I cannot remember the exact details but if event_id is -1 then there is no
perf event created for this probe which means there is nothing to attach the
program to.

> And...
> 
> On 9/12/22 13:04, Kris Van Hees via DTrace-devel wrote:
> > The return value for the attach() provider hook was being passed to
> > the dtrace_go() through dt_bpf_load_progs(), confusing the dtrace
> > command line utility which was expecting 0 or -1 (with the proper
> > error code being set).
> > 
> > It turns out return values were misused in a few other places.  The
> > code will now explicitly check for -1 as failure indication (and return
> > -1 explicitly where needed) for dt_aggregate_go(), dt_bpf_gmap_create(),
> > dt_bpf_load_prog(), and dt_bpf_load_progs().
> 
> This implies that when dt_bpf_load_progs() checks dt_bpf_load_prog() for
> errors, it should check ==-1 rather than <0.
> 
> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > @@ -1059,15 +1059,13 @@ dt_bpf_load_progs(dtrace_hdl_t *dtp, uint_t cflags)
> >   		fd = dt_bpf_load_prog(dtp, prp, dp, cflags);
> >   		if (fd < 0)
> > -			return fd;
> > +			return -1;
> 
> Here!  Shouldn't we check fd==-1 rather than fd<0?  This presumably does not
> matter, but it seems to be what the commit message is advertising.

Fixed - thanks.

> >   		dt_difo_free(dtp, dp);
> > -		if (!prp->prov->impl->attach)
> > -			return -1;
> > -		rc = prp->prov->impl->attach(dtp, prp, fd);
> > -		if (rc < 0)
> > -			return rc;
> > +		if (!prp->prov->impl->attach ||
> > +		    prp->prov->impl->attach(dtp, prp, fd) < 0)
> > +			return dt_set_errno(dtp, EDT_ENABLING_ERR);
> >   	}
> >   	return 0;
> > diff --git a/libdtrace/dt_work.c b/libdtrace/dt_work.c
> > @@ -69,17 +68,13 @@ dtrace_go(dtrace_hdl_t *dtp, uint_t cflags)
> >                   setrlimit(RLIMIT_MEMLOCK, &rl);
> >           }
> > -	/*
> > -	 * Create the global BPF maps.  This is done only once regardless of
> > -	 * how many programs there are.
> > -	 */
> > -	err = dt_bpf_gmap_create(dtp);
> > -	if (err)
> > -		return err;
> > +	/* Create the global BPF maps. */
> > +	if (dt_bpf_gmap_create(dtp) == -1)
> > +		return -1;
> > -	err = dt_bpf_load_progs(dtp, cflags);
> > -	if (err)
> > -		return err;
> > +	/* Load the BPF programs.  */
> 
> Usually (like a few lines earlier), only one space before "*/".
> 
> > +	if (dt_bpf_load_progs(dtp, cflags) == -1)
> > +		return -1;
> >   	/*
> >   	 * Set up the event polling file descriptor.
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel