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

[Kris Van Hees]

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

> 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