[DTrace-devel] [PATCH] bpf, perf: helper functions should return -errno on failure
Kris Van Hees
kris.van.hees at oracle.com
Fri Dec 1 03:06:54 UTC 2023
On Thu, Nov 30, 2023 at 06:09:10PM -0500, Eugene Loh via DTrace-devel wrote:
> On 11/30/23 16:52, Kris Van Hees wrote:
>
> > On Thu, Nov 30, 2023 at 04:21:58PM -0500, Eugene Loh via DTrace-devel wrote:
> > > One can argue about whether these return values "make sense" or "are more
> > > convenient," but this is a big departure from "similar" functions, including
> > > what people are used to thinking of as bpf() and perf_event_open(). That
> > > is, these functions normally return -1 on error, with errno indicating the
> > > error.
> > One could argue that, but depending on errno is dangerous unless you do it
> > in the immediate caller, because anything further along the call chain might
> > affect the errno value. Since we are returning a value anyway, and we know
> > that the valid value of these functions can only be a non-negative value, it
> > is reasonable to make use of the return value to pass the actual error code
> > rather than relying on a global variable.
>
> Sure, errno is vulnerable and we don't want to be relying on it too far away
> from the call site. So passing -errno up through layers that are
> exclusively ours -- functions like dt_bpf_prog_load() -- is fine by me. I'm
> just saying that there are many more people who are used to bpf() and
> perf_event_open() as having certain return values; so let's leave those two
> well known functions (well, the APIs) alone.
>
> For starters, how about leaving perf_event_open() out of this patch?
> Changing the behavior of perf_event_open() simply results in having to make
> a companion change in dt_tp_attach(). If one pulled perf_event_open() out
> of this patch, the patch would be that much simpler.
I prefer the consistency of both system call wrappers to behave the same.
Besides, the changes to perf_event_open() and dt_tp_attach() are so minimal
that the claim that "the patch would be that much simpler" is a bit of an
over-statement I think.
> As for bpf(), how about just renaming it to be dt_bpf(). That indicates
> that it's something new. I would think that would handle both what you're
> trying to do with the patch as well as my concerns about changing something
> that is familiar to very many people.
I don't really see the concern given that this is a change that is entirely
isolated to DTrace itself, in a function that is not exported outside of
libdtrace. So there shouldn't be users that are caught unaware.
But I can rename them to be dt_bpf() and dt_perf_event_open() - makes no real
difference to me.
> BTW, if you're trying to get rid of callers looking at errno, there seem to
> be other places where this change still needs to be made. I'm not sure,
> though, since I don't at what point this patch will be applied.
I am not trying to change all uses of errno - I am changing the way those two
functions behave because they are functions we implement - they are not C lib
system call functions. So there is no need to make them behave as such.
Are we ok now for Reviewed-by?
>
> > Since the only safe way to use errno would be to have the actual caller of
> > bpf() and perf_event_open() immediately check errno if those function return
> > 01, and then would need to pass that error code to their callers as a return
> > value or using some other non-errno mechanism, why not have those two
> > functions simply do that for us? An earlier version of this patch actually
> > modified all the callers, and that seemed just plain silly because it was just
> > code duplication. The logical place to check for the -1 value and to then
> > look at errno is at the lowest level, and that is what thiss patch does.
> >
> > Can it get your reviewed-by, please?
> >
> > > On 11/29/23 16:11, Kris Van Hees via DTrace-devel wrote:
> > > > The bpf() and perf_event_open() helper functions were returning -1 on
> > > > failure and callers were supposed to check errno. Since the only valid
> > > > values are >= 0, they now return -errno on failure so callers can just
> > > > check the return value rather than errno.
> > > >
> > > > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > > > ---
> > > > libdtrace/dt_bpf.c | 29 ++++++++++++++++++-----------
> > > > libdtrace/dt_prov_rawtp.c | 4 ++--
> > > > libdtrace/dt_provider_tp.c | 2 +-
> > > > 3 files changed, 21 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > > > index 6c9758a1..8034af48 100644
> > > > --- a/libdtrace/dt_bpf.c
> > > > +++ b/libdtrace/dt_bpf.c
> > > > @@ -31,14 +31,20 @@ int
> > > > perf_event_open(struct perf_event_attr *attr, pid_t pid,
> > > > int cpu, int group_fd, unsigned long flags)
> > > > {
> > > > - return syscall(__NR_perf_event_open, attr, pid, cpu, group_fd,
> > > > - flags | PERF_FLAG_FD_CLOEXEC);
> > > > + int rc;
> > > > +
> > > > + rc = syscall(__NR_perf_event_open, attr, pid, cpu, group_fd,
> > > > + flags | PERF_FLAG_FD_CLOEXEC);
> > > > + return rc >= 0 ? rc : -errno;
> > > > }
> > > > int
> > > > bpf(enum bpf_cmd cmd, union bpf_attr *attr)
> > > > {
> > > > - return syscall(__NR_bpf, cmd, attr, sizeof(union bpf_attr));
> > > > + int rc;
> > > > +
> > > > + rc = syscall(__NR_bpf, cmd, attr, sizeof(union bpf_attr));
> > > > + return rc >= 0 ? rc : -errno;
> > > > }
> > > > static int
> > > > @@ -92,7 +98,7 @@ dt_bpf_prog_load(enum bpf_prog_type prog_type, const dtrace_difo_t *dp,
> > > > /* Syscall could return EAGAIN - try at most 5 times. */
> > > > do {
> > > > fd = bpf(BPF_PROG_LOAD, &attr);
> > > > - } while (fd < 0 && errno == EAGAIN && ++i < 5);
> > > > + } while (fd == -EAGAIN && ++i < 5);
> > > > return fd;
> > > > }
> > > > @@ -143,7 +149,7 @@ dt_bpf_map_create_meta(enum bpf_map_type otype, const char *name,
> > > > ifd = bpf(BPF_MAP_CREATE, &attr);
> > > > if (ifd < 0)
> > > > - return -1;
> > > > + return ifd;
> > > > /* Create the map-of-maps. */
> > > > memset(&attr, 0, sizeof(attr));
> > > > @@ -976,7 +982,7 @@ dt_bpf_load_prog(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > > > {
> > > > size_t logsz;
> > > > char *log;
> > > > - int rc, origerrno = 0;
> > > > + int rc, origerr = 0;
> > > > const dtrace_probedesc_t *pdp = prp->desc;
> > > > char *p, *q;
> > > > @@ -996,7 +1002,7 @@ dt_bpf_load_prog(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > > > if (rc >= 0)
> > > > return rc;
> > > > - origerrno = errno;
> > > > + origerr = -rc;
> > > > }
> > > > if (dtp->dt_options[DTRACEOPT_BPFLOGSIZE] != DTRACEOPT_UNSET)
> > > > @@ -1008,18 +1014,19 @@ dt_bpf_load_prog(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > > > rc = dt_bpf_prog_load(prp->prov->impl->prog_type, dp, 4 | 2 | 1,
> > > > log, logsz);
> > > > if (rc < 0) {
> > > > - char msg[64];
> > > > + char msg[64];
> > > > + int err = -rc;
> > > > snprintf(msg, sizeof(msg),
> > > > "BPF program load for '%s:%s:%s:%s' failed",
> > > > pdp->prv, pdp->mod, pdp->fun, pdp->prb);
> > > > - if (errno == EPERM)
> > > > + if (err == EPERM)
> > > > return dt_bpf_lockmem_error(dtp, msg);
> > > > dt_bpf_error(dtp, "%s: %s\n", msg,
> > > > - strerror(origerrno ? origerrno : errno));
> > > > + strerror(origerr ? origerr : err));
> > > > /* check whether we have an incomplete BPF log */
> > > > - if (errno == ENOSPC) {
> > > > + if (err == ENOSPC) {
> > > > fprintf(stderr,
> > > > "BPF verifier log is incomplete and is not reported.\n"
> > > > "Set DTrace option 'bpflogsize' to some greater size for more output.\n"
> > > > diff --git a/libdtrace/dt_prov_rawtp.c b/libdtrace/dt_prov_rawtp.c
> > > > index 1ff0a311..24104dd5 100644
> > > > --- a/libdtrace/dt_prov_rawtp.c
> > > > +++ b/libdtrace/dt_prov_rawtp.c
> > > > @@ -181,11 +181,11 @@ static int probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
> > > > dif.dtdo_len = 2;
> > > > bpf_fd = dt_bpf_prog_load(dt_rawtp.prog_type, &dif, 0, NULL, 0);
> > > > - if (bpf_fd == -1)
> > > > + if (bpf_fd < 0)
> > > > continue;
> > > > rtp_fd = dt_bpf_raw_tracepoint_open(prp->desc->prb, bpf_fd);
> > > > close(bpf_fd);
> > > > - if (rtp_fd == -1)
> > > > + if (rtp_fd < 0)
> > > > continue;
> > > > close(rtp_fd);
> > > > break;
> > > > diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
> > > > index f3e03323..23766b2a 100644
> > > > --- a/libdtrace/dt_provider_tp.c
> > > > +++ b/libdtrace/dt_provider_tp.c
> > > > @@ -75,7 +75,7 @@ dt_tp_attach(dtrace_hdl_t *dtp, tp_probe_t *tpp, int bpf_fd)
> > > > fd = perf_event_open(&attr, -1, 0, -1, 0);
> > > > if (fd < 0)
> > > > - return -errno;
> > > > + return fd;
> > > > tpp->event_fd = fd;
> > > > }
> > > _______________________________________________
> > > DTrace-devel mailing list
> > > DTrace-devel at oss.oracle.com
> > > https://oss.oracle.com/mailman/listinfo/dtrace-devel
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list