Re: [PATCH bpf-next v5 2/4] libbpf: add error reporting in XDP

[Eric Leblond <eric@regit.org>]

Hi,

Sorry for the delay, missed the mail.

On Sat, 2018-01-06 at 22:16 +0100, Daniel Borkmann wrote:
> On 01/04/2018 09:21 AM, Eric Leblond wrote:
> > Parse netlink ext attribute to get the error message returned by
> > the card. Code is partially take from libnl.
> > 
> > Signed-off-by: Eric Leblond <eric@regit.org>
> > Acked-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> >  samples/bpf/Makefile   |   2 +-
> >  tools/lib/bpf/Build    |   2 +-
> >  tools/lib/bpf/bpf.c    |  10 ++-
> >  tools/lib/bpf/nlattr.c | 187
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tools/lib/bpf/nlattr.h |  70 ++++++++++++++++++
> >  5 files changed, 268 insertions(+), 3 deletions(-)
> >  create mode 100644 tools/lib/bpf/nlattr.c
> >  create mode 100644 tools/lib/bpf/nlattr.h
> > 
> > diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> > index 4fb944a7ecf8..c889ebcba9b3 100644
> > --- a/samples/bpf/Makefile
> > +++ b/samples/bpf/Makefile
> > @@ -44,7 +44,7 @@ hostprogs-y += xdp_monitor
> >  hostprogs-y += syscall_tp
> >  
> >  # Libbpf dependencies
> > -LIBBPF := ../../tools/lib/bpf/bpf.o
> > +LIBBPF := ../../tools/lib/bpf/bpf.o ../../tools/lib/bpf/nlattr.o
> >  CGROUP_HELPERS :=
> > ../../tools/testing/selftests/bpf/cgroup_helpers.o
> >  
> >  test_lru_dist-objs := test_lru_dist.o $(LIBBPF)
> > diff --git a/tools/lib/bpf/Build b/tools/lib/bpf/Build
> > index d8749756352d..64c679d67109 100644
> > --- a/tools/lib/bpf/Build
> > +++ b/tools/lib/bpf/Build
> > @@ -1 +1 @@
> > -libbpf-y := libbpf.o bpf.o
> > +libbpf-y := libbpf.o bpf.o nlattr.o
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index e6c61035b64c..10d71b9fdbd0 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/bpf.h>
> >  #include "bpf.h"
> >  #include "libbpf.h"
> > +#include "nlattr.h"
> >  #include <linux/rtnetlink.h>
> >  #include <sys/socket.h>
> >  #include <errno.h>
> > @@ -440,6 +441,7 @@ int bpf_set_link_xdp_fd(int ifindex, int fd,
> > __u32 flags)
> >  	struct nlmsghdr *nh;
> >  	struct nlmsgerr *err;
> >  	socklen_t addrlen;
> > +	int one;
> 
> Hmm, it's not initialized here to 1 ...
> 
> >  	memset(&sa, 0, sizeof(sa));
> >  	sa.nl_family = AF_NETLINK;
> > @@ -449,6 +451,11 @@ int bpf_set_link_xdp_fd(int ifindex, int fd,
> > __u32 flags)
> >  		return -errno;
> >  	}
> >  
> > +	if (setsockopt(sock, SOL_NETLINK, NETLINK_EXT_ACK,
> > +		       &one, sizeof(one)) < 0) {
> 
> ... so we turn it on by chance here.

Indeed, fixing that.

> > +		fprintf(stderr, "Netlink error reporting not
> > supported\n");
> > +	}
> > +
> >  	if (bind(sock, (struct sockaddr *)&sa, sizeof(sa)) < 0) {
> >  		ret = -errno;
> >  		goto cleanup;
> > @@ -524,7 +531,8 @@ int bpf_set_link_xdp_fd(int ifindex, int fd,
> > __u32 flags)
> >  			err = (struct nlmsgerr *)NLMSG_DATA(nh);
> >  			if (!err->error)
> >  				continue;
> > -			ret = err->error;
> > +			ret = -err->error;
> 
> This one looks strange. Your prior patch added the 'ret = err->error'
> and this one negates it. Which variant is the correct version? From
> digging into the kernel code, my take is that 'ret = err->error' was
> the correct variant since it already holds the negative error code.
> Could you double check?

Yes all netlink_ack usage I have seen are using the negative value of
the error. Fixing that too.

BR,
-- 
Eric Leblond <eric@regit.org>
Blog: https://home.regit.org/