linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
@ 2022-01-13  9:00 Di Zhu
  2022-01-13  9:00 ` [PATCH bpf-next v5 2/2] selftests: bpf: test " Di Zhu
  2022-01-13 16:15 ` [PATCH bpf-next v5 1/2] bpf: support " Jakub Sitnicki
  0 siblings, 2 replies; 18+ messages in thread
From: Di Zhu @ 2022-01-13  9:00 UTC (permalink / raw)
  To: ast, davem, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, jakub
  Cc: bpf, linux-kernel, zhudi2, luzhihao, rose.chen

Right now there is no way to query whether BPF programs are
attached to a sockmap or not.

we can use the standard interface in libbpf to query, such as:
bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
the mapFd is the fd of sockmap.

Signed-off-by: Di Zhu <zhudi2@huawei.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
 include/linux/bpf.h  |  9 +++++
 kernel/bpf/syscall.c |  5 +++
 net/core/sock_map.c  | 78 ++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 85 insertions(+), 7 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6e947cd91152..c4ca14c9f838 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2071,6 +2071,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog,
 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);
 int sock_map_update_elem_sys(struct bpf_map *map, void *key, void *value, u64 flags);
+int sock_map_bpf_prog_query(const union bpf_attr *attr,
+			    union bpf_attr __user *uattr);
+
 void sock_map_unhash(struct sock *sk);
 void sock_map_close(struct sock *sk, long timeout);
 #else
@@ -2124,6 +2127,12 @@ static inline int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
 {
 	return -EOPNOTSUPP;
 }
+
+static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
+					  union bpf_attr __user *uattr)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_BPF_SYSCALL */
 #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index fa4505f9b611..9e0631f091a6 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3318,6 +3318,11 @@ static int bpf_prog_query(const union bpf_attr *attr,
 	case BPF_FLOW_DISSECTOR:
 	case BPF_SK_LOOKUP:
 		return netns_bpf_prog_query(attr, uattr);
+	case BPF_SK_SKB_STREAM_PARSER:
+	case BPF_SK_SKB_STREAM_VERDICT:
+	case BPF_SK_MSG_VERDICT:
+	case BPF_SK_SKB_VERDICT:
+		return sock_map_bpf_prog_query(attr, uattr);
 	default:
 		return -EINVAL;
 	}
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 1827669eedd6..a424f51041ca 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1416,38 +1416,50 @@ static struct sk_psock_progs *sock_map_progs(struct bpf_map *map)
 	return NULL;
 }
 
-static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
-				struct bpf_prog *old, u32 which)
+static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
+				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:
-		pprog = &progs->msg_parser;
+		*pprog = &progs->msg_parser;
 		break;
 #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
 	case BPF_SK_SKB_STREAM_PARSER:
-		pprog = &progs->stream_parser;
+		*pprog = &progs->stream_parser;
 		break;
 #endif
 	case BPF_SK_SKB_STREAM_VERDICT:
 		if (progs->skb_verdict)
 			return -EBUSY;
-		pprog = &progs->stream_verdict;
+		*pprog = &progs->stream_verdict;
 		break;
 	case BPF_SK_SKB_VERDICT:
 		if (progs->stream_verdict)
 			return -EBUSY;
-		pprog = &progs->skb_verdict;
+		*pprog = &progs->skb_verdict;
 		break;
 	default:
 		return -EOPNOTSUPP;
 	}
 
+	return 0;
+}
+
+static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
+				struct bpf_prog *old, u32 which)
+{
+	struct bpf_prog **pprog;
+	int ret;
+
+	ret = sock_map_prog_lookup(map, &pprog, which);
+	if (ret)
+		return ret;
+
 	if (old)
 		return psock_replace_prog(pprog, prog, old);
 
@@ -1455,6 +1467,58 @@ static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
 	return 0;
 }
 
+int sock_map_bpf_prog_query(const union bpf_attr *attr,
+			    union bpf_attr __user *uattr)
+{
+	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
+	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
+	struct bpf_prog **pprog;
+	struct bpf_prog *prog;
+	struct bpf_map *map;
+	struct fd f;
+	u32 id = 0;
+	int ret;
+
+	if (attr->query.query_flags)
+		return -EINVAL;
+
+	f = fdget(ufd);
+	map = __bpf_map_get(f);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	rcu_read_lock();
+
+	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
+	if (ret)
+		goto end;
+
+	prog = *pprog;
+	prog_cnt = !prog ? 0 : 1;
+
+	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
+		goto end;
+
+	id = prog->aux->id;
+
+	/* we do not hold the refcnt, the bpf prog may be released
+	 * asynchronously and the id would be set to 0.
+	 */
+	if (id == 0)
+		prog_cnt = 0;
+
+end:
+	rcu_read_unlock();
+
+	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
+	    (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
+	    copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
+		ret = -EFAULT;
+
+	fdput(f);
+	return ret;
+}
+
 static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
 {
 	switch (link->map->map_type) {
-- 
2.27.0


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

* [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
  2022-01-13  9:00 [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap Di Zhu
@ 2022-01-13  9:00 ` Di Zhu
  2022-01-15  1:18   ` Andrii Nakryiko
  2022-01-13 16:15 ` [PATCH bpf-next v5 1/2] bpf: support " Jakub Sitnicki
  1 sibling, 1 reply; 18+ messages in thread
From: Di Zhu @ 2022-01-13  9:00 UTC (permalink / raw)
  To: ast, davem, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, jakub
  Cc: bpf, linux-kernel, zhudi2, luzhihao, rose.chen

Add test for querying progs attached to sockmap. we use an existing
libbpf query interface to query prog cnt before and after progs
attaching to sockmap and check whether the queried prog id is right.

Signed-off-by: Di Zhu <zhudi2@huawei.com>
Acked-by: Yonghong Song <yhs@fb.com>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c  | 70 +++++++++++++++++++
 .../bpf/progs/test_sockmap_progs_query.c      | 24 +++++++
 2 files changed, 94 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 85db0f4cdd95..06923ea44bad 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -8,6 +8,7 @@
 #include "test_sockmap_update.skel.h"
 #include "test_sockmap_invalid_update.skel.h"
 #include "test_sockmap_skb_verdict_attach.skel.h"
+#include "test_sockmap_progs_query.skel.h"
 #include "bpf_iter_sockmap.skel.h"
 
 #define TCP_REPAIR		19	/* TCP sock is under repair right now */
@@ -315,6 +316,69 @@ static void test_sockmap_skb_verdict_attach(enum bpf_attach_type first,
 	test_sockmap_skb_verdict_attach__destroy(skel);
 }
 
+static __u32 query_prog_id(int prog_fd)
+{
+	struct bpf_prog_info info = {};
+	__u32 info_len = sizeof(info);
+	int err;
+
+	err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
+	if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
+	    !ASSERT_EQ(info_len, sizeof(info), "bpf_obj_get_info_by_fd"))
+		return 0;
+
+	return info.id;
+}
+
+static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
+{
+	struct test_sockmap_progs_query *skel;
+	int err, map_fd, verdict_fd, duration = 0;
+	__u32 attach_flags = 0;
+	__u32 prog_ids[3] = {};
+	__u32 prog_cnt = 3;
+
+	skel = test_sockmap_progs_query__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "test_sockmap_progs_query__open_and_load"))
+		return;
+
+	map_fd = bpf_map__fd(skel->maps.sock_map);
+
+	if (attach_type == BPF_SK_MSG_VERDICT)
+		verdict_fd = bpf_program__fd(skel->progs.prog_skmsg_verdict);
+	else
+		verdict_fd = bpf_program__fd(skel->progs.prog_skb_verdict);
+
+	err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
+			     &attach_flags, prog_ids, &prog_cnt);
+	if (!ASSERT_OK(err, "bpf_prog_query failed"))
+		goto out;
+
+	if (!ASSERT_EQ(attach_flags,  0, "wrong attach_flags on query"))
+		goto out;
+
+	if (!ASSERT_EQ(prog_cnt, 0, "wrong program count on query"))
+		goto out;
+
+	err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
+	if (!ASSERT_OK(err, "bpf_prog_attach failed"))
+		goto out;
+
+	prog_cnt = 1;
+	err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
+			     &attach_flags, prog_ids, &prog_cnt);
+
+	ASSERT_OK(err, "bpf_prog_query failed");
+	ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query");
+	ASSERT_EQ(prog_cnt, 1, "wrong program count on query");
+	ASSERT_EQ(prog_ids[0], query_prog_id(verdict_fd),
+		  "wrong prog_ids on query");
+
+	bpf_prog_detach2(verdict_fd, map_fd, attach_type);
+out:
+	test_sockmap_progs_query__destroy(skel);
+}
+
 void test_sockmap_basic(void)
 {
 	if (test__start_subtest("sockmap create_update_free"))
@@ -341,4 +405,10 @@ void test_sockmap_basic(void)
 		test_sockmap_skb_verdict_attach(BPF_SK_SKB_STREAM_VERDICT,
 						BPF_SK_SKB_VERDICT);
 	}
+	if (test__start_subtest("sockmap progs query")) {
+		test_sockmap_progs_query(BPF_SK_MSG_VERDICT);
+		test_sockmap_progs_query(BPF_SK_SKB_STREAM_PARSER);
+		test_sockmap_progs_query(BPF_SK_SKB_STREAM_VERDICT);
+		test_sockmap_progs_query(BPF_SK_SKB_VERDICT);
+	}
 }
diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
new file mode 100644
index 000000000000..9d58d61c0dee
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SOCKMAP);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u64);
+} sock_map SEC(".maps");
+
+SEC("sk_skb")
+int prog_skb_verdict(struct __sk_buff *skb)
+{
+	return SK_PASS;
+}
+
+SEC("sk_msg")
+int prog_skmsg_verdict(struct sk_msg_md *msg)
+{
+	return SK_PASS;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.27.0


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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
  2022-01-13  9:00 [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap Di Zhu
  2022-01-13  9:00 ` [PATCH bpf-next v5 2/2] selftests: bpf: test " Di Zhu
@ 2022-01-13 16:15 ` Jakub Sitnicki
  2022-01-15  1:22   ` Andrii Nakryiko
  1 sibling, 1 reply; 18+ messages in thread
From: Jakub Sitnicki @ 2022-01-13 16:15 UTC (permalink / raw)
  To: Di Zhu
  Cc: ast, davem, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, bpf, linux-kernel, luzhihao, rose.chen

On Thu, Jan 13, 2022 at 10:00 AM CET, Di Zhu wrote:
> Right now there is no way to query whether BPF programs are
> attached to a sockmap or not.
>
> we can use the standard interface in libbpf to query, such as:
> bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
> the mapFd is the fd of sockmap.
>
> Signed-off-by: Di Zhu <zhudi2@huawei.com>
> Acked-by: Yonghong Song <yhs@fb.com>
> ---
>  include/linux/bpf.h  |  9 +++++
>  kernel/bpf/syscall.c |  5 +++
>  net/core/sock_map.c  | 78 ++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 85 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 6e947cd91152..c4ca14c9f838 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2071,6 +2071,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog,
>  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);
>  int sock_map_update_elem_sys(struct bpf_map *map, void *key, void *value, u64 flags);
> +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> +			    union bpf_attr __user *uattr);
> +
>  void sock_map_unhash(struct sock *sk);
>  void sock_map_close(struct sock *sk, long timeout);
>  #else
> @@ -2124,6 +2127,12 @@ static inline int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
>  {
>  	return -EOPNOTSUPP;
>  }
> +
> +static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
> +					  union bpf_attr __user *uattr)
> +{
> +	return -EINVAL;
> +}
>  #endif /* CONFIG_BPF_SYSCALL */
>  #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index fa4505f9b611..9e0631f091a6 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3318,6 +3318,11 @@ static int bpf_prog_query(const union bpf_attr *attr,
>  	case BPF_FLOW_DISSECTOR:
>  	case BPF_SK_LOOKUP:
>  		return netns_bpf_prog_query(attr, uattr);
> +	case BPF_SK_SKB_STREAM_PARSER:
> +	case BPF_SK_SKB_STREAM_VERDICT:
> +	case BPF_SK_MSG_VERDICT:
> +	case BPF_SK_SKB_VERDICT:
> +		return sock_map_bpf_prog_query(attr, uattr);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 1827669eedd6..a424f51041ca 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -1416,38 +1416,50 @@ static struct sk_psock_progs *sock_map_progs(struct bpf_map *map)
>  	return NULL;
>  }
>
> -static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
> -				struct bpf_prog *old, u32 which)
> +static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
> +				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:
> -		pprog = &progs->msg_parser;
> +		*pprog = &progs->msg_parser;
>  		break;
>  #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
>  	case BPF_SK_SKB_STREAM_PARSER:
> -		pprog = &progs->stream_parser;
> +		*pprog = &progs->stream_parser;
>  		break;
>  #endif
>  	case BPF_SK_SKB_STREAM_VERDICT:
>  		if (progs->skb_verdict)
>  			return -EBUSY;
> -		pprog = &progs->stream_verdict;
> +		*pprog = &progs->stream_verdict;
>  		break;
>  	case BPF_SK_SKB_VERDICT:
>  		if (progs->stream_verdict)
>  			return -EBUSY;
> -		pprog = &progs->skb_verdict;
> +		*pprog = &progs->skb_verdict;
>  		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
>
> +	return 0;
> +}
> +
> +static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
> +				struct bpf_prog *old, u32 which)
> +{
> +	struct bpf_prog **pprog;
> +	int ret;
> +
> +	ret = sock_map_prog_lookup(map, &pprog, which);
> +	if (ret)
> +		return ret;
> +
>  	if (old)
>  		return psock_replace_prog(pprog, prog, old);
>
> @@ -1455,6 +1467,58 @@ static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
>  	return 0;
>  }
>
> +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> +			    union bpf_attr __user *uattr)
> +{
> +	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> +	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> +	struct bpf_prog **pprog;
> +	struct bpf_prog *prog;
> +	struct bpf_map *map;
> +	struct fd f;
> +	u32 id = 0;
> +	int ret;
> +
> +	if (attr->query.query_flags)
> +		return -EINVAL;
> +
> +	f = fdget(ufd);
> +	map = __bpf_map_get(f);
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	rcu_read_lock();
> +
> +	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> +	if (ret)
> +		goto end;
> +
> +	prog = *pprog;
> +	prog_cnt = !prog ? 0 : 1;
> +
> +	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> +		goto end;
> +
> +	id = prog->aux->id;

^ This looks like a concurrent read/write.

Would wrap with READ_ONCE() and corresponding WRITE_ONCE() in
bpf_prog_free_id(). See [1] for rationale.

[1] https://github.com/google/kernel-sanitizers/blob/master/other/READ_WRITE_ONCE.md

> +
> +	/* we do not hold the refcnt, the bpf prog may be released
> +	 * asynchronously and the id would be set to 0.
> +	 */
> +	if (id == 0)
> +		prog_cnt = 0;
> +
> +end:
> +	rcu_read_unlock();
> +
> +	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
> +	    (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
> +	    copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
> +		ret = -EFAULT;
> +
> +	fdput(f);
> +	return ret;
> +}
> +
>  static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
>  {
>  	switch (link->map->map_type) {

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

* Re: [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap
  2022-01-13  9:00 ` [PATCH bpf-next v5 2/2] selftests: bpf: test " Di Zhu
@ 2022-01-15  1:18   ` Andrii Nakryiko
  0 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-01-15  1:18 UTC (permalink / raw)
  To: Di Zhu
  Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
	Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
	john fastabend, KP Singh, Jakub Sitnicki, bpf, open list,
	luzhihao, rose.chen

On Thu, Jan 13, 2022 at 1:01 AM Di Zhu <zhudi2@huawei.com> wrote:
>
> Add test for querying progs attached to sockmap. we use an existing
> libbpf query interface to query prog cnt before and after progs
> attaching to sockmap and check whether the queried prog id is right.
>
> Signed-off-by: Di Zhu <zhudi2@huawei.com>
> Acked-by: Yonghong Song <yhs@fb.com>
> ---
>  .../selftests/bpf/prog_tests/sockmap_basic.c  | 70 +++++++++++++++++++
>  .../bpf/progs/test_sockmap_progs_query.c      | 24 +++++++
>  2 files changed, 94 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index 85db0f4cdd95..06923ea44bad 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -8,6 +8,7 @@
>  #include "test_sockmap_update.skel.h"
>  #include "test_sockmap_invalid_update.skel.h"
>  #include "test_sockmap_skb_verdict_attach.skel.h"
> +#include "test_sockmap_progs_query.skel.h"
>  #include "bpf_iter_sockmap.skel.h"
>
>  #define TCP_REPAIR             19      /* TCP sock is under repair right now */
> @@ -315,6 +316,69 @@ static void test_sockmap_skb_verdict_attach(enum bpf_attach_type first,
>         test_sockmap_skb_verdict_attach__destroy(skel);
>  }
>
> +static __u32 query_prog_id(int prog_fd)
> +{
> +       struct bpf_prog_info info = {};
> +       __u32 info_len = sizeof(info);
> +       int err;
> +
> +       err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
> +       if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
> +           !ASSERT_EQ(info_len, sizeof(info), "bpf_obj_get_info_by_fd"))
> +               return 0;
> +
> +       return info.id;
> +}
> +
> +static void test_sockmap_progs_query(enum bpf_attach_type attach_type)
> +{
> +       struct test_sockmap_progs_query *skel;
> +       int err, map_fd, verdict_fd, duration = 0;
> +       __u32 attach_flags = 0;
> +       __u32 prog_ids[3] = {};
> +       __u32 prog_cnt = 3;
> +
> +       skel = test_sockmap_progs_query__open_and_load();
> +       if (!ASSERT_OK_PTR(skel, "test_sockmap_progs_query__open_and_load"))
> +               return;
> +
> +       map_fd = bpf_map__fd(skel->maps.sock_map);
> +
> +       if (attach_type == BPF_SK_MSG_VERDICT)
> +               verdict_fd = bpf_program__fd(skel->progs.prog_skmsg_verdict);
> +       else
> +               verdict_fd = bpf_program__fd(skel->progs.prog_skb_verdict);
> +
> +       err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> +                            &attach_flags, prog_ids, &prog_cnt);
> +       if (!ASSERT_OK(err, "bpf_prog_query failed"))
> +               goto out;
> +
> +       if (!ASSERT_EQ(attach_flags,  0, "wrong attach_flags on query"))
> +               goto out;
> +
> +       if (!ASSERT_EQ(prog_cnt, 0, "wrong program count on query"))
> +               goto out;
> +
> +       err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
> +       if (!ASSERT_OK(err, "bpf_prog_attach failed"))
> +               goto out;
> +
> +       prog_cnt = 1;
> +       err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> +                            &attach_flags, prog_ids, &prog_cnt);
> +
> +       ASSERT_OK(err, "bpf_prog_query failed");
> +       ASSERT_EQ(attach_flags, 0, "wrong attach_flags on query");
> +       ASSERT_EQ(prog_cnt, 1, "wrong program count on query");
> +       ASSERT_EQ(prog_ids[0], query_prog_id(verdict_fd),
> +                 "wrong prog_ids on query");

See how much easier it is to follow these tests, why didn't you do the
same with err, attach_flags and prog above?


> +
> +       bpf_prog_detach2(verdict_fd, map_fd, attach_type);
> +out:
> +       test_sockmap_progs_query__destroy(skel);
> +}
> +
>  void test_sockmap_basic(void)
>  {
>         if (test__start_subtest("sockmap create_update_free"))
> @@ -341,4 +405,10 @@ void test_sockmap_basic(void)
>                 test_sockmap_skb_verdict_attach(BPF_SK_SKB_STREAM_VERDICT,
>                                                 BPF_SK_SKB_VERDICT);
>         }
> +       if (test__start_subtest("sockmap progs query")) {
> +               test_sockmap_progs_query(BPF_SK_MSG_VERDICT);
> +               test_sockmap_progs_query(BPF_SK_SKB_STREAM_PARSER);
> +               test_sockmap_progs_query(BPF_SK_SKB_STREAM_VERDICT);
> +               test_sockmap_progs_query(BPF_SK_SKB_VERDICT);

Why are these not separate subtests? What's the benefit of bundling
them into one subtest?

> +       }
>  }
> diff --git a/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> new file mode 100644
> index 000000000000..9d58d61c0dee
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +
> +struct {
> +       __uint(type, BPF_MAP_TYPE_SOCKMAP);
> +       __uint(max_entries, 1);
> +       __type(key, __u32);
> +       __type(value, __u64);
> +} sock_map SEC(".maps");
> +
> +SEC("sk_skb")
> +int prog_skb_verdict(struct __sk_buff *skb)
> +{
> +       return SK_PASS;
> +}
> +
> +SEC("sk_msg")
> +int prog_skmsg_verdict(struct sk_msg_md *msg)
> +{
> +       return SK_PASS;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.27.0
>

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
  2022-01-13 16:15 ` [PATCH bpf-next v5 1/2] bpf: support " Jakub Sitnicki
@ 2022-01-15  1:22   ` Andrii Nakryiko
  0 siblings, 0 replies; 18+ messages in thread
From: Andrii Nakryiko @ 2022-01-15  1:22 UTC (permalink / raw)
  To: Jakub Sitnicki
  Cc: Di Zhu, Alexei Starovoitov, David S. Miller, Daniel Borkmann,
	Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
	john fastabend, KP Singh, bpf, open list, luzhihao, rose.chen

On Thu, Jan 13, 2022 at 8:15 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> On Thu, Jan 13, 2022 at 10:00 AM CET, Di Zhu wrote:
> > Right now there is no way to query whether BPF programs are
> > attached to a sockmap or not.
> >
> > we can use the standard interface in libbpf to query, such as:
> > bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
> > the mapFd is the fd of sockmap.
> >
> > Signed-off-by: Di Zhu <zhudi2@huawei.com>
> > Acked-by: Yonghong Song <yhs@fb.com>
> > ---
> >  include/linux/bpf.h  |  9 +++++
> >  kernel/bpf/syscall.c |  5 +++
> >  net/core/sock_map.c  | 78 ++++++++++++++++++++++++++++++++++++++++----
> >  3 files changed, 85 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 6e947cd91152..c4ca14c9f838 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2071,6 +2071,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog,
> >  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);
> >  int sock_map_update_elem_sys(struct bpf_map *map, void *key, void *value, u64 flags);
> > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > +                         union bpf_attr __user *uattr);
> > +
> >  void sock_map_unhash(struct sock *sk);
> >  void sock_map_close(struct sock *sk, long timeout);
> >  #else
> > @@ -2124,6 +2127,12 @@ static inline int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
> >  {
> >       return -EOPNOTSUPP;
> >  }
> > +
> > +static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > +                                       union bpf_attr __user *uattr)
> > +{
> > +     return -EINVAL;
> > +}
> >  #endif /* CONFIG_BPF_SYSCALL */
> >  #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index fa4505f9b611..9e0631f091a6 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -3318,6 +3318,11 @@ static int bpf_prog_query(const union bpf_attr *attr,
> >       case BPF_FLOW_DISSECTOR:
> >       case BPF_SK_LOOKUP:
> >               return netns_bpf_prog_query(attr, uattr);
> > +     case BPF_SK_SKB_STREAM_PARSER:
> > +     case BPF_SK_SKB_STREAM_VERDICT:
> > +     case BPF_SK_MSG_VERDICT:
> > +     case BPF_SK_SKB_VERDICT:
> > +             return sock_map_bpf_prog_query(attr, uattr);
> >       default:
> >               return -EINVAL;
> >       }
> > diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> > index 1827669eedd6..a424f51041ca 100644
> > --- a/net/core/sock_map.c
> > +++ b/net/core/sock_map.c
> > @@ -1416,38 +1416,50 @@ static struct sk_psock_progs *sock_map_progs(struct bpf_map *map)
> >       return NULL;
> >  }
> >
> > -static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
> > -                             struct bpf_prog *old, u32 which)
> > +static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
> > +                             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:
> > -             pprog = &progs->msg_parser;
> > +             *pprog = &progs->msg_parser;
> >               break;
> >  #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
> >       case BPF_SK_SKB_STREAM_PARSER:
> > -             pprog = &progs->stream_parser;
> > +             *pprog = &progs->stream_parser;
> >               break;
> >  #endif
> >       case BPF_SK_SKB_STREAM_VERDICT:
> >               if (progs->skb_verdict)
> >                       return -EBUSY;
> > -             pprog = &progs->stream_verdict;
> > +             *pprog = &progs->stream_verdict;
> >               break;
> >       case BPF_SK_SKB_VERDICT:
> >               if (progs->stream_verdict)
> >                       return -EBUSY;
> > -             pprog = &progs->skb_verdict;
> > +             *pprog = &progs->skb_verdict;
> >               break;
> >       default:
> >               return -EOPNOTSUPP;
> >       }
> >
> > +     return 0;
> > +}
> > +
> > +static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
> > +                             struct bpf_prog *old, u32 which)
> > +{
> > +     struct bpf_prog **pprog;
> > +     int ret;
> > +
> > +     ret = sock_map_prog_lookup(map, &pprog, which);
> > +     if (ret)
> > +             return ret;
> > +
> >       if (old)
> >               return psock_replace_prog(pprog, prog, old);
> >
> > @@ -1455,6 +1467,58 @@ static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
> >       return 0;
> >  }
> >
> > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > +                         union bpf_attr __user *uattr)
> > +{
> > +     __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> > +     u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> > +     struct bpf_prog **pprog;
> > +     struct bpf_prog *prog;
> > +     struct bpf_map *map;
> > +     struct fd f;
> > +     u32 id = 0;
> > +     int ret;
> > +
> > +     if (attr->query.query_flags)
> > +             return -EINVAL;
> > +
> > +     f = fdget(ufd);
> > +     map = __bpf_map_get(f);
> > +     if (IS_ERR(map))
> > +             return PTR_ERR(map);
> > +
> > +     rcu_read_lock();
> > +
> > +     ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> > +     if (ret)
> > +             goto end;
> > +
> > +     prog = *pprog;
> > +     prog_cnt = !prog ? 0 : 1;
> > +
> > +     if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> > +             goto end;
> > +
> > +     id = prog->aux->id;
>
> ^ This looks like a concurrent read/write.

You mean that bpf_prog_load() might be setting it in a different
thread? I think ID is allocated and fixed before prog FD is available
to the user-space, so prog->aux->id is set in stone and immutable in
that regard.

>
> Would wrap with READ_ONCE() and corresponding WRITE_ONCE() in
> bpf_prog_free_id(). See [1] for rationale.
>
> [1] https://github.com/google/kernel-sanitizers/blob/master/other/READ_WRITE_ONCE.md
>
> > +
> > +     /* we do not hold the refcnt, the bpf prog may be released
> > +      * asynchronously and the id would be set to 0.
> > +      */
> > +     if (id == 0)
> > +             prog_cnt = 0;
> > +
> > +end:
> > +     rcu_read_unlock();
> > +
> > +     if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
> > +         (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
> > +         copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
> > +             ret = -EFAULT;
> > +
> > +     fdput(f);
> > +     return ret;
> > +}
> > +
> >  static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
> >  {
> >       switch (link->map->map_type) {

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
  2022-01-15  2:53 ` Andrii Nakryiko
@ 2022-01-15 19:09   ` Jakub Sitnicki
  0 siblings, 0 replies; 18+ messages in thread
From: Jakub Sitnicki @ 2022-01-15 19:09 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: zhudi (E),
	Alexei Starovoitov, David S. Miller, Daniel Borkmann,
	Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
	john fastabend, KP Singh, bpf, open list, Luzhihao (luzhihao,
	Euler), Chenxiang (EulerOS)

On Sat, Jan 15, 2022 at 03:53 AM CET, Andrii Nakryiko wrote:
> On Fri, Jan 14, 2022 at 6:38 PM zhudi (E) <zhudi2@huawei.com> wrote:
>>
>> > On Thu, Jan 13, 2022 at 8:15 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>> > >
>> > > On Thu, Jan 13, 2022 at 10:00 AM CET, Di Zhu wrote:

[...]

>> > > > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
>> > > > +                         union bpf_attr __user *uattr)
>> > > > +{
>> > > > +     __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
>> > > > +     u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
>> > > > +     struct bpf_prog **pprog;
>> > > > +     struct bpf_prog *prog;
>> > > > +     struct bpf_map *map;
>> > > > +     struct fd f;
>> > > > +     u32 id = 0;
>> > > > +     int ret;
>> > > > +
>> > > > +     if (attr->query.query_flags)
>> > > > +             return -EINVAL;
>> > > > +
>> > > > +     f = fdget(ufd);
>> > > > +     map = __bpf_map_get(f);
>> > > > +     if (IS_ERR(map))
>> > > > +             return PTR_ERR(map);
>> > > > +
>> > > > +     rcu_read_lock();
>> > > > +
>> > > > +     ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
>> > > > +     if (ret)
>> > > > +             goto end;
>> > > > +
>> > > > +     prog = *pprog;
>> > > > +     prog_cnt = !prog ? 0 : 1;
>> > > > +
>> > > > +     if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
>> > > > +             goto end;
>> > > > +
>> > > > +     id = prog->aux->id;
>> > >
>> > > ^ This looks like a concurrent read/write.
>> >
>> > You mean that bpf_prog_load() might be setting it in a different
>> > thread? I think ID is allocated and fixed before prog FD is available
>> > to the user-space, so prog->aux->id is set in stone and immutable in
>> > that regard.
>>
>> What we're talking about here is that bpf_prog_free_id() will write the id
>> identifier synchronously.
>
> Hm.. let's say bpf_prog_free_id() happens right after we read id 123.
> It's impossible to distinguish that from reading valid ID (that's not
> yet freed), returning it to user-space and before user-space can do
> anything about that this program and it's ID are freed. User-space
> either way will get an ID that's not valid anymore. I don't see any
> use of READ_ONCE/WRITE_ONCE with prog->aux->id, which is why I was
> asking what changed.
>

You're right, READ_ONCE/WRITE_ONCE is not improving anything here.

I've suggested it not to make the query op more reliable, but rather to
mark the shared access.

But in this case annotating it with data_race() [1] would be a better
fit, I think, because we don't care if we get the old or the new value.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/memory-model/Documentation/access-marking.txt#n58

>>
>> >
>> > >
>> > > Would wrap with READ_ONCE() and corresponding WRITE_ONCE() in
>> > > bpf_prog_free_id(). See [1] for rationale.
>> > >
>> > > [1]
>> > https://github.com/google/kernel-sanitizers/blob/master/other/READ_WRITE_O
>> > NCE.md
>> > >

[...]

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
  2022-01-15  2:38 zhudi (E)
@ 2022-01-15  2:53 ` Andrii Nakryiko
  2022-01-15 19:09   ` Jakub Sitnicki
  0 siblings, 1 reply; 18+ messages in thread
From: Andrii Nakryiko @ 2022-01-15  2:53 UTC (permalink / raw)
  To: zhudi (E)
  Cc: Jakub Sitnicki, Alexei Starovoitov, David S. Miller,
	Daniel Borkmann, Andrii Nakryiko, Martin Lau, Song Liu,
	Yonghong Song, john fastabend, KP Singh, bpf, open list,
	Luzhihao (luzhihao, Euler), Chenxiang (EulerOS)

On Fri, Jan 14, 2022 at 6:38 PM zhudi (E) <zhudi2@huawei.com> wrote:
>
> > On Thu, Jan 13, 2022 at 8:15 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
> > >
> > > On Thu, Jan 13, 2022 at 10:00 AM CET, Di Zhu wrote:
> > > > Right now there is no way to query whether BPF programs are
> > > > attached to a sockmap or not.
> > > >
> > > > we can use the standard interface in libbpf to query, such as:
> > > > bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
> > > > the mapFd is the fd of sockmap.
> > > >
> > > > Signed-off-by: Di Zhu <zhudi2@huawei.com>
> > > > Acked-by: Yonghong Song <yhs@fb.com>
> > > > ---
> > > >  include/linux/bpf.h  |  9 +++++
> > > >  kernel/bpf/syscall.c |  5 +++
> > > >  net/core/sock_map.c  | 78
> > ++++++++++++++++++++++++++++++++++++++++----
> > > >  3 files changed, 85 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > > index 6e947cd91152..c4ca14c9f838 100644
> > > > --- a/include/linux/bpf.h
> > > > +++ b/include/linux/bpf.h
> > > > @@ -2071,6 +2071,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog
> > *prog,
> > > >  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);
> > > >  int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
> > *value, u64 flags);
> > > > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > > > +                         union bpf_attr __user *uattr);
> > > > +
> > > >  void sock_map_unhash(struct sock *sk);
> > > >  void sock_map_close(struct sock *sk, long timeout);
> > > >  #else
> > > > @@ -2124,6 +2127,12 @@ static inline int
> > sock_map_update_elem_sys(struct bpf_map *map, void *key, void
> > > >  {
> > > >       return -EOPNOTSUPP;
> > > >  }
> > > > +
> > > > +static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > > > +                                       union bpf_attr __user
> > *uattr)
> > > > +{
> > > > +     return -EINVAL;
> > > > +}
> > > >  #endif /* CONFIG_BPF_SYSCALL */
> > > >  #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
> > > >
> > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > > index fa4505f9b611..9e0631f091a6 100644
> > > > --- a/kernel/bpf/syscall.c
> > > > +++ b/kernel/bpf/syscall.c
> > > > @@ -3318,6 +3318,11 @@ static int bpf_prog_query(const union bpf_attr
> > *attr,
> > > >       case BPF_FLOW_DISSECTOR:
> > > >       case BPF_SK_LOOKUP:
> > > >               return netns_bpf_prog_query(attr, uattr);
> > > > +     case BPF_SK_SKB_STREAM_PARSER:
> > > > +     case BPF_SK_SKB_STREAM_VERDICT:
> > > > +     case BPF_SK_MSG_VERDICT:
> > > > +     case BPF_SK_SKB_VERDICT:
> > > > +             return sock_map_bpf_prog_query(attr, uattr);
> > > >       default:
> > > >               return -EINVAL;
> > > >       }
> > > > diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> > > > index 1827669eedd6..a424f51041ca 100644
> > > > --- a/net/core/sock_map.c
> > > > +++ b/net/core/sock_map.c
> > > > @@ -1416,38 +1416,50 @@ static struct sk_psock_progs
> > *sock_map_progs(struct bpf_map *map)
> > > >       return NULL;
> > > >  }
> > > >
> > > > -static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog
> > *prog,
> > > > -                             struct bpf_prog *old, u32 which)
> > > > +static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog
> > ***pprog,
> > > > +                             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:
> > > > -             pprog = &progs->msg_parser;
> > > > +             *pprog = &progs->msg_parser;
> > > >               break;
> > > >  #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
> > > >       case BPF_SK_SKB_STREAM_PARSER:
> > > > -             pprog = &progs->stream_parser;
> > > > +             *pprog = &progs->stream_parser;
> > > >               break;
> > > >  #endif
> > > >       case BPF_SK_SKB_STREAM_VERDICT:
> > > >               if (progs->skb_verdict)
> > > >                       return -EBUSY;
> > > > -             pprog = &progs->stream_verdict;
> > > > +             *pprog = &progs->stream_verdict;
> > > >               break;
> > > >       case BPF_SK_SKB_VERDICT:
> > > >               if (progs->stream_verdict)
> > > >                       return -EBUSY;
> > > > -             pprog = &progs->skb_verdict;
> > > > +             *pprog = &progs->skb_verdict;
> > > >               break;
> > > >       default:
> > > >               return -EOPNOTSUPP;
> > > >       }
> > > >
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog
> > *prog,
> > > > +                             struct bpf_prog *old, u32 which)
> > > > +{
> > > > +     struct bpf_prog **pprog;
> > > > +     int ret;
> > > > +
> > > > +     ret = sock_map_prog_lookup(map, &pprog, which);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > >       if (old)
> > > >               return psock_replace_prog(pprog, prog, old);
> > > >
> > > > @@ -1455,6 +1467,58 @@ static int sock_map_prog_update(struct bpf_map
> > *map, struct bpf_prog *prog,
> > > >       return 0;
> > > >  }
> > > >
> > > > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > > > +                         union bpf_attr __user *uattr)
> > > > +{
> > > > +     __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> > > > +     u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> > > > +     struct bpf_prog **pprog;
> > > > +     struct bpf_prog *prog;
> > > > +     struct bpf_map *map;
> > > > +     struct fd f;
> > > > +     u32 id = 0;
> > > > +     int ret;
> > > > +
> > > > +     if (attr->query.query_flags)
> > > > +             return -EINVAL;
> > > > +
> > > > +     f = fdget(ufd);
> > > > +     map = __bpf_map_get(f);
> > > > +     if (IS_ERR(map))
> > > > +             return PTR_ERR(map);
> > > > +
> > > > +     rcu_read_lock();
> > > > +
> > > > +     ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> > > > +     if (ret)
> > > > +             goto end;
> > > > +
> > > > +     prog = *pprog;
> > > > +     prog_cnt = !prog ? 0 : 1;
> > > > +
> > > > +     if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> > > > +             goto end;
> > > > +
> > > > +     id = prog->aux->id;
> > >
> > > ^ This looks like a concurrent read/write.
> >
> > You mean that bpf_prog_load() might be setting it in a different
> > thread? I think ID is allocated and fixed before prog FD is available
> > to the user-space, so prog->aux->id is set in stone and immutable in
> > that regard.
>
> What we're talking about here is that bpf_prog_free_id() will write the id
> identifier synchronously.

Hm.. let's say bpf_prog_free_id() happens right after we read id 123.
It's impossible to distinguish that from reading valid ID (that's not
yet freed), returning it to user-space and before user-space can do
anything about that this program and it's ID are freed. User-space
either way will get an ID that's not valid anymore. I don't see any
use of READ_ONCE/WRITE_ONCE with prog->aux->id, which is why I was
asking what changed.

>
> >
> > >
> > > Would wrap with READ_ONCE() and corresponding WRITE_ONCE() in
> > > bpf_prog_free_id(). See [1] for rationale.
> > >
> > > [1]
> > https://github.com/google/kernel-sanitizers/blob/master/other/READ_WRITE_O
> > NCE.md
> > >
> > > > +
> > > > +     /* we do not hold the refcnt, the bpf prog may be released
> > > > +      * asynchronously and the id would be set to 0.
> > > > +      */
> > > > +     if (id == 0)
> > > > +             prog_cnt = 0;
> > > > +
> > > > +end:
> > > > +     rcu_read_unlock();
> > > > +
> > > > +     if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
> > > > +         (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
> > > > +         copy_to_user(&uattr->query.prog_cnt, &prog_cnt,
> > sizeof(prog_cnt)))
> > > > +             ret = -EFAULT;
> > > > +
> > > > +     fdput(f);
> > > > +     return ret;
> > > > +}
> > > > +
> > > >  static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
> > > >  {
> > > >       switch (link->map->map_type) {

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
@ 2022-01-15  2:38 zhudi (E)
  2022-01-15  2:53 ` Andrii Nakryiko
  0 siblings, 1 reply; 18+ messages in thread
From: zhudi (E) @ 2022-01-15  2:38 UTC (permalink / raw)
  To: Andrii Nakryiko, Jakub Sitnicki
  Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
	Andrii Nakryiko, Martin Lau, Song Liu, Yonghong Song,
	john fastabend, KP Singh, bpf, open list, Luzhihao (luzhihao,
	Euler), Chenxiang (EulerOS)

> On Thu, Jan 13, 2022 at 8:15 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
> >
> > On Thu, Jan 13, 2022 at 10:00 AM CET, Di Zhu wrote:
> > > Right now there is no way to query whether BPF programs are
> > > attached to a sockmap or not.
> > >
> > > we can use the standard interface in libbpf to query, such as:
> > > bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
> > > the mapFd is the fd of sockmap.
> > >
> > > Signed-off-by: Di Zhu <zhudi2@huawei.com>
> > > Acked-by: Yonghong Song <yhs@fb.com>
> > > ---
> > >  include/linux/bpf.h  |  9 +++++
> > >  kernel/bpf/syscall.c |  5 +++
> > >  net/core/sock_map.c  | 78
> ++++++++++++++++++++++++++++++++++++++++----
> > >  3 files changed, 85 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 6e947cd91152..c4ca14c9f838 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -2071,6 +2071,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog
> *prog,
> > >  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);
> > >  int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
> *value, u64 flags);
> > > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > > +                         union bpf_attr __user *uattr);
> > > +
> > >  void sock_map_unhash(struct sock *sk);
> > >  void sock_map_close(struct sock *sk, long timeout);
> > >  #else
> > > @@ -2124,6 +2127,12 @@ static inline int
> sock_map_update_elem_sys(struct bpf_map *map, void *key, void
> > >  {
> > >       return -EOPNOTSUPP;
> > >  }
> > > +
> > > +static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > > +                                       union bpf_attr __user
> *uattr)
> > > +{
> > > +     return -EINVAL;
> > > +}
> > >  #endif /* CONFIG_BPF_SYSCALL */
> > >  #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
> > >
> > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > index fa4505f9b611..9e0631f091a6 100644
> > > --- a/kernel/bpf/syscall.c
> > > +++ b/kernel/bpf/syscall.c
> > > @@ -3318,6 +3318,11 @@ static int bpf_prog_query(const union bpf_attr
> *attr,
> > >       case BPF_FLOW_DISSECTOR:
> > >       case BPF_SK_LOOKUP:
> > >               return netns_bpf_prog_query(attr, uattr);
> > > +     case BPF_SK_SKB_STREAM_PARSER:
> > > +     case BPF_SK_SKB_STREAM_VERDICT:
> > > +     case BPF_SK_MSG_VERDICT:
> > > +     case BPF_SK_SKB_VERDICT:
> > > +             return sock_map_bpf_prog_query(attr, uattr);
> > >       default:
> > >               return -EINVAL;
> > >       }
> > > diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> > > index 1827669eedd6..a424f51041ca 100644
> > > --- a/net/core/sock_map.c
> > > +++ b/net/core/sock_map.c
> > > @@ -1416,38 +1416,50 @@ static struct sk_psock_progs
> *sock_map_progs(struct bpf_map *map)
> > >       return NULL;
> > >  }
> > >
> > > -static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog
> *prog,
> > > -                             struct bpf_prog *old, u32 which)
> > > +static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog
> ***pprog,
> > > +                             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:
> > > -             pprog = &progs->msg_parser;
> > > +             *pprog = &progs->msg_parser;
> > >               break;
> > >  #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
> > >       case BPF_SK_SKB_STREAM_PARSER:
> > > -             pprog = &progs->stream_parser;
> > > +             *pprog = &progs->stream_parser;
> > >               break;
> > >  #endif
> > >       case BPF_SK_SKB_STREAM_VERDICT:
> > >               if (progs->skb_verdict)
> > >                       return -EBUSY;
> > > -             pprog = &progs->stream_verdict;
> > > +             *pprog = &progs->stream_verdict;
> > >               break;
> > >       case BPF_SK_SKB_VERDICT:
> > >               if (progs->stream_verdict)
> > >                       return -EBUSY;
> > > -             pprog = &progs->skb_verdict;
> > > +             *pprog = &progs->skb_verdict;
> > >               break;
> > >       default:
> > >               return -EOPNOTSUPP;
> > >       }
> > >
> > > +     return 0;
> > > +}
> > > +
> > > +static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog
> *prog,
> > > +                             struct bpf_prog *old, u32 which)
> > > +{
> > > +     struct bpf_prog **pprog;
> > > +     int ret;
> > > +
> > > +     ret = sock_map_prog_lookup(map, &pprog, which);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > >       if (old)
> > >               return psock_replace_prog(pprog, prog, old);
> > >
> > > @@ -1455,6 +1467,58 @@ static int sock_map_prog_update(struct bpf_map
> *map, struct bpf_prog *prog,
> > >       return 0;
> > >  }
> > >
> > > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > > +                         union bpf_attr __user *uattr)
> > > +{
> > > +     __u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> > > +     u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> > > +     struct bpf_prog **pprog;
> > > +     struct bpf_prog *prog;
> > > +     struct bpf_map *map;
> > > +     struct fd f;
> > > +     u32 id = 0;
> > > +     int ret;
> > > +
> > > +     if (attr->query.query_flags)
> > > +             return -EINVAL;
> > > +
> > > +     f = fdget(ufd);
> > > +     map = __bpf_map_get(f);
> > > +     if (IS_ERR(map))
> > > +             return PTR_ERR(map);
> > > +
> > > +     rcu_read_lock();
> > > +
> > > +     ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> > > +     if (ret)
> > > +             goto end;
> > > +
> > > +     prog = *pprog;
> > > +     prog_cnt = !prog ? 0 : 1;
> > > +
> > > +     if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> > > +             goto end;
> > > +
> > > +     id = prog->aux->id;
> >
> > ^ This looks like a concurrent read/write.
> 
> You mean that bpf_prog_load() might be setting it in a different
> thread? I think ID is allocated and fixed before prog FD is available
> to the user-space, so prog->aux->id is set in stone and immutable in
> that regard.

What we're talking about here is that bpf_prog_free_id() will write the id
identifier synchronously.

> 
> >
> > Would wrap with READ_ONCE() and corresponding WRITE_ONCE() in
> > bpf_prog_free_id(). See [1] for rationale.
> >
> > [1]
> https://github.com/google/kernel-sanitizers/blob/master/other/READ_WRITE_O
> NCE.md
> >
> > > +
> > > +     /* we do not hold the refcnt, the bpf prog may be released
> > > +      * asynchronously and the id would be set to 0.
> > > +      */
> > > +     if (id == 0)
> > > +             prog_cnt = 0;
> > > +
> > > +end:
> > > +     rcu_read_unlock();
> > > +
> > > +     if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
> > > +         (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
> > > +         copy_to_user(&uattr->query.prog_cnt, &prog_cnt,
> sizeof(prog_cnt)))
> > > +             ret = -EFAULT;
> > > +
> > > +     fdput(f);
> > > +     return ret;
> > > +}
> > > +
> > >  static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
> > >  {
> > >       switch (link->map->map_type) {

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
@ 2022-01-14  5:44 zhudi (E)
  0 siblings, 0 replies; 18+ messages in thread
From: zhudi (E) @ 2022-01-14  5:44 UTC (permalink / raw)
  To: Jakub Sitnicki
  Cc: ast, davem, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, bpf, linux-kernel, Luzhihao (luzhihao,
	Euler), Chenxiang (EulerOS)

> On Thu, Jan 13, 2022 at 10:00 AM CET, Di Zhu wrote:
> > Right now there is no way to query whether BPF programs are
> > attached to a sockmap or not.
> >
> > we can use the standard interface in libbpf to query, such as:
> > bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
> > the mapFd is the fd of sockmap.
> >
> > Signed-off-by: Di Zhu <zhudi2@huawei.com>
> > Acked-by: Yonghong Song <yhs@fb.com>
> > ---
> >  include/linux/bpf.h  |  9 +++++
> >  kernel/bpf/syscall.c |  5 +++
> >  net/core/sock_map.c  | 78
> ++++++++++++++++++++++++++++++++++++++++----
> >  3 files changed, 85 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index 6e947cd91152..c4ca14c9f838 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -2071,6 +2071,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog
> *prog,
> >  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);
> >  int sock_map_update_elem_sys(struct bpf_map *map, void *key, void *value,
> u64 flags);


	.......


> > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > +			    union bpf_attr __user *uattr)
> > +{
> > +	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> > +	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> > +	struct bpf_prog **pprog;
> > +	struct bpf_prog *prog;
> > +	struct bpf_map *map;
> > +	struct fd f;
> > +	u32 id = 0;
> > +	int ret;
> > +
> > +	if (attr->query.query_flags)
> > +		return -EINVAL;
> > +
> > +	f = fdget(ufd);
> > +	map = __bpf_map_get(f);
> > +	if (IS_ERR(map))
> > +		return PTR_ERR(map);
> > +
> > +	rcu_read_lock();
> > +
> > +	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> > +	if (ret)
> > +		goto end;
> > +
> > +	prog = *pprog;
> > +	prog_cnt = !prog ? 0 : 1;
> > +
> > +	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> > +		goto end;
> > +
> > +	id = prog->aux->id;
> 
> ^ This looks like a concurrent read/write.
> 
> Would wrap with READ_ONCE() and corresponding WRITE_ONCE() in
> bpf_prog_free_id(). See [1] for rationale.
> 
> [1]
> https://github.com/google/kernel-sanitizers/blob/master/other/READ_WRITE_O
> NCE.md


Thanks for your advice, I will modify this code.

 
> > +
> > +	/* we do not hold the refcnt, the bpf prog may be released
> > +	 * asynchronously and the id would be set to 0.
> > +	 */
> > +	if (id == 0)
> > +		prog_cnt = 0;
> > +
> > +end:
> > +	rcu_read_unlock();
> > +
> > +	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
> > +	    (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
> > +	    copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
> > +		ret = -EFAULT;
> > +
> > +	fdput(f);
> > +	return ret;
> > +}
> > +
> >  static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
> >  {
> >  	switch (link->map->map_type) {

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
@ 2021-11-08  2:13 zhudi (E)
  0 siblings, 0 replies; 18+ messages in thread
From: zhudi (E) @ 2021-11-08  2:13 UTC (permalink / raw)
  To: Cong Wang
  Cc: davem, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, jakub, bpf, linux-kernel

> On Thu, Nov 04, 2021 at 09:07:44AM +0800, Di Zhu wrote:
> > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > +			    union bpf_attr __user *uattr)
> > +{
> > +	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> > +	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> > +	struct bpf_prog **pprog;
> > +	struct bpf_prog *prog;
> > +	struct bpf_map *map;
> > +	struct fd f;
> > +	u32 id = 0;
> > +	int ret;
> > +
> > +	if (attr->query.query_flags)
> > +		return -EINVAL;
> > +
> > +	f = fdget(ufd);
> > +	map = __bpf_map_get(f);
> > +	if (IS_ERR(map))
> > +		return PTR_ERR(map);
> > +
> > +	rcu_read_lock();
> > +
> > +	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> > +	if (ret)
> > +		goto end;
> > +
> > +	prog = *pprog;
> > +	prog_cnt = (!prog) ? 0 : 1;
> > +
> > +	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> > +		goto end;
> 

> This sanity check (except prog_cnt) can be moved before RCU read lock?

I think we should call sock_map_prog_lookup() in any case. Because we can
just return query results(such as -EOPNOTSUPP) which may not care about
the prog_ids.

So this sanity check should right behind this call and must be in rcu critical zone.

> > +
> > +	id = prog->aux->id;
> > +	if (id == 0)
> > +		prog_cnt = 0;
> 
> The id seems generic, so why not handle it in bpf_prog_query() for all progs?

The prog id is a generic, but different progs have different organizational forms, 
so they can only be handled differently at present...

> > +
> > +end:
> > +	rcu_read_unlock();
> > +
> > +	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
> 
> 'flags' is always 0 here, right? So this is not needed as uattr has been already
> cleared in __sys_bpf().

 I recheck the code, it seems that __sys_bpf() does not do this clear things.

 
> Thanks.

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
  2021-11-04  1:07 Di Zhu
  2021-11-04  5:31 ` Yonghong Song
@ 2021-11-05 19:51 ` Cong Wang
  1 sibling, 0 replies; 18+ messages in thread
From: Cong Wang @ 2021-11-05 19:51 UTC (permalink / raw)
  To: Di Zhu
  Cc: davem, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, jakub, bpf, linux-kernel

On Thu, Nov 04, 2021 at 09:07:44AM +0800, Di Zhu wrote:
> +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> +			    union bpf_attr __user *uattr)
> +{
> +	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> +	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> +	struct bpf_prog **pprog;
> +	struct bpf_prog *prog;
> +	struct bpf_map *map;
> +	struct fd f;
> +	u32 id = 0;
> +	int ret;
> +
> +	if (attr->query.query_flags)
> +		return -EINVAL;
> +
> +	f = fdget(ufd);
> +	map = __bpf_map_get(f);
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	rcu_read_lock();
> +
> +	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> +	if (ret)
> +		goto end;
> +
> +	prog = *pprog;
> +	prog_cnt = (!prog) ? 0 : 1;
> +
> +	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> +		goto end;

This sanity check (except prog_cnt) can be moved before RCU read lock?

> +
> +	id = prog->aux->id;
> +	if (id == 0)
> +		prog_cnt = 0;

The id seems generic, so why not handle it in bpf_prog_query() for all progs?

> +
> +end:
> +	rcu_read_unlock();
> +
> +	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||

'flags' is always 0 here, right? So this is not needed as uattr has been already
cleared in __sys_bpf().


Thanks.

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
  2021-11-05  1:57 Di Zhu
@ 2021-11-05  4:23 ` Yonghong Song
  0 siblings, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2021-11-05  4:23 UTC (permalink / raw)
  To: Di Zhu, davem, ast, daniel, andrii, kafai, songliubraving,
	john.fastabend, kpsingh, jakub
  Cc: bpf, linux-kernel



On 11/4/21 6:57 PM, Di Zhu wrote:
> Right now there is no way to query whether BPF programs are
> attached to a sockmap or not.
> 
> we can use the standard interface in libbpf to query, such as:
> bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
> the mapFd is the fd of sockmap.
> 
> Signed-off-by: Di Zhu <zhudi2@huawei.com>

Acked-by: Yonghong Song <yhs@fb.com>

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

* [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
@ 2021-11-05  1:57 Di Zhu
  2021-11-05  4:23 ` Yonghong Song
  0 siblings, 1 reply; 18+ messages in thread
From: Di Zhu @ 2021-11-05  1:57 UTC (permalink / raw)
  To: davem, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, jakub
  Cc: bpf, linux-kernel, zhudi2

Right now there is no way to query whether BPF programs are
attached to a sockmap or not.

we can use the standard interface in libbpf to query, such as:
bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
the mapFd is the fd of sockmap.

Signed-off-by: Di Zhu <zhudi2@huawei.com>
---
/* v2 */
- John Fastabend <john.fastabend@gmail.com>
  - add selftest code

/* v3 */
 - avoid sleeping caused by copy_to_user() in rcu critical zone

/* v4 */
 - Alexei Starovoitov <alexei.starovoitov@gmail.com>
  -split into two patches, one for core code and one for selftest.

/* v5 */
 - Yonghong Song <yhs@fb.com>
  -Some naming and formatting changes
  -use ASSERT_* macros in selftest code
  -Add the necessary comments
---
 include/linux/bpf.h  |  9 +++++
 kernel/bpf/syscall.c |  5 +++
 net/core/sock_map.c  | 78 ++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 85 insertions(+), 7 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d604c8251d88..235ea7fc5fd8 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1961,6 +1961,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog,
 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);
 int sock_map_update_elem_sys(struct bpf_map *map, void *key, void *value, u64 flags);
+int sock_map_bpf_prog_query(const union bpf_attr *attr,
+			    union bpf_attr __user *uattr);
+
 void sock_map_unhash(struct sock *sk);
 void sock_map_close(struct sock *sk, long timeout);
 #else
@@ -2014,6 +2017,12 @@ static inline int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
 {
 	return -EOPNOTSUPP;
 }
+
+static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
+					  union bpf_attr __user *uattr)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_BPF_SYSCALL */
 #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4e50c0bfdb7d..748102c3e0c9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3275,6 +3275,11 @@ static int bpf_prog_query(const union bpf_attr *attr,
 	case BPF_FLOW_DISSECTOR:
 	case BPF_SK_LOOKUP:
 		return netns_bpf_prog_query(attr, uattr);
+	case BPF_SK_SKB_STREAM_PARSER:
+	case BPF_SK_SKB_STREAM_VERDICT:
+	case BPF_SK_MSG_VERDICT:
+	case BPF_SK_SKB_VERDICT:
+		return sock_map_bpf_prog_query(attr, uattr);
 	default:
 		return -EINVAL;
 	}
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index e252b8ec2b85..3801d565bddd 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1412,38 +1412,50 @@ static struct sk_psock_progs *sock_map_progs(struct bpf_map *map)
 	return NULL;
 }
 
-static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
-				struct bpf_prog *old, u32 which)
+static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
+				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:
-		pprog = &progs->msg_parser;
+		*pprog = &progs->msg_parser;
 		break;
 #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
 	case BPF_SK_SKB_STREAM_PARSER:
-		pprog = &progs->stream_parser;
+		*pprog = &progs->stream_parser;
 		break;
 #endif
 	case BPF_SK_SKB_STREAM_VERDICT:
 		if (progs->skb_verdict)
 			return -EBUSY;
-		pprog = &progs->stream_verdict;
+		*pprog = &progs->stream_verdict;
 		break;
 	case BPF_SK_SKB_VERDICT:
 		if (progs->stream_verdict)
 			return -EBUSY;
-		pprog = &progs->skb_verdict;
+		*pprog = &progs->skb_verdict;
 		break;
 	default:
 		return -EOPNOTSUPP;
 	}
 
+	return 0;
+}
+
+static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
+				struct bpf_prog *old, u32 which)
+{
+	struct bpf_prog **pprog;
+	int ret;
+
+	ret = sock_map_prog_lookup(map, &pprog, which);
+	if (ret)
+		return ret;
+
 	if (old)
 		return psock_replace_prog(pprog, prog, old);
 
@@ -1451,6 +1463,58 @@ static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
 	return 0;
 }
 
+int sock_map_bpf_prog_query(const union bpf_attr *attr,
+			    union bpf_attr __user *uattr)
+{
+	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
+	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
+	struct bpf_prog **pprog;
+	struct bpf_prog *prog;
+	struct bpf_map *map;
+	struct fd f;
+	u32 id = 0;
+	int ret;
+
+	if (attr->query.query_flags)
+		return -EINVAL;
+
+	f = fdget(ufd);
+	map = __bpf_map_get(f);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	rcu_read_lock();
+
+	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
+	if (ret)
+		goto end;
+
+	prog = *pprog;
+	prog_cnt = !prog ? 0 : 1;
+
+	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
+		goto end;
+
+	id = prog->aux->id;
+
+	/* we do not hold the refcnt, the bpf prog may be released
+	 * asynchronously and the id would be set to 0.
+	 */
+	if (id == 0)
+		prog_cnt = 0;
+
+end:
+	rcu_read_unlock();
+
+	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
+	    (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
+	    copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
+		ret = -EFAULT;
+
+	fdput(f);
+	return ret;
+}
+
 static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
 {
 	switch (link->map->map_type) {
-- 
2.27.0


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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
@ 2021-11-04  6:35 zhudi (E)
  0 siblings, 0 replies; 18+ messages in thread
From: zhudi (E) @ 2021-11-04  6:35 UTC (permalink / raw)
  To: Yonghong Song, davem, ast, daniel, andrii, kafai, songliubraving,
	john.fastabend, kpsingh, jakub
  Cc: bpf, linux-kernel



> -----邮件原件-----
> 发件人: Yonghong Song [mailto:yhs@fb.com]
> 发送时间: 2021年11月4日 14:31
> 收件人: zhudi (E) <zhudi2@huawei.com>; davem@davemloft.net;
> ast@kernel.org; daniel@iogearbox.net; andrii@kernel.org; kafai@fb.com;
> songliubraving@fb.com; john.fastabend@gmail.com; kpsingh@kernel.org;
> jakub@cloudflare.com
> 抄送: bpf@vger.kernel.org; linux-kernel@vger.kernel.org
> 主题: Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs
> attached to sockmap
> 
> 
> 
> On 11/3/21 11:07 PM, zhudi (E) wrote:
> >   > On 11/3/21 6:07 PM, Di Zhu wrote:
> >>> Right now there is no way to query whether BPF programs are
> >>> attached to a sockmap or not.
> >>>
> >>> we can use the standard interface in libbpf to query, such as:
> >>> bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
> >>> the mapFd is the fd of sockmap.
> >>>
> >>> Signed-off-by: Di Zhu <zhudi2@huawei.com>
> >>> ---
> >>> /* v2 */
> >>> - John Fastabend <john.fastabend@gmail.com>
> >>>     - add selftest code
> >>>
> >>> /* v3 */
> >>>    - avoid sleeping caused by copy_to_user() in rcu critical zone
> >>>
> >>> /* v4 */
> >>>    - Alexei Starovoitov <alexei.starovoitov@gmail.com>
> >>>     -split into two patches, one for core code and one for selftest.
> >>>
> >>> /* v5 */
> >>>    - Yonghong Song <yhs@fb.com>
> >>>     -Some naming and formatting changes
> >>> ---
> >>>    include/linux/bpf.h  |  9 ++++++
> >>>    kernel/bpf/syscall.c |  5 +++
> >>>    net/core/sock_map.c  | 74
> >> +++++++++++++++++++++++++++++++++++++++-----
> >>>    3 files changed, 81 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >>> index d604c8251d88..235ea7fc5fd8 100644
> >>> --- a/include/linux/bpf.h
> >>> +++ b/include/linux/bpf.h
> >>> @@ -1961,6 +1961,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog
> >> *prog,
> >>>    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);
> >>>    int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
> >> *value, u64 flags);
> >>> +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> >>> +			    union bpf_attr __user *uattr);
> >>> +
> >>>    void sock_map_unhash(struct sock *sk);
> >>>    void sock_map_close(struct sock *sk, long timeout);
> >>>    #else
> >>> @@ -2014,6 +2017,12 @@ static inline int
> sock_map_update_elem_sys(struct
> >> bpf_map *map, void *key, void
> >>>    {
> >>>    	return -EOPNOTSUPP;
> >>>    }
> >>> +
> >>> +static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
> >>> +					  union bpf_attr __user *uattr)
> >>> +{
> >>> +	return -EINVAL;
> >>> +}
> >>>    #endif /* CONFIG_BPF_SYSCALL */
> >>>    #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
> >>>
> >>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> >>> index 4e50c0bfdb7d..748102c3e0c9 100644
> >>> --- a/kernel/bpf/syscall.c
> >>> +++ b/kernel/bpf/syscall.c
> >>> @@ -3275,6 +3275,11 @@ static int bpf_prog_query(const union bpf_attr
> >> *attr,
> >>>    	case BPF_FLOW_DISSECTOR:
> >>>    	case BPF_SK_LOOKUP:
> >>>    		return netns_bpf_prog_query(attr, uattr);
> >>> +	case BPF_SK_SKB_STREAM_PARSER:
> >>> +	case BPF_SK_SKB_STREAM_VERDICT:
> >>> +	case BPF_SK_MSG_VERDICT:
> >>> +	case BPF_SK_SKB_VERDICT:
> >>> +		return sock_map_bpf_prog_query(attr, uattr);
> >>>    	default:
> >>>    		return -EINVAL;
> >>>    	}
> >>> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> >>> index e252b8ec2b85..0320d27550fe 100644
> >>> --- a/net/core/sock_map.c
> >>> +++ b/net/core/sock_map.c
> >>> @@ -1412,38 +1412,50 @@ static struct sk_psock_progs
> >> *sock_map_progs(struct bpf_map *map)
> >>>    	return NULL;
> >>>    }
> >>>
> >>> -static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog
> >> *prog,
> >>> -				struct bpf_prog *old, u32 which)
> >>> +static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog
> >> ***pprog,
> >>> +				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:
> >>> -		pprog = &progs->msg_parser;
> >>> +		*pprog = &progs->msg_parser;
> >>>    		break;
> >>>    #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
> >>>    	case BPF_SK_SKB_STREAM_PARSER:
> >>> -		pprog = &progs->stream_parser;
> >>> +		*pprog = &progs->stream_parser;
> >>>    		break;
> >>>    #endif
> >>>    	case BPF_SK_SKB_STREAM_VERDICT:
> >>>    		if (progs->skb_verdict)
> >>>    			return -EBUSY;
> >>> -		pprog = &progs->stream_verdict;
> >>> +		*pprog = &progs->stream_verdict;
> >>>    		break;
> >>>    	case BPF_SK_SKB_VERDICT:
> >>>    		if (progs->stream_verdict)
> >>>    			return -EBUSY;
> >>> -		pprog = &progs->skb_verdict;
> >>> +		*pprog = &progs->skb_verdict;
> >>>    		break;
> >>>    	default:
> >>>    		return -EOPNOTSUPP;
> >>>    	}
> >>>
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog
> >> *prog,
> >>> +				struct bpf_prog *old, u32 which)
> >>> +{
> >>> +	struct bpf_prog **pprog;
> >>> +	int ret;
> >>> +
> >>> +	ret = sock_map_prog_lookup(map, &pprog, which);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>>    	if (old)
> >>>    		return psock_replace_prog(pprog, prog, old);
> >>>
> >>> @@ -1451,6 +1463,54 @@ static int sock_map_prog_update(struct
> bpf_map
> >> *map, struct bpf_prog *prog,
> >>>    	return 0;
> >>>    }
> >>>
> >>> +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> >>> +			    union bpf_attr __user *uattr)
> >>> +{
> >>> +	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> >>> +	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> >>> +	struct bpf_prog **pprog;
> >>> +	struct bpf_prog *prog;
> >>> +	struct bpf_map *map;
> >>> +	struct fd f;
> >>> +	u32 id = 0;
> >>
> >> There is no need to initialize 'id = 0'. id will be assigned later.
> >
> >
> > if (!attr->query.prog_cnt || !prog_ids || !prog_cnt) is met, the id will not be
> assigned later.
> > At the end of the function, we judge whether to copy the program ID by the
> value of the id.
> 
> Ya, that is true. id = 0 is indeed needed.
> 
> >
> >
> >>
> >>> +	int ret;
> >>> +
> >>> +	if (attr->query.query_flags)
> >>> +		return -EINVAL;
> >>> +
> >>> +	f = fdget(ufd);
> >>> +	map = __bpf_map_get(f);
> >>> +	if (IS_ERR(map))
> >>> +		return PTR_ERR(map);
> >>> +
> >>> +	rcu_read_lock();
> >>> +
> >>> +	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> >>> +	if (ret)
> >>> +		goto end;
> >>> +
> >>> +	prog = *pprog;
> >>> +	prog_cnt = (!prog) ? 0 : 1;
> >>
> >> (!prog) => !prog ?
> >
> > Yes, It's just my habit
> >
> >>
> >>> +
> >>> +	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> >>> +		goto end;
> >>> +
> >>> +	id = prog->aux->id;
> >>> +	if (id == 0)
> >>> +		prog_cnt = 0;
> >>
> >>
> >> id will never be 0, see function bpf_prog_alloc_id() is syscall.c.
> >> So 'if (id == 0)' check is not needed.
> >
> >
> > Because we do not hold the reference count, the program may be released
> synchronously
> 
>    synchronously => asynchronously
> 
> > and its ID will be set to 0 in bpf_prog_free_id().
> >
> > Is that right?
> 
> Just checked the code again. You are right. Maybe add a comment here to
> make it clear why we check id == 0 here?


Yes, I'll add a comment


> >
> >
> >>
> >>> +
> >>> +end:
> >>> +	rcu_read_unlock();
> >>> +
> >>> +	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
> >>> +	    (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
> >>> +	    copy_to_user(&uattr->query.prog_cnt, &prog_cnt,
> sizeof(prog_cnt)))
> >>> +		ret = -EFAULT;
> >>> +
> >>> +	fdput(f);
> >>> +	return ret;
> >>> +}
> >>> +
> >>>    static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
> >>>    {
> >>>    	switch (link->map->map_type) {
> >>>

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
  2021-11-04  6:07 zhudi (E)
@ 2021-11-04  6:30 ` Yonghong Song
  0 siblings, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2021-11-04  6:30 UTC (permalink / raw)
  To: zhudi (E),
	davem, ast, daniel, andrii, kafai, songliubraving,
	john.fastabend, kpsingh, jakub
  Cc: bpf, linux-kernel



On 11/3/21 11:07 PM, zhudi (E) wrote:
>   > On 11/3/21 6:07 PM, Di Zhu wrote:
>>> Right now there is no way to query whether BPF programs are
>>> attached to a sockmap or not.
>>>
>>> we can use the standard interface in libbpf to query, such as:
>>> bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
>>> the mapFd is the fd of sockmap.
>>>
>>> Signed-off-by: Di Zhu <zhudi2@huawei.com>
>>> ---
>>> /* v2 */
>>> - John Fastabend <john.fastabend@gmail.com>
>>>     - add selftest code
>>>
>>> /* v3 */
>>>    - avoid sleeping caused by copy_to_user() in rcu critical zone
>>>
>>> /* v4 */
>>>    - Alexei Starovoitov <alexei.starovoitov@gmail.com>
>>>     -split into two patches, one for core code and one for selftest.
>>>
>>> /* v5 */
>>>    - Yonghong Song <yhs@fb.com>
>>>     -Some naming and formatting changes
>>> ---
>>>    include/linux/bpf.h  |  9 ++++++
>>>    kernel/bpf/syscall.c |  5 +++
>>>    net/core/sock_map.c  | 74
>> +++++++++++++++++++++++++++++++++++++++-----
>>>    3 files changed, 81 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>> index d604c8251d88..235ea7fc5fd8 100644
>>> --- a/include/linux/bpf.h
>>> +++ b/include/linux/bpf.h
>>> @@ -1961,6 +1961,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog
>> *prog,
>>>    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);
>>>    int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
>> *value, u64 flags);
>>> +int sock_map_bpf_prog_query(const union bpf_attr *attr,
>>> +			    union bpf_attr __user *uattr);
>>> +
>>>    void sock_map_unhash(struct sock *sk);
>>>    void sock_map_close(struct sock *sk, long timeout);
>>>    #else
>>> @@ -2014,6 +2017,12 @@ static inline int sock_map_update_elem_sys(struct
>> bpf_map *map, void *key, void
>>>    {
>>>    	return -EOPNOTSUPP;
>>>    }
>>> +
>>> +static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
>>> +					  union bpf_attr __user *uattr)
>>> +{
>>> +	return -EINVAL;
>>> +}
>>>    #endif /* CONFIG_BPF_SYSCALL */
>>>    #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
>>>
>>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>>> index 4e50c0bfdb7d..748102c3e0c9 100644
>>> --- a/kernel/bpf/syscall.c
>>> +++ b/kernel/bpf/syscall.c
>>> @@ -3275,6 +3275,11 @@ static int bpf_prog_query(const union bpf_attr
>> *attr,
>>>    	case BPF_FLOW_DISSECTOR:
>>>    	case BPF_SK_LOOKUP:
>>>    		return netns_bpf_prog_query(attr, uattr);
>>> +	case BPF_SK_SKB_STREAM_PARSER:
>>> +	case BPF_SK_SKB_STREAM_VERDICT:
>>> +	case BPF_SK_MSG_VERDICT:
>>> +	case BPF_SK_SKB_VERDICT:
>>> +		return sock_map_bpf_prog_query(attr, uattr);
>>>    	default:
>>>    		return -EINVAL;
>>>    	}
>>> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
>>> index e252b8ec2b85..0320d27550fe 100644
>>> --- a/net/core/sock_map.c
>>> +++ b/net/core/sock_map.c
>>> @@ -1412,38 +1412,50 @@ static struct sk_psock_progs
>> *sock_map_progs(struct bpf_map *map)
>>>    	return NULL;
>>>    }
>>>
>>> -static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog
>> *prog,
>>> -				struct bpf_prog *old, u32 which)
>>> +static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog
>> ***pprog,
>>> +				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:
>>> -		pprog = &progs->msg_parser;
>>> +		*pprog = &progs->msg_parser;
>>>    		break;
>>>    #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
>>>    	case BPF_SK_SKB_STREAM_PARSER:
>>> -		pprog = &progs->stream_parser;
>>> +		*pprog = &progs->stream_parser;
>>>    		break;
>>>    #endif
>>>    	case BPF_SK_SKB_STREAM_VERDICT:
>>>    		if (progs->skb_verdict)
>>>    			return -EBUSY;
>>> -		pprog = &progs->stream_verdict;
>>> +		*pprog = &progs->stream_verdict;
>>>    		break;
>>>    	case BPF_SK_SKB_VERDICT:
>>>    		if (progs->stream_verdict)
>>>    			return -EBUSY;
>>> -		pprog = &progs->skb_verdict;
>>> +		*pprog = &progs->skb_verdict;
>>>    		break;
>>>    	default:
>>>    		return -EOPNOTSUPP;
>>>    	}
>>>
>>> +	return 0;
>>> +}
>>> +
>>> +static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog
>> *prog,
>>> +				struct bpf_prog *old, u32 which)
>>> +{
>>> +	struct bpf_prog **pprog;
>>> +	int ret;
>>> +
>>> +	ret = sock_map_prog_lookup(map, &pprog, which);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>    	if (old)
>>>    		return psock_replace_prog(pprog, prog, old);
>>>
>>> @@ -1451,6 +1463,54 @@ static int sock_map_prog_update(struct bpf_map
>> *map, struct bpf_prog *prog,
>>>    	return 0;
>>>    }
>>>
>>> +int sock_map_bpf_prog_query(const union bpf_attr *attr,
>>> +			    union bpf_attr __user *uattr)
>>> +{
>>> +	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
>>> +	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
>>> +	struct bpf_prog **pprog;
>>> +	struct bpf_prog *prog;
>>> +	struct bpf_map *map;
>>> +	struct fd f;
>>> +	u32 id = 0;
>>
>> There is no need to initialize 'id = 0'. id will be assigned later.
> 
> 
> if (!attr->query.prog_cnt || !prog_ids || !prog_cnt) is met, the id will not be assigned later.
> At the end of the function, we judge whether to copy the program ID by the value of the id.

Ya, that is true. id = 0 is indeed needed.

> 
> 
>>
>>> +	int ret;
>>> +
>>> +	if (attr->query.query_flags)
>>> +		return -EINVAL;
>>> +
>>> +	f = fdget(ufd);
>>> +	map = __bpf_map_get(f);
>>> +	if (IS_ERR(map))
>>> +		return PTR_ERR(map);
>>> +
>>> +	rcu_read_lock();
>>> +
>>> +	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
>>> +	if (ret)
>>> +		goto end;
>>> +
>>> +	prog = *pprog;
>>> +	prog_cnt = (!prog) ? 0 : 1;
>>
>> (!prog) => !prog ?
> 
> Yes, It's just my habit
> 
>>
>>> +
>>> +	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
>>> +		goto end;
>>> +
>>> +	id = prog->aux->id;
>>> +	if (id == 0)
>>> +		prog_cnt = 0;
>>
>>
>> id will never be 0, see function bpf_prog_alloc_id() is syscall.c.
>> So 'if (id == 0)' check is not needed.
> 
> 
> Because we do not hold the reference count, the program may be released synchronously

   synchronously => asynchronously

> and its ID will be set to 0 in bpf_prog_free_id().
> 
> Is that right?

Just checked the code again. You are right. Maybe add a comment here to
make it clear why we check id == 0 here?

> 
> 
>>
>>> +
>>> +end:
>>> +	rcu_read_unlock();
>>> +
>>> +	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
>>> +	    (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
>>> +	    copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
>>> +		ret = -EFAULT;
>>> +
>>> +	fdput(f);
>>> +	return ret;
>>> +}
>>> +
>>>    static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
>>>    {
>>>    	switch (link->map->map_type) {
>>>

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
@ 2021-11-04  6:07 zhudi (E)
  2021-11-04  6:30 ` Yonghong Song
  0 siblings, 1 reply; 18+ messages in thread
From: zhudi (E) @ 2021-11-04  6:07 UTC (permalink / raw)
  To: Yonghong Song, davem, ast, daniel, andrii, kafai, songliubraving,
	john.fastabend, kpsingh, jakub
  Cc: bpf, linux-kernel

 > On 11/3/21 6:07 PM, Di Zhu wrote:
> > Right now there is no way to query whether BPF programs are
> > attached to a sockmap or not.
> >
> > we can use the standard interface in libbpf to query, such as:
> > bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
> > the mapFd is the fd of sockmap.
> >
> > Signed-off-by: Di Zhu <zhudi2@huawei.com>
> > ---
> > /* v2 */
> > - John Fastabend <john.fastabend@gmail.com>
> >    - add selftest code
> >
> > /* v3 */
> >   - avoid sleeping caused by copy_to_user() in rcu critical zone
> >
> > /* v4 */
> >   - Alexei Starovoitov <alexei.starovoitov@gmail.com>
> >    -split into two patches, one for core code and one for selftest.
> >
> > /* v5 */
> >   - Yonghong Song <yhs@fb.com>
> >    -Some naming and formatting changes
> > ---
> >   include/linux/bpf.h  |  9 ++++++
> >   kernel/bpf/syscall.c |  5 +++
> >   net/core/sock_map.c  | 74
> +++++++++++++++++++++++++++++++++++++++-----
> >   3 files changed, 81 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index d604c8251d88..235ea7fc5fd8 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -1961,6 +1961,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog
> *prog,
> >   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);
> >   int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
> *value, u64 flags);
> > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > +			    union bpf_attr __user *uattr);
> > +
> >   void sock_map_unhash(struct sock *sk);
> >   void sock_map_close(struct sock *sk, long timeout);
> >   #else
> > @@ -2014,6 +2017,12 @@ static inline int sock_map_update_elem_sys(struct
> bpf_map *map, void *key, void
> >   {
> >   	return -EOPNOTSUPP;
> >   }
> > +
> > +static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > +					  union bpf_attr __user *uattr)
> > +{
> > +	return -EINVAL;
> > +}
> >   #endif /* CONFIG_BPF_SYSCALL */
> >   #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
> >
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 4e50c0bfdb7d..748102c3e0c9 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -3275,6 +3275,11 @@ static int bpf_prog_query(const union bpf_attr
> *attr,
> >   	case BPF_FLOW_DISSECTOR:
> >   	case BPF_SK_LOOKUP:
> >   		return netns_bpf_prog_query(attr, uattr);
> > +	case BPF_SK_SKB_STREAM_PARSER:
> > +	case BPF_SK_SKB_STREAM_VERDICT:
> > +	case BPF_SK_MSG_VERDICT:
> > +	case BPF_SK_SKB_VERDICT:
> > +		return sock_map_bpf_prog_query(attr, uattr);
> >   	default:
> >   		return -EINVAL;
> >   	}
> > diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> > index e252b8ec2b85..0320d27550fe 100644
> > --- a/net/core/sock_map.c
> > +++ b/net/core/sock_map.c
> > @@ -1412,38 +1412,50 @@ static struct sk_psock_progs
> *sock_map_progs(struct bpf_map *map)
> >   	return NULL;
> >   }
> >
> > -static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog
> *prog,
> > -				struct bpf_prog *old, u32 which)
> > +static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog
> ***pprog,
> > +				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:
> > -		pprog = &progs->msg_parser;
> > +		*pprog = &progs->msg_parser;
> >   		break;
> >   #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
> >   	case BPF_SK_SKB_STREAM_PARSER:
> > -		pprog = &progs->stream_parser;
> > +		*pprog = &progs->stream_parser;
> >   		break;
> >   #endif
> >   	case BPF_SK_SKB_STREAM_VERDICT:
> >   		if (progs->skb_verdict)
> >   			return -EBUSY;
> > -		pprog = &progs->stream_verdict;
> > +		*pprog = &progs->stream_verdict;
> >   		break;
> >   	case BPF_SK_SKB_VERDICT:
> >   		if (progs->stream_verdict)
> >   			return -EBUSY;
> > -		pprog = &progs->skb_verdict;
> > +		*pprog = &progs->skb_verdict;
> >   		break;
> >   	default:
> >   		return -EOPNOTSUPP;
> >   	}
> >
> > +	return 0;
> > +}
> > +
> > +static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog
> *prog,
> > +				struct bpf_prog *old, u32 which)
> > +{
> > +	struct bpf_prog **pprog;
> > +	int ret;
> > +
> > +	ret = sock_map_prog_lookup(map, &pprog, which);
> > +	if (ret)
> > +		return ret;
> > +
> >   	if (old)
> >   		return psock_replace_prog(pprog, prog, old);
> >
> > @@ -1451,6 +1463,54 @@ static int sock_map_prog_update(struct bpf_map
> *map, struct bpf_prog *prog,
> >   	return 0;
> >   }
> >
> > +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> > +			    union bpf_attr __user *uattr)
> > +{
> > +	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> > +	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> > +	struct bpf_prog **pprog;
> > +	struct bpf_prog *prog;
> > +	struct bpf_map *map;
> > +	struct fd f;
> > +	u32 id = 0;
> 
> There is no need to initialize 'id = 0'. id will be assigned later.


if (!attr->query.prog_cnt || !prog_ids || !prog_cnt) is met, the id will not be assigned later.
At the end of the function, we judge whether to copy the program ID by the value of the id. 


> 
> > +	int ret;
> > +
> > +	if (attr->query.query_flags)
> > +		return -EINVAL;
> > +
> > +	f = fdget(ufd);
> > +	map = __bpf_map_get(f);
> > +	if (IS_ERR(map))
> > +		return PTR_ERR(map);
> > +
> > +	rcu_read_lock();
> > +
> > +	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> > +	if (ret)
> > +		goto end;
> > +
> > +	prog = *pprog;
> > +	prog_cnt = (!prog) ? 0 : 1;
> 
> (!prog) => !prog ?

Yes, It's just my habit

> 
> > +
> > +	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> > +		goto end;
> > +
> > +	id = prog->aux->id;
> > +	if (id == 0)
> > +		prog_cnt = 0;
> 
> 
> id will never be 0, see function bpf_prog_alloc_id() is syscall.c.
> So 'if (id == 0)' check is not needed.


Because we do not hold the reference count, the program may be released synchronously 
and its ID will be set to 0 in bpf_prog_free_id().

Is that right?


> 
> > +
> > +end:
> > +	rcu_read_unlock();
> > +
> > +	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
> > +	    (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
> > +	    copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
> > +		ret = -EFAULT;
> > +
> > +	fdput(f);
> > +	return ret;
> > +}
> > +
> >   static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
> >   {
> >   	switch (link->map->map_type) {
> >

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

* Re: [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
  2021-11-04  1:07 Di Zhu
@ 2021-11-04  5:31 ` Yonghong Song
  2021-11-05 19:51 ` Cong Wang
  1 sibling, 0 replies; 18+ messages in thread
From: Yonghong Song @ 2021-11-04  5:31 UTC (permalink / raw)
  To: Di Zhu, davem, ast, daniel, andrii, kafai, songliubraving,
	john.fastabend, kpsingh, jakub
  Cc: bpf, linux-kernel



On 11/3/21 6:07 PM, Di Zhu wrote:
> Right now there is no way to query whether BPF programs are
> attached to a sockmap or not.
> 
> we can use the standard interface in libbpf to query, such as:
> bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
> the mapFd is the fd of sockmap.
> 
> Signed-off-by: Di Zhu <zhudi2@huawei.com>
> ---
> /* v2 */
> - John Fastabend <john.fastabend@gmail.com>
>    - add selftest code
> 
> /* v3 */
>   - avoid sleeping caused by copy_to_user() in rcu critical zone
> 
> /* v4 */
>   - Alexei Starovoitov <alexei.starovoitov@gmail.com>
>    -split into two patches, one for core code and one for selftest.
> 
> /* v5 */
>   - Yonghong Song <yhs@fb.com>
>    -Some naming and formatting changes
> ---
>   include/linux/bpf.h  |  9 ++++++
>   kernel/bpf/syscall.c |  5 +++
>   net/core/sock_map.c  | 74 +++++++++++++++++++++++++++++++++++++++-----
>   3 files changed, 81 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d604c8251d88..235ea7fc5fd8 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1961,6 +1961,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog,
>   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);
>   int sock_map_update_elem_sys(struct bpf_map *map, void *key, void *value, u64 flags);
> +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> +			    union bpf_attr __user *uattr);
> +
>   void sock_map_unhash(struct sock *sk);
>   void sock_map_close(struct sock *sk, long timeout);
>   #else
> @@ -2014,6 +2017,12 @@ static inline int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
>   {
>   	return -EOPNOTSUPP;
>   }
> +
> +static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
> +					  union bpf_attr __user *uattr)
> +{
> +	return -EINVAL;
> +}
>   #endif /* CONFIG_BPF_SYSCALL */
>   #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
>   
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 4e50c0bfdb7d..748102c3e0c9 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -3275,6 +3275,11 @@ static int bpf_prog_query(const union bpf_attr *attr,
>   	case BPF_FLOW_DISSECTOR:
>   	case BPF_SK_LOOKUP:
>   		return netns_bpf_prog_query(attr, uattr);
> +	case BPF_SK_SKB_STREAM_PARSER:
> +	case BPF_SK_SKB_STREAM_VERDICT:
> +	case BPF_SK_MSG_VERDICT:
> +	case BPF_SK_SKB_VERDICT:
> +		return sock_map_bpf_prog_query(attr, uattr);
>   	default:
>   		return -EINVAL;
>   	}
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index e252b8ec2b85..0320d27550fe 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -1412,38 +1412,50 @@ static struct sk_psock_progs *sock_map_progs(struct bpf_map *map)
>   	return NULL;
>   }
>   
> -static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
> -				struct bpf_prog *old, u32 which)
> +static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
> +				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:
> -		pprog = &progs->msg_parser;
> +		*pprog = &progs->msg_parser;
>   		break;
>   #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
>   	case BPF_SK_SKB_STREAM_PARSER:
> -		pprog = &progs->stream_parser;
> +		*pprog = &progs->stream_parser;
>   		break;
>   #endif
>   	case BPF_SK_SKB_STREAM_VERDICT:
>   		if (progs->skb_verdict)
>   			return -EBUSY;
> -		pprog = &progs->stream_verdict;
> +		*pprog = &progs->stream_verdict;
>   		break;
>   	case BPF_SK_SKB_VERDICT:
>   		if (progs->stream_verdict)
>   			return -EBUSY;
> -		pprog = &progs->skb_verdict;
> +		*pprog = &progs->skb_verdict;
>   		break;
>   	default:
>   		return -EOPNOTSUPP;
>   	}
>   
> +	return 0;
> +}
> +
> +static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
> +				struct bpf_prog *old, u32 which)
> +{
> +	struct bpf_prog **pprog;
> +	int ret;
> +
> +	ret = sock_map_prog_lookup(map, &pprog, which);
> +	if (ret)
> +		return ret;
> +
>   	if (old)
>   		return psock_replace_prog(pprog, prog, old);
>   
> @@ -1451,6 +1463,54 @@ static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
>   	return 0;
>   }
>   
> +int sock_map_bpf_prog_query(const union bpf_attr *attr,
> +			    union bpf_attr __user *uattr)
> +{
> +	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
> +	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
> +	struct bpf_prog **pprog;
> +	struct bpf_prog *prog;
> +	struct bpf_map *map;
> +	struct fd f;
> +	u32 id = 0;

There is no need to initialize 'id = 0'. id will be assigned later.

> +	int ret;
> +
> +	if (attr->query.query_flags)
> +		return -EINVAL;
> +
> +	f = fdget(ufd);
> +	map = __bpf_map_get(f);
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	rcu_read_lock();
> +
> +	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
> +	if (ret)
> +		goto end;
> +
> +	prog = *pprog;
> +	prog_cnt = (!prog) ? 0 : 1;

(!prog) => !prog ?

> +
> +	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
> +		goto end;
> +
> +	id = prog->aux->id;
> +	if (id == 0)
> +		prog_cnt = 0;


id will never be 0, see function bpf_prog_alloc_id() is syscall.c.
So 'if (id == 0)' check is not needed.

> +
> +end:
> +	rcu_read_unlock();
> +
> +	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
> +	    (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
> +	    copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
> +		ret = -EFAULT;
> +
> +	fdput(f);
> +	return ret;
> +}
> +
>   static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
>   {
>   	switch (link->map->map_type) {
> 

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

* [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap
@ 2021-11-04  1:07 Di Zhu
  2021-11-04  5:31 ` Yonghong Song
  2021-11-05 19:51 ` Cong Wang
  0 siblings, 2 replies; 18+ messages in thread
From: Di Zhu @ 2021-11-04  1:07 UTC (permalink / raw)
  To: davem, ast, daniel, andrii, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, jakub
  Cc: bpf, linux-kernel, zhudi2

Right now there is no way to query whether BPF programs are
attached to a sockmap or not.

we can use the standard interface in libbpf to query, such as:
bpf_prog_query(mapFd, BPF_SK_SKB_STREAM_PARSER, 0, NULL, ...);
the mapFd is the fd of sockmap.

Signed-off-by: Di Zhu <zhudi2@huawei.com>
---
/* v2 */
- John Fastabend <john.fastabend@gmail.com>
  - add selftest code

/* v3 */
 - avoid sleeping caused by copy_to_user() in rcu critical zone

/* v4 */
 - Alexei Starovoitov <alexei.starovoitov@gmail.com>
  -split into two patches, one for core code and one for selftest.

/* v5 */
 - Yonghong Song <yhs@fb.com>
  -Some naming and formatting changes
---
 include/linux/bpf.h  |  9 ++++++
 kernel/bpf/syscall.c |  5 +++
 net/core/sock_map.c  | 74 +++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 81 insertions(+), 7 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d604c8251d88..235ea7fc5fd8 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1961,6 +1961,9 @@ int bpf_prog_test_run_syscall(struct bpf_prog *prog,
 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);
 int sock_map_update_elem_sys(struct bpf_map *map, void *key, void *value, u64 flags);
+int sock_map_bpf_prog_query(const union bpf_attr *attr,
+			    union bpf_attr __user *uattr);
+
 void sock_map_unhash(struct sock *sk);
 void sock_map_close(struct sock *sk, long timeout);
 #else
@@ -2014,6 +2017,12 @@ static inline int sock_map_update_elem_sys(struct bpf_map *map, void *key, void
 {
 	return -EOPNOTSUPP;
 }
+
+static inline int sock_map_bpf_prog_query(const union bpf_attr *attr,
+					  union bpf_attr __user *uattr)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_BPF_SYSCALL */
 #endif /* CONFIG_NET && CONFIG_BPF_SYSCALL */
 
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4e50c0bfdb7d..748102c3e0c9 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -3275,6 +3275,11 @@ static int bpf_prog_query(const union bpf_attr *attr,
 	case BPF_FLOW_DISSECTOR:
 	case BPF_SK_LOOKUP:
 		return netns_bpf_prog_query(attr, uattr);
+	case BPF_SK_SKB_STREAM_PARSER:
+	case BPF_SK_SKB_STREAM_VERDICT:
+	case BPF_SK_MSG_VERDICT:
+	case BPF_SK_SKB_VERDICT:
+		return sock_map_bpf_prog_query(attr, uattr);
 	default:
 		return -EINVAL;
 	}
diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index e252b8ec2b85..0320d27550fe 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -1412,38 +1412,50 @@ static struct sk_psock_progs *sock_map_progs(struct bpf_map *map)
 	return NULL;
 }
 
-static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
-				struct bpf_prog *old, u32 which)
+static int sock_map_prog_lookup(struct bpf_map *map, struct bpf_prog ***pprog,
+				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:
-		pprog = &progs->msg_parser;
+		*pprog = &progs->msg_parser;
 		break;
 #if IS_ENABLED(CONFIG_BPF_STREAM_PARSER)
 	case BPF_SK_SKB_STREAM_PARSER:
-		pprog = &progs->stream_parser;
+		*pprog = &progs->stream_parser;
 		break;
 #endif
 	case BPF_SK_SKB_STREAM_VERDICT:
 		if (progs->skb_verdict)
 			return -EBUSY;
-		pprog = &progs->stream_verdict;
+		*pprog = &progs->stream_verdict;
 		break;
 	case BPF_SK_SKB_VERDICT:
 		if (progs->stream_verdict)
 			return -EBUSY;
-		pprog = &progs->skb_verdict;
+		*pprog = &progs->skb_verdict;
 		break;
 	default:
 		return -EOPNOTSUPP;
 	}
 
+	return 0;
+}
+
+static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
+				struct bpf_prog *old, u32 which)
+{
+	struct bpf_prog **pprog;
+	int ret;
+
+	ret = sock_map_prog_lookup(map, &pprog, which);
+	if (ret)
+		return ret;
+
 	if (old)
 		return psock_replace_prog(pprog, prog, old);
 
@@ -1451,6 +1463,54 @@ static int sock_map_prog_update(struct bpf_map *map, struct bpf_prog *prog,
 	return 0;
 }
 
+int sock_map_bpf_prog_query(const union bpf_attr *attr,
+			    union bpf_attr __user *uattr)
+{
+	__u32 __user *prog_ids = u64_to_user_ptr(attr->query.prog_ids);
+	u32 prog_cnt = 0, flags = 0, ufd = attr->target_fd;
+	struct bpf_prog **pprog;
+	struct bpf_prog *prog;
+	struct bpf_map *map;
+	struct fd f;
+	u32 id = 0;
+	int ret;
+
+	if (attr->query.query_flags)
+		return -EINVAL;
+
+	f = fdget(ufd);
+	map = __bpf_map_get(f);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	rcu_read_lock();
+
+	ret = sock_map_prog_lookup(map, &pprog, attr->query.attach_type);
+	if (ret)
+		goto end;
+
+	prog = *pprog;
+	prog_cnt = (!prog) ? 0 : 1;
+
+	if (!attr->query.prog_cnt || !prog_ids || !prog_cnt)
+		goto end;
+
+	id = prog->aux->id;
+	if (id == 0)
+		prog_cnt = 0;
+
+end:
+	rcu_read_unlock();
+
+	if (copy_to_user(&uattr->query.attach_flags, &flags, sizeof(flags)) ||
+	    (id != 0 && copy_to_user(prog_ids, &id, sizeof(u32))) ||
+	    copy_to_user(&uattr->query.prog_cnt, &prog_cnt, sizeof(prog_cnt)))
+		ret = -EFAULT;
+
+	fdput(f);
+	return ret;
+}
+
 static void sock_map_unlink(struct sock *sk, struct sk_psock_link *link)
 {
 	switch (link->map->map_type) {
-- 
2.27.0


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

end of thread, other threads:[~2022-01-15 19:10 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-13  9:00 [PATCH bpf-next v5 1/2] bpf: support BPF_PROG_QUERY for progs attached to sockmap Di Zhu
2022-01-13  9:00 ` [PATCH bpf-next v5 2/2] selftests: bpf: test " Di Zhu
2022-01-15  1:18   ` Andrii Nakryiko
2022-01-13 16:15 ` [PATCH bpf-next v5 1/2] bpf: support " Jakub Sitnicki
2022-01-15  1:22   ` Andrii Nakryiko
  -- strict thread matches above, loose matches on Subject: below --
2022-01-15  2:38 zhudi (E)
2022-01-15  2:53 ` Andrii Nakryiko
2022-01-15 19:09   ` Jakub Sitnicki
2022-01-14  5:44 zhudi (E)
2021-11-08  2:13 zhudi (E)
2021-11-05  1:57 Di Zhu
2021-11-05  4:23 ` Yonghong Song
2021-11-04  6:35 zhudi (E)
2021-11-04  6:07 zhudi (E)
2021-11-04  6:30 ` Yonghong Song
2021-11-04  1:07 Di Zhu
2021-11-04  5:31 ` Yonghong Song
2021-11-05 19:51 ` Cong Wang

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