[DTrace-devel] [PATCH] Improve error checking and reporting for BPF map creation

[Eugene Loh]

On 8/9/22 14:02, Eugene Loh 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.

And of course:

*)  This patch should go onto the branch ahead of the err.resize2 patch.

*)  If you do decide to add ks/vs/sz info, that must be reflected in the 
err.resize2.r file as well.

> 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;