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 <e...@regit.org>
> > Acked-by: Alexei Starovoitov <a...@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 <e...@regit.org>
Blog: https://home.regit.org/
