[PATCH net 0/3] fix BPF offload related bugs

[PATCH net 0/3] fix BPF offload related bugs

[Jakub Kicinski]

Hi!

test_offload.py catches some recently added bugs.

First of a bug in test_offload.py itself after recent changes
to netdevsim is fixed.

Second patch fixes a bug in cls_bpf, and last one addresses
a problem with the recently added XDP installation optimization.

Jakub Kicinski (3):
  selftests: bpf: Skip write only files in debugfs
  net: cls_bpf: fix NULL deref on offload filter removal
  net: fix installing orphaned programs

 net/core/dev.c                              | 3 ++-
 net/sched/cls_bpf.c                         | 8 ++++++--
 tools/testing/selftests/bpf/test_offload.py | 5 +++++
 3 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.23.0

[PATCH net 1/3] selftests: bpf: Skip write only files in debugfs

[Jakub Kicinski]

DebugFS for netdevsim now contains some "action trigger" files
which are write only. Don't try to capture the contents of those.

Note that we can't use os.access() because the script requires
root.

Fixes: 4418f862d675 ("netdevsim: implement support for devlink region and snapshots")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
CC: Jiri Pirko <jiri@mellanox.com>

 tools/testing/selftests/bpf/test_offload.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_offload.py b/tools/testing/selftests/bpf/test_offload.py
index 15a666329a34..1afa22c88e42 100755
--- a/tools/testing/selftests/bpf/test_offload.py
+++ b/tools/testing/selftests/bpf/test_offload.py
@@ -22,6 +22,7 @@ import os
 import pprint
 import random
 import re
+import stat
 import string
 import struct
 import subprocess
@@ -311,7 +312,11 @@ def bpftool_prog_load(sample, file_name, maps=[], prog_type="xdp", dev=None,
         for f in out.split():
             if f == "ports":
                 continue
+
             p = os.path.join(path, f)
+            if not os.stat(p).st_mode & stat.S_IRUSR:
+                continue
+
             if os.path.isfile(p):
                 _, out = cmd('cat %s/%s' % (path, f))
                 dfs[f] = out.strip()
-- 
2.23.0

[PATCH net 2/3] net: cls_bpf: fix NULL deref on offload filter removal

[Jakub Kicinski]

Commit 401192113730 ("net: sched: refactor block offloads counter
usage") missed the fact that either new prog or old prog may be
NULL.

Fixes: 401192113730 ("net: sched: refactor block offloads counter usage")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
CC: Vlad Buslov <vladbu@mellanox.com>

 net/sched/cls_bpf.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index bf10bdaf5012..8229ed4a67be 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -162,16 +162,20 @@ static int cls_bpf_offload_cmd(struct tcf_proto *tp, struct cls_bpf_prog *prog,
 	cls_bpf.name = obj->bpf_name;
 	cls_bpf.exts_integrated = obj->exts_integrated;
 
-	if (oldprog)
+	if (oldprog && prog)
 		err = tc_setup_cb_replace(block, tp, TC_SETUP_CLSBPF, &cls_bpf,
 					  skip_sw, &oldprog->gen_flags,
 					  &oldprog->in_hw_count,
 					  &prog->gen_flags, &prog->in_hw_count,
 					  true);
-	else
+	else if (prog)
 		err = tc_setup_cb_add(block, tp, TC_SETUP_CLSBPF, &cls_bpf,
 				      skip_sw, &prog->gen_flags,
 				      &prog->in_hw_count, true);
+	else
+		err = tc_setup_cb_destroy(block, tp, TC_SETUP_CLSBPF, &cls_bpf,
+					  skip_sw, &oldprog->gen_flags,
+					  &oldprog->in_hw_count, true);
 
 	if (prog && err) {
 		cls_bpf_offload_cmd(tp, oldprog, prog, extack);
-- 
2.23.0

[PATCH net 3/3] net: fix installing orphaned programs

[Jakub Kicinski]

When netdevice with offloaded BPF programs is destroyed
the programs are orphaned and removed from the program
IDA - their IDs get released (the programs may remain
accessible via existing open file descriptors and pinned
files). After IDs are released they are set to 0.

This confuses dev_change_xdp_fd() because it compares
the __dev_xdp_query() result where 0 means no program
with prog->aux->id where 0 means orphaned.

dev_change_xdp_fd() would have incorrectly returned success
even though it had not installed the program.

Since drivers already catch this case via bpf_offload_dev_match()
let them handle this case. The error message drivers produce in
this case ("program loaded for a different device") is in fact
correct as the orphaned program must had to be loaded for a
different device.

Fixes: c14a9f633d9e ("net: Don't call XDP_SETUP_PROG when nothing is changed")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
CC: Maxim Mikityanskiy <maximmi@mellanox.com>

 net/core/dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 96afd464284a..99ac84ff398f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -8421,7 +8421,8 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
 			return -EINVAL;
 		}
 
-		if (prog->aux->id == prog_id) {
+		/* prog->aux->id may be 0 for orphaned device-bound progs */
+		if (prog->aux->id && prog->aux->id == prog_id) {
 			bpf_prog_put(prog);
 			return 0;
 		}
-- 
2.23.0

Re: [PATCH net 0/3] fix BPF offload related bugs

[Daniel Borkmann]

On 11/1/19 4:06 AM, Jakub Kicinski wrote:
> Hi!
> 
> test_offload.py catches some recently added bugs.
> 
> First of a bug in test_offload.py itself after recent changes
> to netdevsim is fixed.
> 
> Second patch fixes a bug in cls_bpf, and last one addresses
> a problem with the recently added XDP installation optimization.
> 
> Jakub Kicinski (3):
>    selftests: bpf: Skip write only files in debugfs
>    net: cls_bpf: fix NULL deref on offload filter removal
>    net: fix installing orphaned programs
> 
>   net/core/dev.c                              | 3 ++-
>   net/sched/cls_bpf.c                         | 8 ++++++--
>   tools/testing/selftests/bpf/test_offload.py | 5 +++++
>   3 files changed, 13 insertions(+), 3 deletions(-)

Should this go via -bpf or -net? Either way is fine, but asking
given it's BPF related fixes; planning to do a PR in the evening,
set looks good to me in any case.

Thanks,
Daniel

Re: [PATCH net 0/3] fix BPF offload related bugs

[Jakub Kicinski]

On Fri, 1 Nov 2019 13:10:13 +0100, Daniel Borkmann wrote:
> On 11/1/19 4:06 AM, Jakub Kicinski wrote:
> > Hi!
> > 
> > test_offload.py catches some recently added bugs.
> > 
> > First of a bug in test_offload.py itself after recent changes
> > to netdevsim is fixed.
> > 
> > Second patch fixes a bug in cls_bpf, and last one addresses
> > a problem with the recently added XDP installation optimization.
> > 
> > Jakub Kicinski (3):
> >    selftests: bpf: Skip write only files in debugfs
> >    net: cls_bpf: fix NULL deref on offload filter removal
> >    net: fix installing orphaned programs
> > 
> >   net/core/dev.c                              | 3 ++-
> >   net/sched/cls_bpf.c                         | 8 ++++++--
> >   tools/testing/selftests/bpf/test_offload.py | 5 +++++
> >   3 files changed, 13 insertions(+), 3 deletions(-)  
> 
> Should this go via -bpf or -net? Either way is fine, but asking
> given it's BPF related fixes; planning to do a PR in the evening,
> set looks good to me in any case.

FWIW I'm fine either way, too. I made it net after Alexei wondered if 
we should apply the revert to net-next, but since you took the revert 
to bpf-next perhaps bpf makes sense.

To state the obvious the only thing that matters is for the revert to
be in net-next when these are merged into net-next (IOW bpf-next PR is
what matters most at this point ;)).

Re: [PATCH net 0/3] fix BPF offload related bugs

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Thu, 31 Oct 2019 20:06:57 -0700

> test_offload.py catches some recently added bugs.
> 
> First of a bug in test_offload.py itself after recent changes
> to netdevsim is fixed.
> 
> Second patch fixes a bug in cls_bpf, and last one addresses
> a problem with the recently added XDP installation optimization.

Series applied and queued up for -stable.