* [PATCH bpf-next v2 0/2] bpf/flow_dissector: add mode to enforce global BPF flow dissector @ 2019-10-04 15:56 Stanislav Fomichev 2019-10-04 15:56 ` [PATCH bpf-next v2 1/2] " Stanislav Fomichev 2019-10-04 15:56 ` [PATCH bpf-next v2 2/2] selftests/bpf: add test for BPF flow dissector in the root namespace Stanislav Fomichev 0 siblings, 2 replies; 7+ messages in thread From: Stanislav Fomichev @ 2019-10-04 15:56 UTC (permalink / raw) To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Petar Penkov While having a per-net-ns flow dissector programs is convenient for testing, security-wise it's better to have only one vetted global flow dissector implementation. Let's have a convention that when BPF flow dissector is installed in the root namespace, child namespaces can't override it. The intended use-case is to attach global BPF flow dissector early from the init scripts/systemd. Attaching global dissector is prohibited if some non-root namespace already has flow dissector attached. Also, attaching to non-root namespace is prohibited when there is flow dissector attached to the root namespace. v2: * EPERM -> EEXIST (Song Liu) * Make sure we don't have dissector attached to non-root namespaces when attaching the global one (Andrii Nakryiko) Cc: Petar Penkov <ppenkov@google.com> Stanislav Fomichev (2): bpf/flow_dissector: add mode to enforce global BPF flow dissector selftests/bpf: add test for BPF flow dissector in the root namespace Documentation/bpf/prog_flow_dissector.rst | 3 ++ net/core/flow_dissector.c | 42 ++++++++++++++-- .../selftests/bpf/test_flow_dissector.sh | 48 ++++++++++++++++--- 3 files changed, 83 insertions(+), 10 deletions(-) -- 2.23.0.581.g78d2f28ef7-goog ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next v2 1/2] bpf/flow_dissector: add mode to enforce global BPF flow dissector 2019-10-04 15:56 [PATCH bpf-next v2 0/2] bpf/flow_dissector: add mode to enforce global BPF flow dissector Stanislav Fomichev @ 2019-10-04 15:56 ` Stanislav Fomichev 2019-10-04 17:53 ` Song Liu 2019-10-05 18:34 ` Andrii Nakryiko 2019-10-04 15:56 ` [PATCH bpf-next v2 2/2] selftests/bpf: add test for BPF flow dissector in the root namespace Stanislav Fomichev 1 sibling, 2 replies; 7+ messages in thread From: Stanislav Fomichev @ 2019-10-04 15:56 UTC (permalink / raw) To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Petar Penkov Always use init_net flow dissector BPF program if it's attached and fall back to the per-net namespace one. Also, deny installing new programs if there is already one attached to the root namespace. Users can still detach their BPF programs, but can't attach any new ones (-EEXIST). Cc: Petar Penkov <ppenkov@google.com> Signed-off-by: Stanislav Fomichev <sdf@google.com> --- Documentation/bpf/prog_flow_dissector.rst | 3 ++ net/core/flow_dissector.c | 42 ++++++++++++++++++++--- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/Documentation/bpf/prog_flow_dissector.rst b/Documentation/bpf/prog_flow_dissector.rst index a78bf036cadd..4d86780ab0f1 100644 --- a/Documentation/bpf/prog_flow_dissector.rst +++ b/Documentation/bpf/prog_flow_dissector.rst @@ -142,3 +142,6 @@ BPF flow dissector doesn't support exporting all the metadata that in-kernel C-based implementation can export. Notable example is single VLAN (802.1Q) and double VLAN (802.1AD) tags. Please refer to the ``struct bpf_flow_keys`` for a set of information that's currently can be exported from the BPF context. + +When BPF flow dissector is attached to the root network namespace (machine-wide +policy), users can't override it in their child network namespaces. diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index 7c09d87d3269..9821e730fc70 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -114,19 +114,50 @@ int skb_flow_dissector_bpf_prog_attach(const union bpf_attr *attr, { struct bpf_prog *attached; struct net *net; + int ret = 0; net = current->nsproxy->net_ns; mutex_lock(&flow_dissector_mutex); + + if (net == &init_net) { + /* BPF flow dissector in the root namespace overrides + * any per-net-namespace one. When attaching to root, + * make sure we don't have any BPF program attached + * to the non-root namespaces. + */ + struct net *ns; + + for_each_net(ns) { + if (net == &init_net) + continue; + + if (rcu_access_pointer(ns->flow_dissector_prog)) { + ret = -EEXIST; + goto out; + } + } + } else { + /* Make sure root flow dissector is not attached + * when attaching to the non-root namespace. + */ + + if (rcu_access_pointer(init_net.flow_dissector_prog)) { + ret = -EEXIST; + goto out; + } + } + attached = rcu_dereference_protected(net->flow_dissector_prog, lockdep_is_held(&flow_dissector_mutex)); if (attached) { /* Only one BPF program can be attached at a time */ - mutex_unlock(&flow_dissector_mutex); - return -EEXIST; + ret = -EEXIST; + goto out; } rcu_assign_pointer(net->flow_dissector_prog, prog); +out: mutex_unlock(&flow_dissector_mutex); - return 0; + return ret; } int skb_flow_dissector_bpf_prog_detach(const union bpf_attr *attr) @@ -910,7 +941,10 @@ bool __skb_flow_dissect(const struct net *net, WARN_ON_ONCE(!net); if (net) { rcu_read_lock(); - attached = rcu_dereference(net->flow_dissector_prog); + attached = rcu_dereference(init_net.flow_dissector_prog); + + if (!attached) + attached = rcu_dereference(net->flow_dissector_prog); if (attached) { struct bpf_flow_keys flow_keys; -- 2.23.0.581.g78d2f28ef7-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf/flow_dissector: add mode to enforce global BPF flow dissector 2019-10-04 15:56 ` [PATCH bpf-next v2 1/2] " Stanislav Fomichev @ 2019-10-04 17:53 ` Song Liu 2019-10-05 18:34 ` Andrii Nakryiko 1 sibling, 0 replies; 7+ messages in thread From: Song Liu @ 2019-10-04 17:53 UTC (permalink / raw) To: Stanislav Fomichev Cc: Networking, bpf, David S . Miller, Alexei Starovoitov, Daniel Borkmann, Petar Penkov On Fri, Oct 4, 2019 at 8:58 AM Stanislav Fomichev <sdf@google.com> wrote: > > Always use init_net flow dissector BPF program if it's attached and fall > back to the per-net namespace one. Also, deny installing new programs if > there is already one attached to the root namespace. > Users can still detach their BPF programs, but can't attach any > new ones (-EEXIST). > > Cc: Petar Penkov <ppenkov@google.com> > Signed-off-by: Stanislav Fomichev <sdf@google.com> Acked-by: Song Liu <songliubraving@fb.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf/flow_dissector: add mode to enforce global BPF flow dissector 2019-10-04 15:56 ` [PATCH bpf-next v2 1/2] " Stanislav Fomichev 2019-10-04 17:53 ` Song Liu @ 2019-10-05 18:34 ` Andrii Nakryiko 2019-10-07 15:27 ` Stanislav Fomichev 1 sibling, 1 reply; 7+ messages in thread From: Andrii Nakryiko @ 2019-10-05 18:34 UTC (permalink / raw) To: Stanislav Fomichev Cc: Networking, bpf, David S. Miller, Alexei Starovoitov, Daniel Borkmann, Petar Penkov On Fri, Oct 4, 2019 at 8:58 AM Stanislav Fomichev <sdf@google.com> wrote: > > Always use init_net flow dissector BPF program if it's attached and fall > back to the per-net namespace one. Also, deny installing new programs if > there is already one attached to the root namespace. > Users can still detach their BPF programs, but can't attach any > new ones (-EEXIST). > > Cc: Petar Penkov <ppenkov@google.com> > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- Looks good, but see my note below. Regardless: Acked-by: Andrii Nakryiko <andriin@fb.com> > Documentation/bpf/prog_flow_dissector.rst | 3 ++ > net/core/flow_dissector.c | 42 ++++++++++++++++++++--- > 2 files changed, 41 insertions(+), 4 deletions(-) > > diff --git a/Documentation/bpf/prog_flow_dissector.rst b/Documentation/bpf/prog_flow_dissector.rst > index a78bf036cadd..4d86780ab0f1 100644 > --- a/Documentation/bpf/prog_flow_dissector.rst > +++ b/Documentation/bpf/prog_flow_dissector.rst > @@ -142,3 +142,6 @@ BPF flow dissector doesn't support exporting all the metadata that in-kernel > C-based implementation can export. Notable example is single VLAN (802.1Q) > and double VLAN (802.1AD) tags. Please refer to the ``struct bpf_flow_keys`` > for a set of information that's currently can be exported from the BPF context. > + > +When BPF flow dissector is attached to the root network namespace (machine-wide > +policy), users can't override it in their child network namespaces. > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > index 7c09d87d3269..9821e730fc70 100644 > --- a/net/core/flow_dissector.c > +++ b/net/core/flow_dissector.c > @@ -114,19 +114,50 @@ int skb_flow_dissector_bpf_prog_attach(const union bpf_attr *attr, > { > struct bpf_prog *attached; > struct net *net; > + int ret = 0; > > net = current->nsproxy->net_ns; > mutex_lock(&flow_dissector_mutex); > + > + if (net == &init_net) { > + /* BPF flow dissector in the root namespace overrides > + * any per-net-namespace one. When attaching to root, > + * make sure we don't have any BPF program attached > + * to the non-root namespaces. > + */ > + struct net *ns; > + > + for_each_net(ns) { > + if (net == &init_net) > + continue; You don't need this condition, if something is attached to init_net, you will return -EEXIST anyway. Or is this a performance optimization? > + > + if (rcu_access_pointer(ns->flow_dissector_prog)) { > + ret = -EEXIST; > + goto out; > + } > + } > + } else { > + /* Make sure root flow dissector is not attached > + * when attaching to the non-root namespace. > + */ > + nit: extra empty line > + if (rcu_access_pointer(init_net.flow_dissector_prog)) { > + ret = -EEXIST; > + goto out; > + } > + } > + > attached = rcu_dereference_protected(net->flow_dissector_prog, > lockdep_is_held(&flow_dissector_mutex)); > if (attached) { > /* Only one BPF program can be attached at a time */ > - mutex_unlock(&flow_dissector_mutex); > - return -EEXIST; > + ret = -EEXIST; > + goto out; > } > rcu_assign_pointer(net->flow_dissector_prog, prog); > +out: > mutex_unlock(&flow_dissector_mutex); > - return 0; > + return ret; > } > > int skb_flow_dissector_bpf_prog_detach(const union bpf_attr *attr) > @@ -910,7 +941,10 @@ bool __skb_flow_dissect(const struct net *net, > WARN_ON_ONCE(!net); > if (net) { > rcu_read_lock(); > - attached = rcu_dereference(net->flow_dissector_prog); > + attached = rcu_dereference(init_net.flow_dissector_prog); > + > + if (!attached) > + attached = rcu_dereference(net->flow_dissector_prog); > > if (attached) { > struct bpf_flow_keys flow_keys; > -- > 2.23.0.581.g78d2f28ef7-goog > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpf/flow_dissector: add mode to enforce global BPF flow dissector 2019-10-05 18:34 ` Andrii Nakryiko @ 2019-10-07 15:27 ` Stanislav Fomichev 0 siblings, 0 replies; 7+ messages in thread From: Stanislav Fomichev @ 2019-10-07 15:27 UTC (permalink / raw) To: Andrii Nakryiko Cc: Stanislav Fomichev, Networking, bpf, David S. Miller, Alexei Starovoitov, Daniel Borkmann, Petar Penkov On 10/05, Andrii Nakryiko wrote: > On Fri, Oct 4, 2019 at 8:58 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > Always use init_net flow dissector BPF program if it's attached and fall > > back to the per-net namespace one. Also, deny installing new programs if > > there is already one attached to the root namespace. > > Users can still detach their BPF programs, but can't attach any > > new ones (-EEXIST). > > > > Cc: Petar Penkov <ppenkov@google.com> > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > Looks good, but see my note below. Regardless: > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > > Documentation/bpf/prog_flow_dissector.rst | 3 ++ > > net/core/flow_dissector.c | 42 ++++++++++++++++++++--- > > 2 files changed, 41 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/bpf/prog_flow_dissector.rst b/Documentation/bpf/prog_flow_dissector.rst > > index a78bf036cadd..4d86780ab0f1 100644 > > --- a/Documentation/bpf/prog_flow_dissector.rst > > +++ b/Documentation/bpf/prog_flow_dissector.rst > > @@ -142,3 +142,6 @@ BPF flow dissector doesn't support exporting all the metadata that in-kernel > > C-based implementation can export. Notable example is single VLAN (802.1Q) > > and double VLAN (802.1AD) tags. Please refer to the ``struct bpf_flow_keys`` > > for a set of information that's currently can be exported from the BPF context. > > + > > +When BPF flow dissector is attached to the root network namespace (machine-wide > > +policy), users can't override it in their child network namespaces. > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > > index 7c09d87d3269..9821e730fc70 100644 > > --- a/net/core/flow_dissector.c > > +++ b/net/core/flow_dissector.c > > @@ -114,19 +114,50 @@ int skb_flow_dissector_bpf_prog_attach(const union bpf_attr *attr, > > { > > struct bpf_prog *attached; > > struct net *net; > > + int ret = 0; > > > > net = current->nsproxy->net_ns; > > mutex_lock(&flow_dissector_mutex); > > + > > + if (net == &init_net) { > > + /* BPF flow dissector in the root namespace overrides > > + * any per-net-namespace one. When attaching to root, > > + * make sure we don't have any BPF program attached > > + * to the non-root namespaces. > > + */ > > + struct net *ns; > > + > > + for_each_net(ns) { > > + if (net == &init_net) > > + continue; > > You don't need this condition, if something is attached to init_net, > you will return -EEXIST anyway. Or is this a performance optimization? Ah, I agree, will remove an respin. > > + > > + if (rcu_access_pointer(ns->flow_dissector_prog)) { > > + ret = -EEXIST; > > + goto out; > > + } > > + } > > + } else { > > + /* Make sure root flow dissector is not attached > > + * when attaching to the non-root namespace. > > + */ > > + > > nit: extra empty line Thx, will fix. > > + if (rcu_access_pointer(init_net.flow_dissector_prog)) { > > + ret = -EEXIST; > > + goto out; > > + } > > + } > > + > > attached = rcu_dereference_protected(net->flow_dissector_prog, > > lockdep_is_held(&flow_dissector_mutex)); > > if (attached) { > > /* Only one BPF program can be attached at a time */ > > - mutex_unlock(&flow_dissector_mutex); > > - return -EEXIST; > > + ret = -EEXIST; > > + goto out; > > } > > rcu_assign_pointer(net->flow_dissector_prog, prog); > > +out: > > mutex_unlock(&flow_dissector_mutex); > > - return 0; > > + return ret; > > } > > > > int skb_flow_dissector_bpf_prog_detach(const union bpf_attr *attr) > > @@ -910,7 +941,10 @@ bool __skb_flow_dissect(const struct net *net, > > WARN_ON_ONCE(!net); > > if (net) { > > rcu_read_lock(); > > - attached = rcu_dereference(net->flow_dissector_prog); > > + attached = rcu_dereference(init_net.flow_dissector_prog); > > + > > + if (!attached) > > + attached = rcu_dereference(net->flow_dissector_prog); > > > > if (attached) { > > struct bpf_flow_keys flow_keys; > > -- > > 2.23.0.581.g78d2f28ef7-goog > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf-next v2 2/2] selftests/bpf: add test for BPF flow dissector in the root namespace 2019-10-04 15:56 [PATCH bpf-next v2 0/2] bpf/flow_dissector: add mode to enforce global BPF flow dissector Stanislav Fomichev 2019-10-04 15:56 ` [PATCH bpf-next v2 1/2] " Stanislav Fomichev @ 2019-10-04 15:56 ` Stanislav Fomichev 2019-10-04 17:54 ` Song Liu 1 sibling, 1 reply; 7+ messages in thread From: Stanislav Fomichev @ 2019-10-04 15:56 UTC (permalink / raw) To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Petar Penkov Make sure non-root namespaces get an error if root flow dissector is attached. Cc: Petar Penkov <ppenkov@google.com> Signed-off-by: Stanislav Fomichev <sdf@google.com> --- .../selftests/bpf/test_flow_dissector.sh | 48 ++++++++++++++++--- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/tools/testing/selftests/bpf/test_flow_dissector.sh b/tools/testing/selftests/bpf/test_flow_dissector.sh index d23d4da66b83..2c3a25d64faf 100755 --- a/tools/testing/selftests/bpf/test_flow_dissector.sh +++ b/tools/testing/selftests/bpf/test_flow_dissector.sh @@ -18,19 +18,55 @@ fi # this is the case and run it with in_netns.sh if it is being run in the root # namespace. if [[ -z $(ip netns identify $$) ]]; then + err=0 + if bpftool="$(which bpftool)"; then + echo "Testing global flow dissector..." + + $bpftool prog loadall ./bpf_flow.o /sys/fs/bpf/flow \ + type flow_dissector + + if ! unshare --net $bpftool prog attach pinned \ + /sys/fs/bpf/flow/flow_dissector flow_dissector; then + echo "Unexpected unsuccessful attach in namespace" >&2 + err=1 + fi + + $bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector \ + flow_dissector + + if unshare --net $bpftool prog attach pinned \ + /sys/fs/bpf/flow/flow_dissector flow_dissector; then + echo "Unexpected successful attach in namespace" >&2 + err=1 + fi + + if ! $bpftool prog detach pinned \ + /sys/fs/bpf/flow/flow_dissector flow_dissector; then + echo "Failed to detach flow dissector" >&2 + err=1 + fi + + rm -rf /sys/fs/bpf/flow + else + echo "Skipping root flow dissector test, bpftool not found" >&2 + fi + + # Run the rest of the tests in a net namespace. ../net/in_netns.sh "$0" "$@" - exit $? -fi + err=$(( $err + $? )) -# Determine selftest success via shell exit code -exit_handler() -{ - if (( $? == 0 )); then + if (( $err == 0 )); then echo "selftests: $TESTNAME [PASS]"; else echo "selftests: $TESTNAME [FAILED]"; fi + exit $err +fi + +# Determine selftest success via shell exit code +exit_handler() +{ set +e # Cleanup -- 2.23.0.581.g78d2f28ef7-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: add test for BPF flow dissector in the root namespace 2019-10-04 15:56 ` [PATCH bpf-next v2 2/2] selftests/bpf: add test for BPF flow dissector in the root namespace Stanislav Fomichev @ 2019-10-04 17:54 ` Song Liu 0 siblings, 0 replies; 7+ messages in thread From: Song Liu @ 2019-10-04 17:54 UTC (permalink / raw) To: Stanislav Fomichev Cc: Networking, bpf, David S . Miller, Alexei Starovoitov, Daniel Borkmann, Petar Penkov On Fri, Oct 4, 2019 at 8:58 AM Stanislav Fomichev <sdf@google.com> wrote: > > Make sure non-root namespaces get an error if root flow dissector is > attached. > > Cc: Petar Penkov <ppenkov@google.com> > Signed-off-by: Stanislav Fomichev <sdf@google.com> Acked-by: Song Liu <songliubraving@fb.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-07 15:27 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-04 15:56 [PATCH bpf-next v2 0/2] bpf/flow_dissector: add mode to enforce global BPF flow dissector Stanislav Fomichev 2019-10-04 15:56 ` [PATCH bpf-next v2 1/2] " Stanislav Fomichev 2019-10-04 17:53 ` Song Liu 2019-10-05 18:34 ` Andrii Nakryiko 2019-10-07 15:27 ` Stanislav Fomichev 2019-10-04 15:56 ` [PATCH bpf-next v2 2/2] selftests/bpf: add test for BPF flow dissector in the root namespace Stanislav Fomichev 2019-10-04 17:54 ` Song Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).