bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector
@ 2020-06-29  9:56 Lorenz Bauer
  2020-06-29  9:56 ` [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH Lorenz Bauer
                   ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Lorenz Bauer @ 2020-06-29  9:56 UTC (permalink / raw)
  To: ast, daniel, sdf, jakub, john.fastabend; +Cc: kernel-team, bpf, Lorenz Bauer

Both sockmap and flow_dissector ingnore various arguments passed to
BPF_PROG_ATTACH and BPF_PROG_DETACH. We can fix the attach case by
checking that the unused arguments are zero. I considered requiring
target_fd to be -1 instead of 0, but this leads to a lot of churn
in selftests. There is also precedent in that bpf_iter already
expects 0 for a similar field. I think that we can come up with a
work around for fd 0 should we need to in the future.

The detach case is more problematic: both cgroups and lirc2 verify
that attach_bpf_fd matches the currently attached program. This
way you need access to the program fd to be able to remove it.
Neither sockmap nor flow_dissector do this. flow_dissector even
has a check for CAP_NET_ADMIN because of this. The patch set
addresses this by implementing the desired behaviour.

There is a possibility for user space breakage: any callers that
don't provide the correct fd will fail with ENOENT. For sockmap
the risk is low: even the selftests assume that sockmap works
the way I described. For flow_dissector the story is less
straightforward, and the selftests use a variety of arguments.

I've includes fixes tags for the oldest commits that allow an easy
backport, however the behaviour dates back to when sockmap and
flow_dissector were introduced. What is the best way to handle these?

This set is based on top of Jakub's work "bpf, netns: Prepare
for multi-prog attachment" available at
https://lore.kernel.org/bpf/87k0zwmhtb.fsf@cloudflare.com/T/

Since v1:
- Adjust selftests
- Implement detach behaviour

Lorenz Bauer (6):
  bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
  bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH
  bpf: sockmap: check value of unused args to BPF_PROG_ATTACH
  bpf: sockmap: require attach_bpf_fd when detaching a program
  selftests: bpf: pass program and target_fd in flow_dissector_reattach
  selftests: bpf: pass program to bpf_prog_detach in flow_dissector

 include/linux/bpf-netns.h                     |  5 +-
 include/linux/bpf.h                           | 13 ++++-
 include/linux/skmsg.h                         | 13 +++++
 kernel/bpf/net_namespace.c                    | 22 ++++++--
 kernel/bpf/syscall.c                          |  6 +--
 net/core/sock_map.c                           | 53 +++++++++++++++++--
 .../selftests/bpf/prog_tests/flow_dissector.c |  4 +-
 .../bpf/prog_tests/flow_dissector_reattach.c  | 12 ++---
 8 files changed, 103 insertions(+), 25 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
  2020-06-29  9:56 [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Lorenz Bauer
@ 2020-06-29  9:56 ` Lorenz Bauer
  2020-11-04 18:08   ` Jiri Benc
  2020-06-29  9:56 ` [PATCH bpf v2 2/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH Lorenz Bauer
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Lorenz Bauer @ 2020-06-29  9:56 UTC (permalink / raw)
  To: ast, daniel, sdf, jakub, john.fastabend; +Cc: kernel-team, bpf, Lorenz Bauer

Using BPF_PROG_ATTACH on a flow dissector program supports neither
target_fd, attach_flags or replace_bpf_fd but accepts any value.

Enforce that all of them are zero. This is fine for replace_bpf_fd
since its presence is indicated by BPF_F_REPLACE. It's more
problematic for target_fd, since zero is a valid fd. Should we
want to use the flag later on we'd have to add an exception for
fd 0. The alternative is to force a value like -1. This requires
more changes to tests. There is also precedent for using 0,
since bpf_iter uses this for target_fd as well.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: b27f7bb590ba ("flow_dissector: Move out netns_bpf prog callbacks")
---
 kernel/bpf/net_namespace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
index 3e89c7ad42cb..bf18eabeaea2 100644
--- a/kernel/bpf/net_namespace.c
+++ b/kernel/bpf/net_namespace.c
@@ -217,6 +217,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 	struct net *net;
 	int ret;
 
+	if (attr->target_fd || attr->attach_flags || attr->replace_bpf_fd)
+		return -EINVAL;
+
 	type = to_netns_bpf_attach_type(attr->attach_type);
 	if (type < 0)
 		return -EINVAL;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH bpf v2 2/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH
  2020-06-29  9:56 [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Lorenz Bauer
  2020-06-29  9:56 ` [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH Lorenz Bauer
@ 2020-06-29  9:56 ` Lorenz Bauer
  2020-06-29  9:56 ` [PATCH bpf v2 3/6] bpf: sockmap: check value of unused args to BPF_PROG_ATTACH Lorenz Bauer
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Lorenz Bauer @ 2020-06-29  9:56 UTC (permalink / raw)
  To: ast, daniel, sdf, jakub, john.fastabend; +Cc: kernel-team, bpf, Lorenz Bauer

Using BPF_PROG_DETACH on a flow dissector program supports neither
attach_flags nor attach_bpf_fd. Yet no value is enforced for them.

Enforce that attach_flags are zero, and require the current program
to be passed via attach_bpf_fd. This allows us to remove the check
for CAP_SYS_ADMIN, since userspace can now no longer remove
arbitrary flow dissector programs.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: b27f7bb590ba ("flow_dissector: Move out netns_bpf prog callbacks")
---
 include/linux/bpf-netns.h  |  5 +++--
 kernel/bpf/net_namespace.c | 19 +++++++++++++++----
 kernel/bpf/syscall.c       |  4 +---
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/include/linux/bpf-netns.h b/include/linux/bpf-netns.h
index 4052d649f36d..47d5b0c708c9 100644
--- a/include/linux/bpf-netns.h
+++ b/include/linux/bpf-netns.h
@@ -33,7 +33,7 @@ int netns_bpf_prog_query(const union bpf_attr *attr,
 			 union bpf_attr __user *uattr);
 int netns_bpf_prog_attach(const union bpf_attr *attr,
 			  struct bpf_prog *prog);
-int netns_bpf_prog_detach(const union bpf_attr *attr);
+int netns_bpf_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype);
 int netns_bpf_link_create(const union bpf_attr *attr,
 			  struct bpf_prog *prog);
 #else
@@ -49,7 +49,8 @@ static inline int netns_bpf_prog_attach(const union bpf_attr *attr,
 	return -EOPNOTSUPP;
 }
 
-static inline int netns_bpf_prog_detach(const union bpf_attr *attr)
+static inline int netns_bpf_prog_detach(const union bpf_attr *attr,
+					enum bpf_prog_type ptype)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
index bf18eabeaea2..7216215990ed 100644
--- a/kernel/bpf/net_namespace.c
+++ b/kernel/bpf/net_namespace.c
@@ -286,7 +286,8 @@ static void netns_bpf_run_array_detach(struct net *net,
 
 /* Must be called with netns_bpf_mutex held. */
 static int __netns_bpf_prog_detach(struct net *net,
-				   enum netns_bpf_attach_type type)
+				   enum netns_bpf_attach_type type,
+				   struct bpf_prog *old)
 {
 	struct bpf_prog *attached;
 
@@ -295,7 +296,7 @@ static int __netns_bpf_prog_detach(struct net *net,
 		return -EINVAL;
 
 	attached = net->bpf.progs[type];
-	if (!attached)
+	if (!attached || attached != old)
 		return -ENOENT;
 	netns_bpf_run_array_detach(net, type);
 	net->bpf.progs[type] = NULL;
@@ -303,19 +304,29 @@ static int __netns_bpf_prog_detach(struct net *net,
 	return 0;
 }
 
-int netns_bpf_prog_detach(const union bpf_attr *attr)
+int netns_bpf_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype)
 {
 	enum netns_bpf_attach_type type;
+	struct bpf_prog *prog;
 	int ret;
 
+	if (attr->target_fd)
+		return -EINVAL;
+
 	type = to_netns_bpf_attach_type(attr->attach_type);
 	if (type < 0)
 		return -EINVAL;
 
+	prog = bpf_prog_get_type(attr->attach_bpf_fd, ptype);
+	if (IS_ERR(prog))
+		return PTR_ERR(prog);
+
 	mutex_lock(&netns_bpf_mutex);
-	ret = __netns_bpf_prog_detach(current->nsproxy->net_ns, type);
+	ret = __netns_bpf_prog_detach(current->nsproxy->net_ns, type, prog);
 	mutex_unlock(&netns_bpf_mutex);
 
+	bpf_prog_put(prog);
+
 	return ret;
 }
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 8da159936bab..c0ec572f056c 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2897,9 +2897,7 @@ static int bpf_prog_detach(const union bpf_attr *attr)
 	case BPF_PROG_TYPE_LIRC_MODE2:
 		return lirc_prog_detach(attr);
 	case BPF_PROG_TYPE_FLOW_DISSECTOR:
-		if (!capable(CAP_NET_ADMIN))
-			return -EPERM;
-		return netns_bpf_prog_detach(attr);
+		return netns_bpf_prog_detach(attr, ptype);
 	case BPF_PROG_TYPE_CGROUP_DEVICE:
 	case BPF_PROG_TYPE_CGROUP_SKB:
 	case BPF_PROG_TYPE_CGROUP_SOCK:
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH bpf v2 3/6] bpf: sockmap: check value of unused args to BPF_PROG_ATTACH
  2020-06-29  9:56 [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Lorenz Bauer
  2020-06-29  9:56 ` [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH Lorenz Bauer
  2020-06-29  9:56 ` [PATCH bpf v2 2/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH Lorenz Bauer
@ 2020-06-29  9:56 ` Lorenz Bauer
  2020-06-29  9:56 ` [PATCH bpf v2 4/6] bpf: sockmap: require attach_bpf_fd when detaching a program Lorenz Bauer
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Lorenz Bauer @ 2020-06-29  9:56 UTC (permalink / raw)
  To: ast, daniel, sdf, jakub, john.fastabend; +Cc: kernel-team, bpf, Lorenz Bauer

Using BPF_PROG_ATTACH on a sockmap program currently understands no
flags or replace_bpf_fd, but accepts any value. Return EINVAL instead.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
---
 net/core/sock_map.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 4c1123c749bb..db45c1453d39 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -70,6 +70,9 @@ int sock_map_get_from_fd(const union bpf_attr *attr, struct bpf_prog *prog)
 	struct fd f;
 	int ret;
 
+	if (attr->attach_flags || attr->replace_bpf_fd)
+		return -EINVAL;
+
 	f = fdget(ufd);
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH bpf v2 4/6] bpf: sockmap: require attach_bpf_fd when detaching a program
  2020-06-29  9:56 [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Lorenz Bauer
                   ` (2 preceding siblings ...)
  2020-06-29  9:56 ` [PATCH bpf v2 3/6] bpf: sockmap: check value of unused args to BPF_PROG_ATTACH Lorenz Bauer
@ 2020-06-29  9:56 ` Lorenz Bauer
  2020-07-08  1:30   ` Martin KaFai Lau
  2020-06-29  9:56 ` [PATCH bpf v2 5/6] selftests: bpf: pass program and target_fd in flow_dissector_reattach Lorenz Bauer
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Lorenz Bauer @ 2020-06-29  9:56 UTC (permalink / raw)
  To: ast, daniel, sdf, jakub, john.fastabend; +Cc: kernel-team, bpf, Lorenz Bauer

The sockmap code currently ignores the value of attach_bpf_fd when
detaching a program. This is contrary to the usual behaviour of
checking that attach_bpf_fd represents the currently attached
program.

Ensure that attach_bpf_fd is indeed the currently attached
program. It turns out that all sockmap selftests already do this,
which indicates that this is unlikely to cause breakage.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface")
---
 include/linux/bpf.h   | 13 +++++++++--
 include/linux/skmsg.h | 13 +++++++++++
 kernel/bpf/syscall.c  |  2 +-
 net/core/sock_map.c   | 50 ++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 70 insertions(+), 8 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5d0506f46f24..6c3160fbae0b 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1555,13 +1555,16 @@ static inline void bpf_map_offload_map_free(struct bpf_map *map)
 #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
 
 #if defined(CONFIG_BPF_STREAM_PARSER)
-int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog, u32 which);
+int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
+			 struct bpf_prog *old, u32 which);
 int sock_map_get_from_fd(const union bpf_attr *attr, struct bpf_prog *prog);
+int sock_map_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype);
 void sock_map_unhash(struct sock *sk);
 void sock_map_close(struct sock *sk, long timeout);
 #else
 static inline int sock_map_prog_update(struct bpf_map *map,
-				       struct bpf_prog *prog, u32 which)
+				       struct bpf_prog *prog,
+				       struct bpf_prog *old, u32 which)
 {
 	return -EOPNOTSUPP;
 }
@@ -1571,6 +1574,12 @@ static inline int sock_map_get_from_fd(const union bpf_attr *attr,
 {
 	return -EINVAL;
 }
+
+static inline int sock_map_prog_detach(const union bpf_attr *attr,
+				       enum bpf_prog_type ptype)
+{
+	return -EOPNOTSUPP;
+}
 #endif /* CONFIG_BPF_STREAM_PARSER */
 
 #if defined(CONFIG_INET) && defined(CONFIG_BPF_SYSCALL)
diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h
index 08674cd14d5a..1e9ed840b9fc 100644
--- a/include/linux/skmsg.h
+++ b/include/linux/skmsg.h
@@ -430,6 +430,19 @@ static inline void psock_set_prog(struct bpf_prog **pprog,
 		bpf_prog_put(prog);
 }
 
+static inline int psock_replace_prog(struct bpf_prog **pprog,
+				     struct bpf_prog *prog,
+				     struct bpf_prog *old)
+{
+	if (cmpxchg(pprog, old, prog) != old)
+		return -ENOENT;
+
+	if (old)
+		bpf_prog_put(old);
+
+	return 0;
+}
+
 static inline void psock_progs_drop(struct sk_psock_progs *progs)
 {
 	psock_set_prog(&progs->msg_parser, NULL);
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index c0ec572f056c..77340045b071 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2893,7 +2893,7 @@ static int bpf_prog_detach(const union bpf_attr *attr)
 	switch (ptype) {
 	case BPF_PROG_TYPE_SK_MSG:
 	case BPF_PROG_TYPE_SK_SKB:
-		return sock_map_get_from_fd(attr, NULL);
+		return sock_map_prog_detach(attr, ptype);
 	case BPF_PROG_TYPE_LIRC_MODE2:
 		return lirc_prog_detach(attr);
 	case BPF_PROG_TYPE_FLOW_DISSECTOR:
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index db45c1453d39..119f52a99dc1 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -77,7 +77,42 @@ int sock_map_get_from_fd(const union bpf_attr *attr, struct bpf_prog *prog)
 	map = __bpf_map_get(f);
 	if (IS_ERR(map))
 		return PTR_ERR(map);
-	ret = sock_map_prog_update(map, prog, attr->attach_type);
+	ret = sock_map_prog_update(map, prog, NULL, attr->attach_type);
+	fdput(f);
+	return ret;
+}
+
+int sock_map_prog_detach(const union bpf_attr *attr, enum bpf_prog_type ptype)
+{
+	u32 ufd = attr->target_fd;
+	struct bpf_prog *prog;
+	struct bpf_map *map;
+	struct fd f;
+	int ret;
+
+	if (attr->attach_flags || attr->replace_bpf_fd)
+		return -EINVAL;
+
+	f = fdget(ufd);
+	map = __bpf_map_get(f);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	prog = bpf_prog_get(attr->attach_bpf_fd);
+	if (IS_ERR(prog)) {
+		ret = PTR_ERR(prog);
+		goto put_map;
+	}
+
+	if (prog->type != ptype) {
+		ret = -EINVAL;
+		goto put_prog;
+	}
+
+	ret = sock_map_prog_update(map, NULL, prog, attr->attach_type);
+put_prog:
+	bpf_prog_put(prog);
+put_map:
 	fdput(f);
 	return ret;
 }
@@ -1212,27 +1247,32 @@ static struct sk_psock_progs *sock_map_progs(struct bpf_map *map)
 }
 
 int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
-			 u32 which)
+			 struct bpf_prog *old, u32 which)
 {
 	struct sk_psock_progs *progs = sock_map_progs(map);
+	struct bpf_prog **pprog;
 
 	if (!progs)
 		return -EOPNOTSUPP;
 
 	switch (which) {
 	case BPF_SK_MSG_VERDICT:
-		psock_set_prog(&progs->msg_parser, prog);
+		pprog = &progs->msg_parser;
 		break;
 	case BPF_SK_SKB_STREAM_PARSER:
-		psock_set_prog(&progs->skb_parser, prog);
+		pprog = &progs->skb_parser;
 		break;
 	case BPF_SK_SKB_STREAM_VERDICT:
-		psock_set_prog(&progs->skb_verdict, prog);
+		pprog = &progs->skb_verdict;
 		break;
 	default:
 		return -EOPNOTSUPP;
 	}
 
+	if (old)
+		return psock_replace_prog(pprog, prog, old);
+
+	psock_set_prog(pprog, prog);
 	return 0;
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH bpf v2 5/6] selftests: bpf: pass program and target_fd in flow_dissector_reattach
  2020-06-29  9:56 [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Lorenz Bauer
                   ` (3 preceding siblings ...)
  2020-06-29  9:56 ` [PATCH bpf v2 4/6] bpf: sockmap: require attach_bpf_fd when detaching a program Lorenz Bauer
@ 2020-06-29  9:56 ` Lorenz Bauer
  2020-06-29  9:56 ` [PATCH bpf v2 6/6] selftests: bpf: pass program to bpf_prog_detach in flow_dissector Lorenz Bauer
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 20+ messages in thread
From: Lorenz Bauer @ 2020-06-29  9:56 UTC (permalink / raw)
  To: ast, daniel, sdf, jakub, john.fastabend; +Cc: kernel-team, bpf, Lorenz Bauer

Pass 0 as target_fd when attaching and detaching flow dissector.
Additionally, pass the expected program when detaching.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 1f043f87bb59 ("selftests/bpf: Add tests for attaching bpf_link to netns")
---
 .../bpf/prog_tests/flow_dissector_reattach.c         | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
index 15cb554a66d8..d70adbc7309a 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
@@ -116,7 +116,7 @@ static void test_prog_attach_prog_attach(int netns, int prog1, int prog2)
 	CHECK_FAIL(query_attached_prog_id(netns) != query_prog_id(prog2));
 
 out_detach:
-	err = bpf_prog_detach(0, BPF_FLOW_DISSECTOR);
+	err = bpf_prog_detach2(prog2, 0, BPF_FLOW_DISSECTOR);
 	if (CHECK_FAIL(err))
 		perror("bpf_prog_detach");
 	CHECK_FAIL(prog_is_attached(netns));
@@ -152,7 +152,7 @@ static void test_prog_attach_link_create(int netns, int prog1, int prog2)
 	DECLARE_LIBBPF_OPTS(bpf_link_create_opts, opts);
 	int err, link;
 
-	err = bpf_prog_attach(prog1, -1, BPF_FLOW_DISSECTOR, 0);
+	err = bpf_prog_attach(prog1, 0, BPF_FLOW_DISSECTOR, 0);
 	if (CHECK_FAIL(err)) {
 		perror("bpf_prog_attach(prog1)");
 		return;
@@ -168,7 +168,7 @@ static void test_prog_attach_link_create(int netns, int prog1, int prog2)
 		close(link);
 	CHECK_FAIL(query_attached_prog_id(netns) != query_prog_id(prog1));
 
-	err = bpf_prog_detach(-1, BPF_FLOW_DISSECTOR);
+	err = bpf_prog_detach2(prog1, 0, BPF_FLOW_DISSECTOR);
 	if (CHECK_FAIL(err))
 		perror("bpf_prog_detach");
 	CHECK_FAIL(prog_is_attached(netns));
@@ -188,7 +188,7 @@ static void test_link_create_prog_attach(int netns, int prog1, int prog2)
 
 	/* Expect failure attaching prog when link exists */
 	errno = 0;
-	err = bpf_prog_attach(prog2, -1, BPF_FLOW_DISSECTOR, 0);
+	err = bpf_prog_attach(prog2, 0, BPF_FLOW_DISSECTOR, 0);
 	if (CHECK_FAIL(!err || errno != EEXIST))
 		perror("bpf_prog_attach(prog2) expected EEXIST");
 	CHECK_FAIL(query_attached_prog_id(netns) != query_prog_id(prog1));
@@ -211,7 +211,7 @@ static void test_link_create_prog_detach(int netns, int prog1, int prog2)
 
 	/* Expect failure detaching prog when link exists */
 	errno = 0;
-	err = bpf_prog_detach(-1, BPF_FLOW_DISSECTOR);
+	err = bpf_prog_detach2(prog1, 0, BPF_FLOW_DISSECTOR);
 	if (CHECK_FAIL(!err || errno != EINVAL))
 		perror("bpf_prog_detach expected EINVAL");
 	CHECK_FAIL(query_attached_prog_id(netns) != query_prog_id(prog1));
@@ -231,7 +231,7 @@ static void test_prog_attach_detach_query(int netns, int prog1, int prog2)
 	}
 	CHECK_FAIL(query_attached_prog_id(netns) != query_prog_id(prog1));
 
-	err = bpf_prog_detach(0, BPF_FLOW_DISSECTOR);
+	err = bpf_prog_detach2(prog1, 0, BPF_FLOW_DISSECTOR);
 	if (CHECK_FAIL(err)) {
 		perror("bpf_prog_detach");
 		return;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH bpf v2 6/6] selftests: bpf: pass program to bpf_prog_detach in flow_dissector
  2020-06-29  9:56 [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Lorenz Bauer
                   ` (4 preceding siblings ...)
  2020-06-29  9:56 ` [PATCH bpf v2 5/6] selftests: bpf: pass program and target_fd in flow_dissector_reattach Lorenz Bauer
@ 2020-06-29  9:56 ` Lorenz Bauer
  2020-06-30  5:48 ` [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Yonghong Song
  2020-06-30 18:00 ` Alexei Starovoitov
  7 siblings, 0 replies; 20+ messages in thread
From: Lorenz Bauer @ 2020-06-29  9:56 UTC (permalink / raw)
  To: ast, daniel, sdf, jakub, john.fastabend; +Cc: kernel-team, bpf, Lorenz Bauer

Calling bpf_prog_detach is incorrect, since it takes target_fd as
its argument. The intention here is to pass it as attach_bpf_fd,
so use bpf_prog_detach2 and pass zero for target_fd.

Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
Fixes: 06716e04a043 ("selftests/bpf: Extend test_flow_dissector to cover link creation")
---
 tools/testing/selftests/bpf/prog_tests/flow_dissector.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
index ea14e3ece812..f11f187990e9 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c
@@ -527,8 +527,8 @@ static void test_skb_less_prog_attach(struct bpf_flow *skel, int tap_fd)
 
 	run_tests_skb_less(tap_fd, skel->maps.last_dissection);
 
-	err = bpf_prog_detach(prog_fd, BPF_FLOW_DISSECTOR);
-	CHECK(err, "bpf_prog_detach", "err %d errno %d\n", err, errno);
+	err = bpf_prog_detach2(prog_fd, 0, BPF_FLOW_DISSECTOR);
+	CHECK(err, "bpf_prog_detach2", "err %d errno %d\n", err, errno);
 }
 
 static void test_skb_less_link_create(struct bpf_flow *skel, int tap_fd)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector
  2020-06-29  9:56 [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Lorenz Bauer
                   ` (5 preceding siblings ...)
  2020-06-29  9:56 ` [PATCH bpf v2 6/6] selftests: bpf: pass program to bpf_prog_detach in flow_dissector Lorenz Bauer
@ 2020-06-30  5:48 ` Yonghong Song
  2020-06-30  8:39   ` Lorenz Bauer
  2020-06-30 18:00 ` Alexei Starovoitov
  7 siblings, 1 reply; 20+ messages in thread
From: Yonghong Song @ 2020-06-30  5:48 UTC (permalink / raw)
  To: Lorenz Bauer, ast, daniel, sdf, jakub, john.fastabend; +Cc: kernel-team, bpf



On 6/29/20 2:56 AM, Lorenz Bauer wrote:
> Both sockmap and flow_dissector ingnore various arguments passed to
> BPF_PROG_ATTACH and BPF_PROG_DETACH. We can fix the attach case by
> checking that the unused arguments are zero. I considered requiring
> target_fd to be -1 instead of 0, but this leads to a lot of churn
> in selftests. There is also precedent in that bpf_iter already
> expects 0 for a similar field. I think that we can come up with a

Since bpf_iter is mentioned here, I would like to provide a little
context on how target_fd in link_create is treated there.

Currently, target_fd is always 0 as it is not used. This is
just easier if we want to use it in the future.

In the future, bpf_iter can maintain that target_fd must be 0
or it may not so. For example, it can add a flag value in
link_create such that when flag is set it will take whatever
value in target_fd and use it. Or it may just take a non-0
target_fd as an indication of the flag is set. I have not
finalized patches yet. I intend to do the latter, i.e.,
taking a non-0 target_fd. But we will see once my bpf_iter
patches for map elements are out.

There is another example where 0 and non-0 prog_fd make a difference.
The attach_prog_fd field when doing prog_load.
When attach_prog_fd is 0, it means attaching to vmlinux through
attach_btf_id. If attach_prog_fd is not 0, it means attaching to
another bpf program (replace). So user space (libbpf) may
already need to pay attention to this.

> work around for fd 0 should we need to in the future.
> 
> The detach case is more problematic: both cgroups and lirc2 verify
> that attach_bpf_fd matches the currently attached program. This
> way you need access to the program fd to be able to remove it.
> Neither sockmap nor flow_dissector do this. flow_dissector even
> has a check for CAP_NET_ADMIN because of this. The patch set
> addresses this by implementing the desired behaviour.
> 
> There is a possibility for user space breakage: any callers that
> don't provide the correct fd will fail with ENOENT. For sockmap
> the risk is low: even the selftests assume that sockmap works
> the way I described. For flow_dissector the story is less
> straightforward, and the selftests use a variety of arguments.
> 
> I've includes fixes tags for the oldest commits that allow an easy
> backport, however the behaviour dates back to when sockmap and
> flow_dissector were introduced. What is the best way to handle these?
> 
> This set is based on top of Jakub's work "bpf, netns: Prepare
> for multi-prog attachment" available at
> https://lore.kernel.org/bpf/87k0zwmhtb.fsf@cloudflare.com/T/
> 
> Since v1:
> - Adjust selftests
> - Implement detach behaviour
> 
> Lorenz Bauer (6):
>    bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
>    bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH
>    bpf: sockmap: check value of unused args to BPF_PROG_ATTACH
>    bpf: sockmap: require attach_bpf_fd when detaching a program
>    selftests: bpf: pass program and target_fd in flow_dissector_reattach
>    selftests: bpf: pass program to bpf_prog_detach in flow_dissector
> 
>   include/linux/bpf-netns.h                     |  5 +-
>   include/linux/bpf.h                           | 13 ++++-
>   include/linux/skmsg.h                         | 13 +++++
>   kernel/bpf/net_namespace.c                    | 22 ++++++--
>   kernel/bpf/syscall.c                          |  6 +--
>   net/core/sock_map.c                           | 53 +++++++++++++++++--
>   .../selftests/bpf/prog_tests/flow_dissector.c |  4 +-
>   .../bpf/prog_tests/flow_dissector_reattach.c  | 12 ++---
>   8 files changed, 103 insertions(+), 25 deletions(-)
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector
  2020-06-30  5:48 ` [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Yonghong Song
@ 2020-06-30  8:39   ` Lorenz Bauer
  2020-06-30 15:08     ` Yonghong Song
  0 siblings, 1 reply; 20+ messages in thread
From: Lorenz Bauer @ 2020-06-30  8:39 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev,
	Jakub Sitnicki, John Fastabend, kernel-team, bpf

On Tue, 30 Jun 2020 at 06:48, Yonghong Song <yhs@fb.com> wrote:
>
> Since bpf_iter is mentioned here, I would like to provide a little
> context on how target_fd in link_create is treated there.

Thanks!

> Currently, target_fd is always 0 as it is not used. This is
> just easier if we want to use it in the future.
>
> In the future, bpf_iter can maintain that target_fd must be 0
> or it may not so. For example, it can add a flag value in
> link_create such that when flag is set it will take whatever
> value in target_fd and use it. Or it may just take a non-0
> target_fd as an indication of the flag is set. I have not
> finalized patches yet. I intend to do the latter, i.e.,
> taking a non-0 target_fd. But we will see once my bpf_iter
> patches for map elements are out.

I had a piece of code for sockmap which did something like this:

    prog = bpf_prog_get(attr->attach_bpf_fd)
    if (IS_ERR(prog))
        if (!attr->attach_bpf_fd)
            // fall back to old behaviour
        else
            return PTR_ERR(prog)
    else if (prog->type != TYPE)
        return -EINVAL

The benefit is that it continues to work if a binary is invoked with
stdin closed, which could lead to a BPF program with fd 0.

Could this work for bpf_iter as well?

>
> There is another example where 0 and non-0 prog_fd make a difference.
> The attach_prog_fd field when doing prog_load.
> When attach_prog_fd is 0, it means attaching to vmlinux through
> attach_btf_id. If attach_prog_fd is not 0, it means attaching to
> another bpf program (replace). So user space (libbpf) may
> already need to pay attention to this.

That is unfortunate. What was the reason to use 0 instead of -1 to
attach to vmlinux?

>
> > work around for fd 0 should we need to in the future.
> >
> > The detach case is more problematic: both cgroups and lirc2 verify
> > that attach_bpf_fd matches the currently attached program. This
> > way you need access to the program fd to be able to remove it.
> > Neither sockmap nor flow_dissector do this. flow_dissector even
> > has a check for CAP_NET_ADMIN because of this. The patch set
> > addresses this by implementing the desired behaviour.
> >
> > There is a possibility for user space breakage: any callers that
> > don't provide the correct fd will fail with ENOENT. For sockmap
> > the risk is low: even the selftests assume that sockmap works
> > the way I described. For flow_dissector the story is less
> > straightforward, and the selftests use a variety of arguments.
> >
> > I've includes fixes tags for the oldest commits that allow an easy
> > backport, however the behaviour dates back to when sockmap and
> > flow_dissector were introduced. What is the best way to handle these?
> >
> > This set is based on top of Jakub's work "bpf, netns: Prepare
> > for multi-prog attachment" available at
> > https://lore.kernel.org/bpf/87k0zwmhtb.fsf@cloudflare.com/T/
> >
> > Since v1:
> > - Adjust selftests
> > - Implement detach behaviour
> >
> > Lorenz Bauer (6):
> >    bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
> >    bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH
> >    bpf: sockmap: check value of unused args to BPF_PROG_ATTACH
> >    bpf: sockmap: require attach_bpf_fd when detaching a program
> >    selftests: bpf: pass program and target_fd in flow_dissector_reattach
> >    selftests: bpf: pass program to bpf_prog_detach in flow_dissector
> >
> >   include/linux/bpf-netns.h                     |  5 +-
> >   include/linux/bpf.h                           | 13 ++++-
> >   include/linux/skmsg.h                         | 13 +++++
> >   kernel/bpf/net_namespace.c                    | 22 ++++++--
> >   kernel/bpf/syscall.c                          |  6 +--
> >   net/core/sock_map.c                           | 53 +++++++++++++++++--
> >   .../selftests/bpf/prog_tests/flow_dissector.c |  4 +-
> >   .../bpf/prog_tests/flow_dissector_reattach.c  | 12 ++---
> >   8 files changed, 103 insertions(+), 25 deletions(-)
> >



-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector
  2020-06-30  8:39   ` Lorenz Bauer
@ 2020-06-30 15:08     ` Yonghong Song
  2020-06-30 15:50       ` Lorenz Bauer
  0 siblings, 1 reply; 20+ messages in thread
From: Yonghong Song @ 2020-06-30 15:08 UTC (permalink / raw)
  To: Lorenz Bauer
  Cc: Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev,
	Jakub Sitnicki, John Fastabend, kernel-team, bpf



On 6/30/20 1:39 AM, Lorenz Bauer wrote:
> On Tue, 30 Jun 2020 at 06:48, Yonghong Song <yhs@fb.com> wrote:
>>
>> Since bpf_iter is mentioned here, I would like to provide a little
>> context on how target_fd in link_create is treated there.
> 
> Thanks!
> 
>> Currently, target_fd is always 0 as it is not used. This is
>> just easier if we want to use it in the future.
>>
>> In the future, bpf_iter can maintain that target_fd must be 0
>> or it may not so. For example, it can add a flag value in
>> link_create such that when flag is set it will take whatever
>> value in target_fd and use it. Or it may just take a non-0
>> target_fd as an indication of the flag is set. I have not
>> finalized patches yet. I intend to do the latter, i.e.,
>> taking a non-0 target_fd. But we will see once my bpf_iter
>> patches for map elements are out.
> 
> I had a piece of code for sockmap which did something like this:
> 
>      prog = bpf_prog_get(attr->attach_bpf_fd)
>      if (IS_ERR(prog))
>          if (!attr->attach_bpf_fd)
>              // fall back to old behaviour
>          else
>              return PTR_ERR(prog)
>      else if (prog->type != TYPE)
>          return -EINVAL
> 
> The benefit is that it continues to work if a binary is invoked with
> stdin closed, which could lead to a BPF program with fd 0.

For bpf_iter, there is no legacy. So I will have something like
     // somecondition could be new attr->flags, or some kernel internal 
checking
     if (somecondition) {
       /* not accepting fd 0 */
       if (attr->attach_bpf_fd == 0)
         return -EINVAL;
       prog = bpf_prog_get(attr->attach_bpf_fd)
       if (IS_ERR(prog))
         return PTR_ERR(prog)
     } else if (attr->attach_bpf_fd != 0)
       return -EINVAL;
or I could have
     if (somecondition) {
       /* accepting any fd */
       prog = bpf_prog_get(attr->attach_bpf_fd)
       if (IS_ERR(prog))
         return PTR_ERR(prog)
     } else if (attr->attach_bpf_fd != 0)
       return -EINVAL;

This "somecondition" is false for the current bpf_iter, so existing
behavior attr->attach_bpf_fd == 0 is still enforced.

> 
> Could this work for bpf_iter as well?
> 
>>
>> There is another example where 0 and non-0 prog_fd make a difference.
>> The attach_prog_fd field when doing prog_load.
>> When attach_prog_fd is 0, it means attaching to vmlinux through
>> attach_btf_id. If attach_prog_fd is not 0, it means attaching to
>> another bpf program (replace). So user space (libbpf) may
>> already need to pay attention to this.
> 
> That is unfortunate. What was the reason to use 0 instead of -1 to
> attach to vmlinux?

attaching to vmlinux happens first and at that time attach_prog_fd
does not exist. Later when replace prog feature is introduced,
attach_prog_fd is added. This field is used to differentiate
between vmlinux func attachment vs. bpf_prog attachment. A little
bit unfortunate, but using 0 is easier as we have check_attr
in the kernel to ensure all kernel-unsupported fields must be 0.
using -1 will break that.

> 
>>
>>> work around for fd 0 should we need to in the future.
>>>
>>> The detach case is more problematic: both cgroups and lirc2 verify
>>> that attach_bpf_fd matches the currently attached program. This
>>> way you need access to the program fd to be able to remove it.
>>> Neither sockmap nor flow_dissector do this. flow_dissector even
>>> has a check for CAP_NET_ADMIN because of this. The patch set
>>> addresses this by implementing the desired behaviour.
>>>
>>> There is a possibility for user space breakage: any callers that
>>> don't provide the correct fd will fail with ENOENT. For sockmap
>>> the risk is low: even the selftests assume that sockmap works
>>> the way I described. For flow_dissector the story is less
>>> straightforward, and the selftests use a variety of arguments.
>>>
>>> I've includes fixes tags for the oldest commits that allow an easy
>>> backport, however the behaviour dates back to when sockmap and
>>> flow_dissector were introduced. What is the best way to handle these?
>>>
>>> This set is based on top of Jakub's work "bpf, netns: Prepare
>>> for multi-prog attachment" available at
>>> https://lore.kernel.org/bpf/87k0zwmhtb.fsf@cloudflare.com/T/
>>>
>>> Since v1:
>>> - Adjust selftests
>>> - Implement detach behaviour
>>>
>>> Lorenz Bauer (6):
>>>     bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
>>>     bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH
>>>     bpf: sockmap: check value of unused args to BPF_PROG_ATTACH
>>>     bpf: sockmap: require attach_bpf_fd when detaching a program
>>>     selftests: bpf: pass program and target_fd in flow_dissector_reattach
>>>     selftests: bpf: pass program to bpf_prog_detach in flow_dissector
>>>
>>>    include/linux/bpf-netns.h                     |  5 +-
>>>    include/linux/bpf.h                           | 13 ++++-
>>>    include/linux/skmsg.h                         | 13 +++++
>>>    kernel/bpf/net_namespace.c                    | 22 ++++++--
>>>    kernel/bpf/syscall.c                          |  6 +--
>>>    net/core/sock_map.c                           | 53 +++++++++++++++++--
>>>    .../selftests/bpf/prog_tests/flow_dissector.c |  4 +-
>>>    .../bpf/prog_tests/flow_dissector_reattach.c  | 12 ++---
>>>    8 files changed, 103 insertions(+), 25 deletions(-)
>>>
> 
> 
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector
  2020-06-30 15:08     ` Yonghong Song
@ 2020-06-30 15:50       ` Lorenz Bauer
  0 siblings, 0 replies; 20+ messages in thread
From: Lorenz Bauer @ 2020-06-30 15:50 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev,
	Jakub Sitnicki, John Fastabend, kernel-team, bpf

On Tue, 30 Jun 2020 at 16:08, Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 6/30/20 1:39 AM, Lorenz Bauer wrote:
> > On Tue, 30 Jun 2020 at 06:48, Yonghong Song <yhs@fb.com> wrote:
> >>
> >> Since bpf_iter is mentioned here, I would like to provide a little
> >> context on how target_fd in link_create is treated there.
> >
> > Thanks!
> >
> >> Currently, target_fd is always 0 as it is not used. This is
> >> just easier if we want to use it in the future.
> >>
> >> In the future, bpf_iter can maintain that target_fd must be 0
> >> or it may not so. For example, it can add a flag value in
> >> link_create such that when flag is set it will take whatever
> >> value in target_fd and use it. Or it may just take a non-0
> >> target_fd as an indication of the flag is set. I have not
> >> finalized patches yet. I intend to do the latter, i.e.,
> >> taking a non-0 target_fd. But we will see once my bpf_iter
> >> patches for map elements are out.
> >
> > I had a piece of code for sockmap which did something like this:
> >
> >      prog = bpf_prog_get(attr->attach_bpf_fd)
> >      if (IS_ERR(prog))
> >          if (!attr->attach_bpf_fd)
> >              // fall back to old behaviour
> >          else
> >              return PTR_ERR(prog)
> >      else if (prog->type != TYPE)
> >          return -EINVAL
> >
> > The benefit is that it continues to work if a binary is invoked with
> > stdin closed, which could lead to a BPF program with fd 0.
>
> For bpf_iter, there is no legacy. So I will have something like
>      // somecondition could be new attr->flags, or some kernel internal
> checking
>      if (somecondition) {
>        /* not accepting fd 0 */
>        if (attr->attach_bpf_fd == 0)
>          return -EINVAL;
>        prog = bpf_prog_get(attr->attach_bpf_fd)
>        if (IS_ERR(prog))
>          return PTR_ERR(prog)
>      } else if (attr->attach_bpf_fd != 0)
>        return -EINVAL;
> or I could have
>      if (somecondition) {
>        /* accepting any fd */
>        prog = bpf_prog_get(attr->attach_bpf_fd)
>        if (IS_ERR(prog))
>          return PTR_ERR(prog)
>      } else if (attr->attach_bpf_fd != 0)
>        return -EINVAL;
>
> This "somecondition" is false for the current bpf_iter, so existing
> behavior attr->attach_bpf_fd == 0 is still enforced.
>
> >
> > Could this work for bpf_iter as well?
> >
> >>
> >> There is another example where 0 and non-0 prog_fd make a difference.
> >> The attach_prog_fd field when doing prog_load.
> >> When attach_prog_fd is 0, it means attaching to vmlinux through
> >> attach_btf_id. If attach_prog_fd is not 0, it means attaching to
> >> another bpf program (replace). So user space (libbpf) may
> >> already need to pay attention to this.
> >
> > That is unfortunate. What was the reason to use 0 instead of -1 to
> > attach to vmlinux?
>
> attaching to vmlinux happens first and at that time attach_prog_fd
> does not exist. Later when replace prog feature is introduced,
> attach_prog_fd is added. This field is used to differentiate
> between vmlinux func attachment vs. bpf_prog attachment. A little
> bit unfortunate, but using 0 is easier as we have check_attr
> in the kernel to ensure all kernel-unsupported fields must be 0.
> using -1 will break that.

Ah, that makes sense, thank you!

Best
Lorenz

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector
  2020-06-29  9:56 [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Lorenz Bauer
                   ` (6 preceding siblings ...)
  2020-06-30  5:48 ` [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Yonghong Song
@ 2020-06-30 18:00 ` Alexei Starovoitov
  2020-06-30 18:31   ` Jakub Sitnicki
  7 siblings, 1 reply; 20+ messages in thread
From: Alexei Starovoitov @ 2020-06-30 18:00 UTC (permalink / raw)
  To: Lorenz Bauer
  Cc: Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev,
	Jakub Sitnicki, John Fastabend, kernel-team, bpf

On Mon, Jun 29, 2020 at 2:59 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>
> Both sockmap and flow_dissector ingnore various arguments passed to
> BPF_PROG_ATTACH and BPF_PROG_DETACH. We can fix the attach case by
> checking that the unused arguments are zero. I considered requiring
> target_fd to be -1 instead of 0, but this leads to a lot of churn
> in selftests. There is also precedent in that bpf_iter already
> expects 0 for a similar field. I think that we can come up with a
> work around for fd 0 should we need to in the future.
>
> The detach case is more problematic: both cgroups and lirc2 verify
> that attach_bpf_fd matches the currently attached program. This
> way you need access to the program fd to be able to remove it.
> Neither sockmap nor flow_dissector do this. flow_dissector even
> has a check for CAP_NET_ADMIN because of this. The patch set
> addresses this by implementing the desired behaviour.
>
> There is a possibility for user space breakage: any callers that
> don't provide the correct fd will fail with ENOENT. For sockmap
> the risk is low: even the selftests assume that sockmap works
> the way I described. For flow_dissector the story is less
> straightforward, and the selftests use a variety of arguments.
>
> I've includes fixes tags for the oldest commits that allow an easy
> backport, however the behaviour dates back to when sockmap and
> flow_dissector were introduced. What is the best way to handle these?
>
> This set is based on top of Jakub's work "bpf, netns: Prepare
> for multi-prog attachment" available at
> https://lore.kernel.org/bpf/87k0zwmhtb.fsf@cloudflare.com/T/

Folks, you should have used 'bpf' in the subject of Jakub's patches.
I've dropped Jakub's set from bpf-next and re-applied to bpf tree.
And applied this set on top.
Thanks!

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector
  2020-06-30 18:00 ` Alexei Starovoitov
@ 2020-06-30 18:31   ` Jakub Sitnicki
  2020-06-30 18:42     ` Alexei Starovoitov
  0 siblings, 1 reply; 20+ messages in thread
From: Jakub Sitnicki @ 2020-06-30 18:31 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Lorenz Bauer, Alexei Starovoitov, Daniel Borkmann,
	Stanislav Fomichev, John Fastabend, kernel-team, bpf

On Tue, Jun 30, 2020 at 08:00 PM CEST, Alexei Starovoitov wrote:
> On Mon, Jun 29, 2020 at 2:59 AM Lorenz Bauer <lmb@cloudflare.com> wrote:
>>
>> Both sockmap and flow_dissector ingnore various arguments passed to
>> BPF_PROG_ATTACH and BPF_PROG_DETACH. We can fix the attach case by
>> checking that the unused arguments are zero. I considered requiring
>> target_fd to be -1 instead of 0, but this leads to a lot of churn
>> in selftests. There is also precedent in that bpf_iter already
>> expects 0 for a similar field. I think that we can come up with a
>> work around for fd 0 should we need to in the future.
>>
>> The detach case is more problematic: both cgroups and lirc2 verify
>> that attach_bpf_fd matches the currently attached program. This
>> way you need access to the program fd to be able to remove it.
>> Neither sockmap nor flow_dissector do this. flow_dissector even
>> has a check for CAP_NET_ADMIN because of this. The patch set
>> addresses this by implementing the desired behaviour.
>>
>> There is a possibility for user space breakage: any callers that
>> don't provide the correct fd will fail with ENOENT. For sockmap
>> the risk is low: even the selftests assume that sockmap works
>> the way I described. For flow_dissector the story is less
>> straightforward, and the selftests use a variety of arguments.
>>
>> I've includes fixes tags for the oldest commits that allow an easy
>> backport, however the behaviour dates back to when sockmap and
>> flow_dissector were introduced. What is the best way to handle these?
>>
>> This set is based on top of Jakub's work "bpf, netns: Prepare
>> for multi-prog attachment" available at
>> https://lore.kernel.org/bpf/87k0zwmhtb.fsf@cloudflare.com/T/
>
> Folks, you should have used 'bpf' in the subject of Jakub's patches.
> I've dropped Jakub's set from bpf-next and re-applied to bpf tree.
> And applied this set on top.
> Thanks!

The preparatory work for multi-prog for netns programs [0] that landed
recently in bpf-next wasn't fixing anything, so I thought it was -next
material.

I've messed up because I've asked Lorenz to base his patchset on top of
mine, but didn't consider the fact that Lorenz's fixes are targeted for
v5.8 and earlier :-/

So alternatively, we could respin Lorenz patchset on top of 'bpf', if
you want, to untangle this situation.

But if you decide to keep my patchset in 'bpf', then can you please also
apply the today's fixup [1]? Or I can resend with correct subject prefix
tomorrow.

Thanks,
-jkbs

[0] https://lore.kernel.org/bpf/20200625141357.910330-1-jakub@cloudflare.com/
[1] https://lore.kernel.org/bpf/20200630164541.1329993-1-jakub@cloudflare.com/

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector
  2020-06-30 18:31   ` Jakub Sitnicki
@ 2020-06-30 18:42     ` Alexei Starovoitov
  2020-07-01  7:45       ` Jakub Sitnicki
  0 siblings, 1 reply; 20+ messages in thread
From: Alexei Starovoitov @ 2020-06-30 18:42 UTC (permalink / raw)
  To: Jakub Sitnicki
  Cc: Lorenz Bauer, Alexei Starovoitov, Daniel Borkmann,
	Stanislav Fomichev, John Fastabend, kernel-team, bpf

On Tue, Jun 30, 2020 at 11:31 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> But if you decide to keep my patchset in 'bpf', then can you please also
> apply the today's fixup [1]? Or I can resend with correct subject prefix
> tomorrow.

Already did.
I found it in patchworks, but not in my mailbox.
Please cc myself and Daniel in the future.
vger can be unreliable some times.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector
  2020-06-30 18:42     ` Alexei Starovoitov
@ 2020-07-01  7:45       ` Jakub Sitnicki
  0 siblings, 0 replies; 20+ messages in thread
From: Jakub Sitnicki @ 2020-07-01  7:45 UTC (permalink / raw)
  To: Alexei Starovoitov
  Cc: Lorenz Bauer, Alexei Starovoitov, Daniel Borkmann,
	Stanislav Fomichev, John Fastabend, kernel-team, bpf

On Tue, Jun 30, 2020 at 08:42 PM CEST, Alexei Starovoitov wrote:
> On Tue, Jun 30, 2020 at 11:31 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>>
>> But if you decide to keep my patchset in 'bpf', then can you please also
>> apply the today's fixup [1]? Or I can resend with correct subject prefix
>> tomorrow.
>
> Already did.
> I found it in patchworks, but not in my mailbox.
> Please cc myself and Daniel in the future.
> vger can be unreliable some times.

Noted. Will do from now on.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 4/6] bpf: sockmap: require attach_bpf_fd when detaching a program
  2020-06-29  9:56 ` [PATCH bpf v2 4/6] bpf: sockmap: require attach_bpf_fd when detaching a program Lorenz Bauer
@ 2020-07-08  1:30   ` Martin KaFai Lau
  0 siblings, 0 replies; 20+ messages in thread
From: Martin KaFai Lau @ 2020-07-08  1:30 UTC (permalink / raw)
  To: Lorenz Bauer; +Cc: ast, daniel, sdf, jakub, john.fastabend, kernel-team, bpf

On Mon, Jun 29, 2020 at 10:56:28AM +0100, Lorenz Bauer wrote:
> The sockmap code currently ignores the value of attach_bpf_fd when
> detaching a program. This is contrary to the usual behaviour of
> checking that attach_bpf_fd represents the currently attached
> program.
> 
> Ensure that attach_bpf_fd is indeed the currently attached
> program. It turns out that all sockmap selftests already do this,
> which indicates that this is unlikely to cause breakage.
Please update the tools/testing/selftests/bpf/test_maps.c.
It is currently broken:

#> ./test_maps
Failed empty parser prog detach

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
  2020-06-29  9:56 ` [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH Lorenz Bauer
@ 2020-11-04 18:08   ` Jiri Benc
  2020-11-05 11:00     ` Lorenz Bauer
  0 siblings, 1 reply; 20+ messages in thread
From: Jiri Benc @ 2020-11-04 18:08 UTC (permalink / raw)
  To: Lorenz Bauer; +Cc: ast, daniel, sdf, jakub, john.fastabend, kernel-team, bpf

On Mon, 29 Jun 2020 10:56:25 +0100, Lorenz Bauer wrote:
> Using BPF_PROG_ATTACH on a flow dissector program supports neither
> target_fd, attach_flags or replace_bpf_fd but accepts any value.
> 
> Enforce that all of them are zero. This is fine for replace_bpf_fd
> since its presence is indicated by BPF_F_REPLACE. It's more
> problematic for target_fd, since zero is a valid fd. Should we
> want to use the flag later on we'd have to add an exception for
> fd 0. The alternative is to force a value like -1. This requires
> more changes to tests. There is also precedent for using 0,
> since bpf_iter uses this for target_fd as well.
> 
> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> Fixes: b27f7bb590ba ("flow_dissector: Move out netns_bpf prog callbacks")
> ---
>  kernel/bpf/net_namespace.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
> index 3e89c7ad42cb..bf18eabeaea2 100644
> --- a/kernel/bpf/net_namespace.c
> +++ b/kernel/bpf/net_namespace.c
> @@ -217,6 +217,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>  	struct net *net;
>  	int ret;
>  
> +	if (attr->target_fd || attr->attach_flags || attr->replace_bpf_fd)
> +		return -EINVAL;

I'm debugging failing test_flow_dissector.sh selftest and I wonder how
this patch works.

The test_flow_dissector.sh selftest at line 28 runs:

bpftool prog -d attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector

which invokes this code:

static int parse_attach_detach_args(int argc, char **argv, int *progfd,
                                    enum bpf_attach_type *attach_type,
                                    int *mapfd)
{
	[...]
        if (*attach_type == BPF_FLOW_DISSECTOR) {
                *mapfd = -1;
                return 0;
        }
	[...]
}

The mapfd is later used as attr->target_fd:

static int do_attach(int argc, char **argv)
{
	[...]
        err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
	[...]
}

and rejected in the kernel by the line added by this patch. Seems that
setting flow dissector using bpftool does not work since this patch was
applied? What am I missing?

 Jiri


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
  2020-11-04 18:08   ` Jiri Benc
@ 2020-11-05 11:00     ` Lorenz Bauer
  2020-11-05 11:08       ` Jiri Benc
  0 siblings, 1 reply; 20+ messages in thread
From: Lorenz Bauer @ 2020-11-05 11:00 UTC (permalink / raw)
  To: Jiri Benc
  Cc: Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev,
	Jakub Sitnicki, John Fastabend, kernel-team, bpf

On Wed, 4 Nov 2020 at 18:08, Jiri Benc <jbenc@redhat.com> wrote:
>
> On Mon, 29 Jun 2020 10:56:25 +0100, Lorenz Bauer wrote:
> > Using BPF_PROG_ATTACH on a flow dissector program supports neither
> > target_fd, attach_flags or replace_bpf_fd but accepts any value.
> >
> > Enforce that all of them are zero. This is fine for replace_bpf_fd
> > since its presence is indicated by BPF_F_REPLACE. It's more
> > problematic for target_fd, since zero is a valid fd. Should we
> > want to use the flag later on we'd have to add an exception for
> > fd 0. The alternative is to force a value like -1. This requires
> > more changes to tests. There is also precedent for using 0,
> > since bpf_iter uses this for target_fd as well.
> >
> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com>
> > Fixes: b27f7bb590ba ("flow_dissector: Move out netns_bpf prog callbacks")
> > ---
> >  kernel/bpf/net_namespace.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/bpf/net_namespace.c b/kernel/bpf/net_namespace.c
> > index 3e89c7ad42cb..bf18eabeaea2 100644
> > --- a/kernel/bpf/net_namespace.c
> > +++ b/kernel/bpf/net_namespace.c
> > @@ -217,6 +217,9 @@ int netns_bpf_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> >       struct net *net;
> >       int ret;
> >
> > +     if (attr->target_fd || attr->attach_flags || attr->replace_bpf_fd)
> > +             return -EINVAL;
>
> I'm debugging failing test_flow_dissector.sh selftest and I wonder how
> this patch works.
>
> The test_flow_dissector.sh selftest at line 28 runs:
>
> bpftool prog -d attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector
>
> which invokes this code:
>
> static int parse_attach_detach_args(int argc, char **argv, int *progfd,
>                                     enum bpf_attach_type *attach_type,
>                                     int *mapfd)
> {
>         [...]
>         if (*attach_type == BPF_FLOW_DISSECTOR) {
>                 *mapfd = -1;
>                 return 0;
>         }
>         [...]
> }

Hi Jiri,

Thanks for the bug report. The commit indeed breaks attaching the flow
dissector using bpftool as you point out. I think I didn't catch this
because I don't have bpftool in my path, so that test was actually
skipped... The other parts of the test use a custom loader, which
passes 0 for the arguments and are not affected.

I had a cursory look at bpftool packaging in Debian, Ubuntu and
Fedora, it seems they all package bpftool in a kernel version
dependent package. So the most straightforward fix is probably to
change bpftool to use *mapfd = 0 and then land that via the bpf tree.

What do you think?

Lorenz

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
  2020-11-05 11:00     ` Lorenz Bauer
@ 2020-11-05 11:08       ` Jiri Benc
  2020-11-05 16:04         ` Stanislav Fomichev
  0 siblings, 1 reply; 20+ messages in thread
From: Jiri Benc @ 2020-11-05 11:08 UTC (permalink / raw)
  To: Lorenz Bauer
  Cc: Alexei Starovoitov, Daniel Borkmann, Stanislav Fomichev,
	Jakub Sitnicki, John Fastabend, kernel-team, bpf

On Thu, 5 Nov 2020 11:00:45 +0000, Lorenz Bauer wrote:
> I had a cursory look at bpftool packaging in Debian, Ubuntu and
> Fedora, it seems they all package bpftool in a kernel version
> dependent package. So the most straightforward fix is probably to
> change bpftool to use *mapfd = 0 and then land that via the bpf tree.
> 
> What do you think?

Apparently nobody is using bpftool for this (otherwise someone would
notice), so I'd say go ahead.

Thanks!

 Jiri


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH
  2020-11-05 11:08       ` Jiri Benc
@ 2020-11-05 16:04         ` Stanislav Fomichev
  0 siblings, 0 replies; 20+ messages in thread
From: Stanislav Fomichev @ 2020-11-05 16:04 UTC (permalink / raw)
  To: Jiri Benc
  Cc: Lorenz Bauer, Alexei Starovoitov, Daniel Borkmann,
	Jakub Sitnicki, John Fastabend, kernel-team, bpf

On Thu, Nov 5, 2020 at 3:08 AM Jiri Benc <jbenc@redhat.com> wrote:
>
> On Thu, 5 Nov 2020 11:00:45 +0000, Lorenz Bauer wrote:
> > I had a cursory look at bpftool packaging in Debian, Ubuntu and
> > Fedora, it seems they all package bpftool in a kernel version
> > dependent package. So the most straightforward fix is probably to
> > change bpftool to use *mapfd = 0 and then land that via the bpf tree.
> >
> > What do you think?
>
> Apparently nobody is using bpftool for this (otherwise someone would
> notice), so I'd say go ahead.
Might be a good time to switch selftests to use bpftool to attach the
flow dissector.
I think right now we have a custom binary to do it (mainly because
bpftool wasn't available in the selftests when the flow dissector was
added).

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-11-05 16:04 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29  9:56 [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Lorenz Bauer
2020-06-29  9:56 ` [PATCH bpf v2 1/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_ATTACH Lorenz Bauer
2020-11-04 18:08   ` Jiri Benc
2020-11-05 11:00     ` Lorenz Bauer
2020-11-05 11:08       ` Jiri Benc
2020-11-05 16:04         ` Stanislav Fomichev
2020-06-29  9:56 ` [PATCH bpf v2 2/6] bpf: flow_dissector: check value of unused flags to BPF_PROG_DETACH Lorenz Bauer
2020-06-29  9:56 ` [PATCH bpf v2 3/6] bpf: sockmap: check value of unused args to BPF_PROG_ATTACH Lorenz Bauer
2020-06-29  9:56 ` [PATCH bpf v2 4/6] bpf: sockmap: require attach_bpf_fd when detaching a program Lorenz Bauer
2020-07-08  1:30   ` Martin KaFai Lau
2020-06-29  9:56 ` [PATCH bpf v2 5/6] selftests: bpf: pass program and target_fd in flow_dissector_reattach Lorenz Bauer
2020-06-29  9:56 ` [PATCH bpf v2 6/6] selftests: bpf: pass program to bpf_prog_detach in flow_dissector Lorenz Bauer
2020-06-30  5:48 ` [PATCH bpf v2 0/6] Fix attach / detach uapi for sockmap and flow_dissector Yonghong Song
2020-06-30  8:39   ` Lorenz Bauer
2020-06-30 15:08     ` Yonghong Song
2020-06-30 15:50       ` Lorenz Bauer
2020-06-30 18:00 ` Alexei Starovoitov
2020-06-30 18:31   ` Jakub Sitnicki
2020-06-30 18:42     ` Alexei Starovoitov
2020-07-01  7:45       ` Jakub Sitnicki

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).