diff mbox series

[bpf-next,11/11] selftests/bpf: add checks on extack messages for eBPF hw offload tests

Message ID 20180116003027.9405-12-jakub.kicinski@netronome.com
State Changes Requested, archived
Delegated to: BPF Maintainers
Headers show
Series net: sched: add extack support for cls offload | expand

Commit Message

Jakub Kicinski Jan. 16, 2018, 12:30 a.m. UTC 
From: Quentin Monnet <quentin.monnet@netronome.com>

Add checks to test that netlink extack messages are correctly displayed
in some expected error cases for eBPF offload to netdevsim with TC and
XDP.

A new flag ("--skip-extack") is added to the Python script so as to
allow to skip these checks. This is because extack messages cannot be
displayed by tc and ip if tools from iproute2 package were compiled
without libmnl, but we do not want this to prevent users to run the
other checks.

Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/testing/selftests/bpf/test_offload.py | 97 +++++++++++++++++++++--------
 1 file changed, 71 insertions(+), 26 deletions(-)

Comments

David Ahern Jan. 16, 2018, 12:49 a.m. UTC | #1 
On 1/15/18 5:30 PM, Jakub Kicinski wrote:
> A new flag ("--skip-extack") is added to the Python script so as to
> allow to skip these checks. This is because extack messages cannot be
> displayed by tc and ip if tools from iproute2 package were compiled
> without libmnl, but we do not want this to prevent users to run the
> other checks.

That is unfortunate. Did you consider auto-detecting support? e.g., run
a command that is known to fail and return a message. For example,

$ ip ro add 1.1.1.1/32 dev eth0 onlink
Error: Invalid flags for nexthop - PERVASIVE and ONLINK can not be set.
Jakub Kicinski Jan. 16, 2018, 12:55 a.m. UTC | #2 
On Mon, 15 Jan 2018 17:49:07 -0700, David Ahern wrote:
> On 1/15/18 5:30 PM, Jakub Kicinski wrote:
> > A new flag ("--skip-extack") is added to the Python script so as to
> > allow to skip these checks. This is because extack messages cannot be
> > displayed by tc and ip if tools from iproute2 package were compiled
> > without libmnl, but we do not want this to prevent users to run the
> > other checks.  
> 
> That is unfortunate. Did you consider auto-detecting support? e.g., run
> a command that is known to fail and return a message. For example,
> 
> $ ip ro add 1.1.1.1/32 dev eth0 onlink
> Error: Invalid flags for nexthop - PERVASIVE and ONLINK can not be set.

That's a good idea.  And then if it fails would you suggest to skip the
test entirely or just skip extack checks?  With the --skip-extack flag
the hope was that people would be inclined to install libmnl to save
themselves the hassle of setting the flag each time.  If it's detected
automatically there is no hassle/nudge to install libmnl..
David Ahern Jan. 16, 2018, 1:02 a.m. UTC | #3 
On 1/15/18 5:55 PM, Jakub Kicinski wrote:
> On Mon, 15 Jan 2018 17:49:07 -0700, David Ahern wrote:
>> On 1/15/18 5:30 PM, Jakub Kicinski wrote:
>>> A new flag ("--skip-extack") is added to the Python script so as to
>>> allow to skip these checks. This is because extack messages cannot be
>>> displayed by tc and ip if tools from iproute2 package were compiled
>>> without libmnl, but we do not want this to prevent users to run the
>>> other checks.  
>>
>> That is unfortunate. Did you consider auto-detecting support? e.g., run
>> a command that is known to fail and return a message. For example,
>>
>> $ ip ro add 1.1.1.1/32 dev eth0 onlink
>> Error: Invalid flags for nexthop - PERVASIVE and ONLINK can not be set.
> 
> That's a good idea.  And then if it fails would you suggest to skip the
> test entirely or just skip extack checks?  With the --skip-extack flag
> the hope was that people would be inclined to install libmnl to save
> themselves the hassle of setting the flag each time.  If it's detected
> automatically there is no hassle/nudge to install libmnl..
> 

My inclination is to avoid unnecessary options and adapt to the
environment. ie., just skip the checks. You can still nudge users with a
warning that it appears ip or tc does not have extack support.
Jakub Kicinski Jan. 16, 2018, 1:04 a.m. UTC | #4 
On Mon, 15 Jan 2018 18:02:17 -0700, David Ahern wrote:
> On 1/15/18 5:55 PM, Jakub Kicinski wrote:
> > On Mon, 15 Jan 2018 17:49:07 -0700, David Ahern wrote:  
> >> On 1/15/18 5:30 PM, Jakub Kicinski wrote:  
> >>> A new flag ("--skip-extack") is added to the Python script so as to
> >>> allow to skip these checks. This is because extack messages cannot be
> >>> displayed by tc and ip if tools from iproute2 package were compiled
> >>> without libmnl, but we do not want this to prevent users to run the
> >>> other checks.    
> >>
> >> That is unfortunate. Did you consider auto-detecting support? e.g., run
> >> a command that is known to fail and return a message. For example,
> >>
> >> $ ip ro add 1.1.1.1/32 dev eth0 onlink
> >> Error: Invalid flags for nexthop - PERVASIVE and ONLINK can not be set.  
> > 
> > That's a good idea.  And then if it fails would you suggest to skip the
> > test entirely or just skip extack checks?  With the --skip-extack flag
> > the hope was that people would be inclined to install libmnl to save
> > themselves the hassle of setting the flag each time.  If it's detected
> > automatically there is no hassle/nudge to install libmnl..
> >   
> 
> My inclination is to avoid unnecessary options and adapt to the
> environment. ie., just skip the checks. You can still nudge users with a
> warning that it appears ip or tc does not have extack support.

OK, I'll respin shortly, thanks!
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/test_offload.py b/tools/testing/selftests/bpf/test_offload.py
index e3c750f17cb8..94ced38f650b 100755
--- a/tools/testing/selftests/bpf/test_offload.py
+++ b/tools/testing/selftests/bpf/test_offload.py
@@ -131,7 +131,7 @@  netns = [] # net namespaces to be removed
     if f in files:
         files.remove(f)
 
-def tool(name, args, flags, JSON=True, ns="", fail=True):
+def tool(name, args, flags, JSON=True, ns="", fail=True, include_stderr=False):
     params = ""
     if JSON:
         params += "%s " % (flags["json"])
@@ -139,9 +139,20 @@  netns = [] # net namespaces to be removed
     if ns != "":
         ns = "ip netns exec %s " % (ns)
 
-    ret, out = cmd(ns + name + " " + params + args, fail=fail)
-    if JSON and len(out.strip()) != 0:
-        return ret, json.loads(out)
+    if include_stderr:
+        ret, stdout, stderr = cmd(ns + name + " " + params + args,
+                                  fail=fail, include_stderr=True)
+    else:
+        ret, stdout = cmd(ns + name + " " + params + args,
+                          fail=fail, include_stderr=False)
+
+    if JSON and len(stdout.strip()) != 0:
+        out = json.loads(stdout)
+    else:
+        out = stdout
+
+    if include_stderr:
+        return ret, out, stderr
     else:
         return ret, out
 
@@ -164,13 +175,15 @@  netns = [] # net namespaces to be removed
         time.sleep(0.05)
     raise Exception("Time out waiting for program counts to stabilize want %d, have %d" % (expected, nprogs))
 
-def ip(args, force=False, JSON=True, ns="", fail=True):
+def ip(args, force=False, JSON=True, ns="", fail=True, include_stderr=False):
     if force:
         args = "-force " + args
-    return tool("ip", args, {"json":"-j"}, JSON=JSON, ns=ns, fail=fail)
+    return tool("ip", args, {"json":"-j"}, JSON=JSON, ns=ns, fail=fail,
+                include_stderr=include_stderr)
 
-def tc(args, JSON=True, ns="", fail=True):
-    return tool("tc", args, {"json":"-p"}, JSON=JSON, ns=ns, fail=fail)
+def tc(args, JSON=True, ns="", fail=True, include_stderr=False):
+    return tool("tc", args, {"json":"-p"}, JSON=JSON, ns=ns, fail=fail,
+                include_stderr=include_stderr)
 
 def ethtool(dev, opt, args, fail=True):
     return cmd("ethtool %s %s %s" % (opt, dev["ifname"], args), fail=fail)
@@ -311,13 +324,13 @@  netns = [] # net namespaces to be removed
         return ip("link set dev %s mtu %d" % (self.dev["ifname"], mtu),
                   fail=fail)
 
-    def set_xdp(self, bpf, mode, force=False, fail=True):
+    def set_xdp(self, bpf, mode, force=False, fail=True, include_stderr=False):
         return ip("link set dev %s xdp%s %s" % (self.dev["ifname"], mode, bpf),
-                  force=force, fail=fail)
+                  force=force, fail=fail, include_stderr=include_stderr)
 
-    def unset_xdp(self, mode, force=False, fail=True):
+    def unset_xdp(self, mode, force=False, fail=True, include_stderr=False):
         return ip("link set dev %s xdp%s off" % (self.dev["ifname"], mode),
-                  force=force, fail=fail)
+                  force=force, fail=fail, include_stderr=include_stderr)
 
     def ip_link_show(self, xdp):
         _, link = ip("link show dev %s" % (self['ifname']))
@@ -373,7 +386,7 @@  netns = [] # net namespaces to be removed
         return filters
 
     def cls_bpf_add_filter(self, bpf, da=False, skip_sw=False, skip_hw=False,
-                           fail=True):
+                           fail=True, include_stderr=False):
         params = ""
         if da:
             params += " da"
@@ -382,7 +395,8 @@  netns = [] # net namespaces to be removed
         if skip_hw:
             params += " skip_hw"
         return tc("filter add dev %s ingress bpf %s %s" %
-                  (self['ifname'], bpf, params), fail=fail)
+                  (self['ifname'], bpf, params), fail=fail,
+                  include_stderr=include_stderr)
 
     def set_ethtool_tc_offloads(self, enable, fail=True):
         args = "hw-tc-offload %s" % ("on" if enable else "off")
@@ -434,9 +448,18 @@  netns = [] # net namespaces to be removed
             fail(dev["ns_dev"] != 0, "Device perameters not zero on removed")
             fail(dev["ns_inode"] != 0, "Device perameters not zero on removed")
 
+def check_extack(output, reference, args):
+    if args.skip_extack:
+        return
+    lines = output.split("\n")
+    comp = len(lines) >= 2 and lines[1] == reference
+    fail(not comp, "Missing or incorrect netlink extack message")
+
 # Parse command line
 parser = argparse.ArgumentParser()
 parser.add_argument("--log", help="output verbose log to given file")
+parser.add_argument("--skip-extack", action="store_true",
+                    help="skip checks on TC netlink extack messages")
 args = parser.parse_args()
 if args.log:
     logfile = open(args.log, 'w+')
@@ -501,8 +524,10 @@  netns = []
     sim.tc_flush_filters()
 
     start_test("Test TC offloads are off by default...")
-    ret, _ = sim.cls_bpf_add_filter(obj, skip_sw=True, fail=False)
+    ret, _, err = sim.cls_bpf_add_filter(obj, skip_sw=True, fail=False,
+                                         include_stderr=True)
     fail(ret == 0, "TC filter loaded without enabling TC offloads")
+    check_extack(err, "Error: TC offload is disabled on net device.", args)
     sim.wait_for_flush()
 
     sim.set_ethtool_tc_offloads(True)
@@ -530,8 +555,10 @@  netns = []
     sim.dfs["bpf_tc_non_bound_accept"] = "N"
 
     start_test("Test TC cBPF unbound bytecode doesn't offload...")
-    ret, _ = sim.cls_bpf_add_filter(bytecode, skip_sw=True, fail=False)
+    ret, _, err = sim.cls_bpf_add_filter(bytecode, skip_sw=True, fail=False,
+                                         include_stderr=True)
     fail(ret == 0, "TC bytecode loaded for offload")
+    check_extack(err, "Error: netdevsim: netdevsim configured to reject unbound programs.", args)
     sim.wait_for_flush()
 
     start_test("Test TC offloads work...")
@@ -612,16 +639,24 @@  netns = []
          "Device parameters reported for non-offloaded program")
 
     start_test("Test XDP prog replace with bad flags...")
-    ret, _ = sim.set_xdp(obj, "offload", force=True, fail=False)
+    ret, _, err = sim.set_xdp(obj, "offload", force=True, fail=False,
+                              include_stderr=True)
     fail(ret == 0, "Replaced XDP program with a program in different mode")
-    ret, _ = sim.set_xdp(obj, "", force=True, fail=False)
+    check_extack(err, "Error: netdevsim: program loaded with different flags.", args)
+    ret, _, err = sim.set_xdp(obj, "", force=True, fail=False,
+                              include_stderr=True)
     fail(ret == 0, "Replaced XDP program with a program in different mode")
+    check_extack(err, "Error: netdevsim: program loaded with different flags.", args)
 
     start_test("Test XDP prog remove with bad flags...")
-    ret, _ = sim.unset_xdp("offload", force=True, fail=False)
+    ret, _, err = sim.unset_xdp("offload", force=True, fail=False,
+                                include_stderr=True)
     fail(ret == 0, "Removed program with a bad mode mode")
-    ret, _ = sim.unset_xdp("", force=True, fail=False)
+    check_extack(err, "Error: netdevsim: program loaded with different flags.", args)
+    ret, _, err = sim.unset_xdp("", force=True, fail=False,
+                                include_stderr=True)
     fail(ret == 0, "Removed program with a bad mode mode")
+    check_extack(err, "Error: netdevsim: program loaded with different flags.", args)
 
     start_test("Test MTU restrictions...")
     ret, _ = sim.set_mtu(9000, fail=False)
@@ -630,8 +665,9 @@  netns = []
     sim.unset_xdp("drv")
     bpftool_prog_list_wait(expected=0)
     sim.set_mtu(9000)
-    ret, _ = sim.set_xdp(obj, "drv", fail=False)
+    ret, _, err = sim.set_xdp(obj, "drv", fail=False, include_stderr=True)
     fail(ret == 0, "Driver should refuse to load program with MTU of 9000...")
+    check_extack(err, "Error: netdevsim: MTU too large w/ XDP enabled.", args)
     sim.set_mtu(1500)
 
     sim.wait_for_flush()
@@ -667,25 +703,32 @@  netns = []
     sim2.set_xdp(obj, "offload")
     pin_file, pinned = pin_prog("/sys/fs/bpf/tmp")
 
-    ret, _ = sim.set_xdp(pinned, "offload", fail=False)
+    ret, _, err = sim.set_xdp(pinned, "offload", fail=False,
+                              include_stderr=True)
     fail(ret == 0, "Pinned program loaded for a different device accepted")
+    check_extack(err, "Error: netdevsim: program bound to different dev.", args)
     sim2.remove()
-    ret, _ = sim.set_xdp(pinned, "offload", fail=False)
+    ret, _, err = sim.set_xdp(pinned, "offload", fail=False,
+                              include_stderr=True)
     fail(ret == 0, "Pinned program loaded for a removed device accepted")
+    check_extack(err, "Error: netdevsim: xdpoffload of non-bound program.", args)
     rm(pin_file)
     bpftool_prog_list_wait(expected=0)
 
     start_test("Test mixing of TC and XDP...")
     sim.tc_add_ingress()
     sim.set_xdp(obj, "offload")
-    ret, _ = sim.cls_bpf_add_filter(obj, skip_sw=True, fail=False)
+    ret, _, err = sim.cls_bpf_add_filter(obj, skip_sw=True, fail=False,
+                                         include_stderr=True)
     fail(ret == 0, "Loading TC when XDP active should fail")
+    check_extack(err, "Error: netdevsim: driver and netdev offload states mismatch.", args)
     sim.unset_xdp("offload")
     sim.wait_for_flush()
 
     sim.cls_bpf_add_filter(obj, skip_sw=True)
-    ret, _ = sim.set_xdp(obj, "offload", fail=False)
+    ret, _, err = sim.set_xdp(obj, "offload", fail=False, include_stderr=True)
     fail(ret == 0, "Loading XDP when TC active should fail")
+    check_extack(err, "Error: netdevsim: TC program is already loaded.", args)
 
     start_test("Test binding TC from pinned...")
     pin_file, pinned = pin_prog("/sys/fs/bpf/tmp")
@@ -708,8 +751,10 @@  netns = []
 
     start_test("Test asking for TC offload of two filters...")
     sim.cls_bpf_add_filter(obj, da=True, skip_sw=True)
-    ret, _ = sim.cls_bpf_add_filter(obj, da=True, skip_sw=True, fail=False)
+    ret, _, err = sim.cls_bpf_add_filter(obj, da=True, skip_sw=True,
+                                         fail=False, include_stderr=True)
     fail(ret == 0, "Managed to offload two TC filters at the same time")
+    check_extack(err, "Error: netdevsim: driver and netdev offload states mismatch.", args)
 
     sim.tc_flush_filters(bound=2, total=2)