[DTrace-devel] [PATCH] bpf, perf: helper functions should return -errno on failure
Eugene Loh
eugene.loh at oracle.com
Thu Nov 30 23:09:10 UTC 2023
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.
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.
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.
> 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
More information about the DTrace-devel
mailing list