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

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

2018-01-18 Thread Eric Leblond

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(, 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,
> > +  , 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 *), 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/

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

2018-01-06 Thread 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 
> 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(, 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,
> +, sizeof(one)) < 0) {

... so we turn it on by chance here.

> + fprintf(stderr, "Netlink error reporting not supported\n");
> + }
> +
>   if (bind(sock, (struct sockaddr *), 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

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

2018-01-04 Thread Eric Leblond

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;
 
memset(, 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,
+  , sizeof(one)) < 0) {
+   fprintf(stderr, "Netlink error reporting not supported\n");
+   }
+
if (bind(sock, (struct sockaddr *), 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;
+   nla_dump_errormsg(nh);
goto cleanup;
case NLMSG_DONE:
break;
diff --git a/tools/lib/bpf/nlattr.c b/tools/lib/bpf/nlattr.c
new file mode 100644
index ..4719434278b2
--- /dev/null
+++ b/tools/lib/bpf/nlattr.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: LGPL-2.1
+
+/*
+ * NETLINK  Netlink attributes
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation version 2.1
+ * of the License.
+ *
+ * Copyright (c) 2003-2013 Thomas Graf 
+ */
+
+#include 
+#include "nlattr.h"
+#include 
+#include 
+#include 
+
+static uint16_t nla_attr_minlen[NLA_TYPE_MAX+1] = {
+   [NLA_U8]= sizeof(uint8_t),
+   [NLA_U16]   = sizeof(uint16_t),
+   [NLA_U32]   = sizeof(uint32_t),
+   [NLA_U64]   = sizeof(uint64_t),
+   [NLA_STRING]= 1,
+   [NLA_FLAG]  = 0,
+};
+
+static int nla_len(const struct nlattr *nla)
+{
+   return nla->nla_len - NLA_HDRLEN;
+}
+
+static struct nlattr *nla_next(const struct nlattr *nla, int *remaining)
+{
+   int totlen = NLA_ALIGN(nla->nla_len);
+
+   *remaining -= totlen;
+   return (struct nlattr *) ((char *) nla + totlen);
+}
+
+static int nla_ok(const struct nlattr *nla, int remaining)
+{
+   return remaining >= sizeof(*nla) &&
+  nla->nla_len >= sizeof(*nla) &&
+  nla->nla_len <= remaining;
+}
+
+static void *nla_data(const struct nlattr *nla)
+{
+   return (char *) nla + NLA_HDRLEN;
+}
+
+static int nla_type(const struct nlattr *nla)
+{
+   return nla->nla_type & NLA_TYPE_MASK;
+}
+
+static int validate_nla(struct nlattr *nla, int maxtype,
+   struct nla_policy *policy)
+{
+   struct nla_policy *pt;
+   unsigned int minlen = 0;
+   int type = nla_type(nla);
+
+   if (type < 0 || type > maxtype)
+   return 0;
+
+   pt = [type];
+
+   if (pt->type > NLA_TYPE_MAX)
+   return 0;
+
+   if (pt->minlen)
+   minlen = pt->minlen;
+   else if (pt->type != NLA_UNSPEC)
+   minlen = nla_attr_minlen[pt->type];
+
+   if (nla_len(nla) < minlen)
+   return -1;
+
+   if (pt->maxlen && nla_len(nla) > pt->maxlen)
+   return -1;
+
+

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information