From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Leblond Subject: Re: [PATCH bpf-next v5 2/4] libbpf: add error reporting in XDP Date: Fri, 19 Jan 2018 00:35:37 +0100 Message-ID: <1516318537.24936.7.camel@regit.org> References: <1515023944.7473.7.camel@regit.org> <20180104082149.13758-1-eric@regit.org> <20180104082149.13758-2-eric@regit.org> <10a2c572-9f2e-8cad-675d-b960c215c557@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Alexei Starovoitov , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Thomas Graf To: Daniel Borkmann , Toshiaki Makita , Philippe Ombredanne Return-path: In-Reply-To: <10a2c572-9f2e-8cad-675d-b960c215c557@iogearbox.net> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.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 > > Acked-by: Alexei Starovoitov > > --- > > 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 > > #include "bpf.h" > > #include "libbpf.h" > > +#include "nlattr.h" > > #include > > #include > > #include > > @@ -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 Blog: https://home.regit.org/