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

[Daniel Borkmann]

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@xxxxxxxxx>
> Acked-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> ---
>  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.

> +		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?

> +			nla_dump_errormsg(nh);
>  			goto cleanup;
>  		case NLMSG_DONE:
>  			break;
Thanks,
Daniel