[DTrace-devel] [PATCH] Improve error checking and reporting for BPF map creation
Eugene Loh
eugene.loh at oracle.com
Tue Aug 9 21:15:54 UTC 2022
Not a big deal. Leaving the patch "as is" is fine by me, but...
On 8/9/22 15:46, Kris Van Hees wrote:
> On Tue, Aug 09, 2022 at 02:02:55PM -0700, Eugene Loh via DTrace-devel wrote:
>> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
>> I guess all I'd add is that if the map create fails, it might be nice to
>> report not just the map name but also ks, vs, and sz. That's less a fault of
>> this patch as a fault of the existing code, but this patch might be a
>> natural place to add such an enhancement.
> I would agree other than that DTRACE_DEBUG already enables output that will
> report the sizes, so it is easy enough to run dtrace with DTRACE_DEBUG=t
Well, DTRACE_DEBUG is arguably not documented. Also, it generates all
kinds of other output as well.
> and
> get that output if a BPF map creation fails. The specifics of those sizes
> are not really user-friendly because it relates to development specifics that
> the average user will not know (and probably care) about.
Agreed.
>
>> On 8/9/22 14:43, Kris Van Hees via DTrace-devel wrote:
>>> Eugene Loh found that when BPF creation fails due to too big a map,
>>> E2BIG is returned. This got reported as "Argument list too long"
>>> (using strerror()). That is not very helpful. It will now report
>>> "Too big".
>>>
>>> In addition, the key and value sizes are size_t entities in the main
>>> caller (dt_bpf_gmap_create()), while create_map() accepts ints for
>>> these values. And the maximum number of map entries is an int in
>>> dt_bpf_gmap_create() but is at times calculated based on size_t values.
>>> And the bpf(BPF_MAP_CREATE) syscall expects uint32_t values for all
>>> three. That is now cleaned up.
>>>
>>> The debug output in create_gmap() also got a small modification to no
>>> longer report sizes in the success report since it is already reported
>>> at the beginning of the function.
>>>
>>> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
>>> ---
>>> libdtrace/dt_bpf.c | 31 ++++++++++++++++++++-----------
>>> 1 file changed, 20 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
>>> index 5ff348ca..0c97ca7b 100644
>>> --- a/libdtrace/dt_bpf.c
>>> +++ b/libdtrace/dt_bpf.c
>>> @@ -100,8 +100,8 @@ int dt_bpf_prog_load(enum bpf_prog_type prog_type, const dtrace_difo_t *dp,
>>> * Create a named BPF map.
>>> */
>>> int dt_bpf_map_create(enum bpf_map_type map_type, const char *name,
>>> - int key_size, int value_size, int max_entries,
>>> - uint32_t map_flags)
>>> + uint32_t key_size, uint32_t value_size,
>>> + uint32_t max_entries, uint32_t map_flags)
>>> {
>>> union bpf_attr attr;
>>> @@ -242,26 +242,35 @@ dt_bpf_init_helpers(dtrace_hdl_t *dtp)
>>> static int
>>> create_gmap(dtrace_hdl_t *dtp, const char *name, enum bpf_map_type type,
>>> - int ksz, int vsz, int size)
>>> + size_t ksz, size_t vsz, size_t size)
>>> {
>>> - int fd;
>>> + int fd, err;
>>> dt_ident_t *idp;
>>> - dt_dprintf("Creating BPF map '%s' (ksz %u, vsz %u, sz %d)\n",
>>> + dt_dprintf("Creating BPF map '%s' (ksz %lu, vsz %lu, sz %lu)\n",
>>> name, ksz, vsz, size);
>>> - fd = dt_bpf_map_create(type, name, ksz, vsz, size, 0);
>>> +
>>> + if (ksz > UINT_MAX || vsz > UINT_MAX || size > UINT_MAX) {
>>> + fd = -1;
>>> + err = E2BIG;
>>> + } else {
>>> + fd = dt_bpf_map_create(type, name, ksz, vsz, size, 0);
>>> + err = errno;
>>> + }
>>> +
>>> if (fd < 0) {
>>> char msg[64];
>>> snprintf(msg, sizeof(msg),
>>> "failed to create BPF map '%s'", name);
>>> - if (errno == EPERM)
>>> + if (err == E2BIG)
>>> + return dt_bpf_error(dtp, "%s: Too big\n", msg);
>>> + if (err == EPERM)
>>> return dt_bpf_lockmem_error(dtp, msg);
>>> - return dt_bpf_error(dtp, "%s: %s\n", msg, strerror(errno));
>>> + return dt_bpf_error(dtp, "%s: %s\n", msg, strerror(err));
>>> }
>>> - dt_dprintf("BPF map '%s' is FD %d (ksz %u, vsz %u, sz %d)\n",
>>> - name, fd, ksz, vsz, size);
>>> + dt_dprintf("BPF map '%s' is FD %d\n", name, fd);
>>> /*
>>> * Assign the fd as id for the BPF map identifier.
>>> @@ -354,7 +363,7 @@ populate_probes_map(dtrace_hdl_t *dtp, int fd)
>>> int
>>> dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>>> {
>>> - int sz;
>>> + size_t sz;
>>> int dvarc = 0;
>>> int ci_mapfd, st_mapfd, pr_mapfd;
>>> uint64_t key = 0;
>> _______________________________________________
>> 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