[PATCH v2 1/5] net: Remove the err argument from sock_from_file

[PATCH v2 1/5] net: Remove the err argument from sock_from_file

[Florent Revest]

From: Florent Revest <revest@google.com>

Currently, the sock_from_file prototype takes an "err" pointer that is
either not set or set to -ENOTSOCK IFF the returned socket is NULL. This
makes the error redundant and it is ignored by a few callers.

This patch simplifies the API by letting callers deduce the error based
on whether the returned socket is NULL or not.

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Florent Revest <revest@google.com>
---
 fs/eventpoll.c               |  3 +--
 fs/io_uring.c                | 16 ++++++++--------
 include/linux/net.h          |  2 +-
 net/core/netclassid_cgroup.c |  3 +--
 net/core/netprio_cgroup.c    |  3 +--
 net/core/sock.c              |  8 +-------
 net/socket.c                 | 27 ++++++++++++++++-----------
 7 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 4df61129566d..c764d8d5a76a 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -415,12 +415,11 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
 	unsigned int napi_id;
 	struct socket *sock;
 	struct sock *sk;
-	int err;
 
 	if (!net_busy_loop_on())
 		return;
 
-	sock = sock_from_file(epi->ffd.file, &err);
+	sock = sock_from_file(epi->ffd.file);
 	if (!sock)
 		return;
 
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 8018c7076b25..ace99b15cbd3 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4341,9 +4341,9 @@ static int io_sendmsg(struct io_kiocb *req, bool force_nonblock,
 	unsigned flags;
 	int ret;
 
-	sock = sock_from_file(req->file, &ret);
+	sock = sock_from_file(req->file);
 	if (unlikely(!sock))
-		return ret;
+		return -ENOTSOCK;
 
 	if (req->async_data) {
 		kmsg = req->async_data;
@@ -4390,9 +4390,9 @@ static int io_send(struct io_kiocb *req, bool force_nonblock,
 	unsigned flags;
 	int ret;
 
-	sock = sock_from_file(req->file, &ret);
+	sock = sock_from_file(req->file);
 	if (unlikely(!sock))
-		return ret;
+		return -ENOTSOCK;
 
 	ret = import_single_range(WRITE, sr->buf, sr->len, &iov, &msg.msg_iter);
 	if (unlikely(ret))
@@ -4569,9 +4569,9 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock,
 	unsigned flags;
 	int ret, cflags = 0;
 
-	sock = sock_from_file(req->file, &ret);
+	sock = sock_from_file(req->file);
 	if (unlikely(!sock))
-		return ret;
+		return -ENOTSOCK;
 
 	if (req->async_data) {
 		kmsg = req->async_data;
@@ -4632,9 +4632,9 @@ static int io_recv(struct io_kiocb *req, bool force_nonblock,
 	unsigned flags;
 	int ret, cflags = 0;
 
-	sock = sock_from_file(req->file, &ret);
+	sock = sock_from_file(req->file);
 	if (unlikely(!sock))
-		return ret;
+		return -ENOTSOCK;
 
 	if (req->flags & REQ_F_BUFFER_SELECT) {
 		kbuf = io_recv_buffer_select(req, !force_nonblock);
diff --git a/include/linux/net.h b/include/linux/net.h
index 0dcd51feef02..9e2324efc26a 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -240,7 +240,7 @@ int sock_sendmsg(struct socket *sock, struct msghdr *msg);
 int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
 struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
 struct socket *sockfd_lookup(int fd, int *err);
-struct socket *sock_from_file(struct file *file, int *err);
+struct socket *sock_from_file(struct file *file);
 #define		     sockfd_put(sock) fput(sock->file)
 int net_ratelimit(void);
 
diff --git a/net/core/netclassid_cgroup.c b/net/core/netclassid_cgroup.c
index 41b24cd31562..b49c57d35a88 100644
--- a/net/core/netclassid_cgroup.c
+++ b/net/core/netclassid_cgroup.c
@@ -68,9 +68,8 @@ struct update_classid_context {
 
 static int update_classid_sock(const void *v, struct file *file, unsigned n)
 {
-	int err;
 	struct update_classid_context *ctx = (void *)v;
-	struct socket *sock = sock_from_file(file, &err);
+	struct socket *sock = sock_from_file(file);
 
 	if (sock) {
 		spin_lock(&cgroup_sk_update_lock);
diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index 9bd4cab7d510..99a431c56f23 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -220,8 +220,7 @@ static ssize_t write_priomap(struct kernfs_open_file *of,
 
 static int update_netprio(const void *v, struct file *file, unsigned n)
 {
-	int err;
-	struct socket *sock = sock_from_file(file, &err);
+	struct socket *sock = sock_from_file(file);
 	if (sock) {
 		spin_lock(&cgroup_sk_update_lock);
 		sock_cgroup_set_prioidx(&sock->sk->sk_cgrp_data,
diff --git a/net/core/sock.c b/net/core/sock.c
index 727ea1cc633c..dd0598d831ef 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2808,14 +2808,8 @@ EXPORT_SYMBOL(sock_no_mmap);
 void __receive_sock(struct file *file)
 {
 	struct socket *sock;
-	int error;
 
-	/*
-	 * The resulting value of "error" is ignored here since we only
-	 * need to take action when the file is a socket and testing
-	 * "sock" for NULL is sufficient.
-	 */
-	sock = sock_from_file(file, &error);
+	sock = sock_from_file(file);
 	if (sock) {
 		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
 		sock_update_classid(&sock->sk->sk_cgrp_data);
diff --git a/net/socket.c b/net/socket.c
index 6e6cccc2104f..c799d9652a2c 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -445,17 +445,15 @@ static int sock_map_fd(struct socket *sock, int flags)
 /**
  *	sock_from_file - Return the &socket bounded to @file.
  *	@file: file
- *	@err: pointer to an error code return
  *
- *	On failure returns %NULL and assigns -ENOTSOCK to @err.
+ *	On failure returns %NULL.
  */
 
-struct socket *sock_from_file(struct file *file, int *err)
+struct socket *sock_from_file(struct file *file)
 {
 	if (file->f_op == &socket_file_ops)
 		return file->private_data;	/* set in sock_map_fd */
 
-	*err = -ENOTSOCK;
 	return NULL;
 }
 EXPORT_SYMBOL(sock_from_file);
@@ -484,9 +482,11 @@ struct socket *sockfd_lookup(int fd, int *err)
 		return NULL;
 	}
 
-	sock = sock_from_file(file, err);
-	if (!sock)
+	sock = sock_from_file(file);
+	if (!sock) {
+		*err = -ENOTSOCK;
 		fput(file);
+	}
 	return sock;
 }
 EXPORT_SYMBOL(sockfd_lookup);
@@ -498,11 +498,12 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
 
 	*err = -EBADF;
 	if (f.file) {
-		sock = sock_from_file(f.file, err);
+		sock = sock_from_file(f.file);
 		if (likely(sock)) {
 			*fput_needed = f.flags & FDPUT_FPUT;
 			return sock;
 		}
+		*err = -ENOTSOCK;
 		fdput(f);
 	}
 	return NULL;
@@ -1715,9 +1716,11 @@ int __sys_accept4_file(struct file *file, unsigned file_flags,
 	if (SOCK_NONBLOCK != O_NONBLOCK && (flags & SOCK_NONBLOCK))
 		flags = (flags & ~SOCK_NONBLOCK) | O_NONBLOCK;
 
-	sock = sock_from_file(file, &err);
-	if (!sock)
+	sock = sock_from_file(file);
+	if (!sock) {
+		err = -ENOTSOCK;
 		goto out;
+	}
 
 	err = -ENFILE;
 	newsock = sock_alloc();
@@ -1840,9 +1843,11 @@ int __sys_connect_file(struct file *file, struct sockaddr_storage *address,
 	struct socket *sock;
 	int err;
 
-	sock = sock_from_file(file, &err);
-	if (!sock)
+	sock = sock_from_file(file);
+	if (!sock) {
+		err = -ENOTSOCK;
 		goto out;
+	}
 
 	err =
 	    security_socket_connect(sock, (struct sockaddr *)address, addrlen);
-- 
2.29.2.299.gdc1121823c-goog

[PATCH v2 2/5] bpf: Add a bpf_sock_from_file helper

[Florent Revest]

From: Florent Revest <revest@google.com>

While eBPF programs can check whether a file is a socket by file->f_op
== &socket_file_ops, they cannot convert the void private_data pointer
to a struct socket BTF pointer. In order to do this a new helper
wrapping sock_from_file is added.

This is useful to tracing programs but also other program types
inheriting this set of helpers such as iterators or LSM programs.

Signed-off-by: Florent Revest <revest@google.com>
---
 include/uapi/linux/bpf.h       |  7 +++++++
 kernel/trace/bpf_trace.c       | 20 ++++++++++++++++++++
 scripts/bpf_helpers_doc.py     |  4 ++++
 tools/include/uapi/linux/bpf.h |  7 +++++++
 4 files changed, 38 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 162999b12790..7d598f161dc0 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3787,6 +3787,12 @@ union bpf_attr {
  *		*ARG_PTR_TO_BTF_ID* of type *task_struct*.
  *	Return
  *		Pointer to the current task.
+ *
+ * struct socket *bpf_sock_from_file(struct file *file)
+ *	Description
+ *		If the given file contains a socket, returns the associated socket.
+ *	Return
+ *		A pointer to a struct socket on success or NULL on failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3948,6 +3954,7 @@ union bpf_attr {
 	FN(task_storage_get),		\
 	FN(task_storage_delete),	\
 	FN(get_current_task_btf),	\
+	FN(sock_from_file),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 02986c7b90eb..d87ca6f93c58 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1260,6 +1260,24 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = {
 	.arg5_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_1(bpf_sock_from_file, struct file *, file)
+{
+	return (unsigned long) sock_from_file(file);
+}
+
+BTF_ID_LIST(bpf_sock_from_file_btf_ids)
+BTF_ID(struct, socket)
+BTF_ID(struct, file)
+
+static const struct bpf_func_proto bpf_sock_from_file_proto = {
+	.func		= bpf_sock_from_file,
+	.gpl_only	= false,
+	.ret_type	= RET_PTR_TO_BTF_ID_OR_NULL,
+	.ret_btf_id	= &bpf_sock_from_file_btf_ids[0],
+	.arg1_type	= ARG_PTR_TO_BTF_ID,
+	.arg1_btf_id	= &bpf_sock_from_file_btf_ids[1],
+};
+
 const struct bpf_func_proto *
 bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -1354,6 +1372,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_per_cpu_ptr_proto;
 	case BPF_FUNC_bpf_this_cpu_ptr:
 		return &bpf_this_cpu_ptr_proto;
+	case BPF_FUNC_sock_from_file:
+		return &bpf_sock_from_file_proto;
 	default:
 		return NULL;
 	}
diff --git a/scripts/bpf_helpers_doc.py b/scripts/bpf_helpers_doc.py
index 31484377b8b1..d609f20e8360 100755
--- a/scripts/bpf_helpers_doc.py
+++ b/scripts/bpf_helpers_doc.py
@@ -435,6 +435,8 @@ class PrinterHelpers(Printer):
             'struct xdp_md',
             'struct path',
             'struct btf_ptr',
+            'struct socket',
+            'struct file',
     ]
     known_types = {
             '...',
@@ -478,6 +480,8 @@ class PrinterHelpers(Printer):
             'struct task_struct',
             'struct path',
             'struct btf_ptr',
+            'struct socket',
+            'struct file',
     }
     mapped_types = {
             'u8': '__u8',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 162999b12790..7d598f161dc0 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3787,6 +3787,12 @@ union bpf_attr {
  *		*ARG_PTR_TO_BTF_ID* of type *task_struct*.
  *	Return
  *		Pointer to the current task.
+ *
+ * struct socket *bpf_sock_from_file(struct file *file)
+ *	Description
+ *		If the given file contains a socket, returns the associated socket.
+ *	Return
+ *		A pointer to a struct socket on success or NULL on failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -3948,6 +3954,7 @@ union bpf_attr {
 	FN(task_storage_get),		\
 	FN(task_storage_delete),	\
 	FN(get_current_task_btf),	\
+	FN(sock_from_file),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.29.2.299.gdc1121823c-goog

[PATCH v2 3/5] bpf: Expose bpf_sk_storage_* to iterator programs

[Florent Revest]

From: Florent Revest <revest@google.com>

Iterators are currently used to expose kernel information to userspace
over fast procfs-like files but iterators could also be used to
manipulate local storage. For example, the task_file iterator could be
used to initialize a socket local storage with associations between
processes and sockets or to selectively delete local storage values.

This exposes both socket local storage helpers to all iterators.
Alternatively we could expose it to only certain iterators with strcmps
on prog->aux->attach_func_name.

Signed-off-by: Florent Revest <revest@google.com>
---
 net/core/bpf_sk_storage.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index a32037daa933..4edd033e899c 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -394,6 +394,7 @@ static bool bpf_sk_storage_tracing_allowed(const struct bpf_prog *prog)
 	 * use the bpf_sk_storage_(get|delete) helper.
 	 */
 	switch (prog->expected_attach_type) {
+	case BPF_TRACE_ITER:
 	case BPF_TRACE_RAW_TP:
 		/* bpf_sk_storage has no trace point */
 		return true;
-- 
2.29.2.299.gdc1121823c-goog

[PATCH v2 4/5] bpf: Add an iterator selftest for bpf_sk_storage_delete

[Florent Revest]

From: Florent Revest <revest@google.com>

The eBPF program iterates over all entries (well, only one) of a socket
local storage map and deletes them all. The test makes sure that the
entry is indeed deleted.

Signed-off-by: Florent Revest <revest@google.com>
---
 .../selftests/bpf/prog_tests/bpf_iter.c       | 64 +++++++++++++++++++
 .../progs/bpf_iter_bpf_sk_storage_helpers.c   | 23 +++++++
 2 files changed, 87 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 448885b95eed..bb4a638f2e6f 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -20,6 +20,7 @@
 #include "bpf_iter_bpf_percpu_hash_map.skel.h"
 #include "bpf_iter_bpf_array_map.skel.h"
 #include "bpf_iter_bpf_percpu_array_map.skel.h"
+#include "bpf_iter_bpf_sk_storage_helpers.skel.h"
 #include "bpf_iter_bpf_sk_storage_map.skel.h"
 #include "bpf_iter_test_kern5.skel.h"
 #include "bpf_iter_test_kern6.skel.h"
@@ -913,6 +914,67 @@ static void test_bpf_percpu_array_map(void)
 	bpf_iter_bpf_percpu_array_map__destroy(skel);
 }
 
+/* An iterator program deletes all local storage in a map. */
+static void test_bpf_sk_storage_delete(void)
+{
+	DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
+	struct bpf_iter_bpf_sk_storage_helpers *skel;
+	union bpf_iter_link_info linfo;
+	int err, len, map_fd, iter_fd;
+	struct bpf_link *link;
+	int sock_fd = -1;
+	__u32 val = 42;
+	char buf[64];
+
+	skel = bpf_iter_bpf_sk_storage_helpers__open_and_load();
+	if (CHECK(!skel, "bpf_iter_bpf_sk_storage_helpers__open_and_load",
+		  "skeleton open_and_load failed\n"))
+		return;
+
+	map_fd = bpf_map__fd(skel->maps.sk_stg_map);
+
+	sock_fd = socket(AF_INET6, SOCK_STREAM, 0);
+	if (CHECK(sock_fd < 0, "socket", "errno: %d\n", errno))
+		goto out;
+	err = bpf_map_update_elem(map_fd, &sock_fd, &val, BPF_NOEXIST);
+	if (CHECK(err, "map_update", "map_update failed\n"))
+		goto out;
+
+	memset(&linfo, 0, sizeof(linfo));
+	linfo.map.map_fd = map_fd;
+	opts.link_info = &linfo;
+	opts.link_info_len = sizeof(linfo);
+	link = bpf_program__attach_iter(skel->progs.delete_bpf_sk_storage_map,
+					&opts);
+	if (CHECK(IS_ERR(link), "attach_iter", "attach_iter failed\n"))
+		goto out;
+
+	iter_fd = bpf_iter_create(bpf_link__fd(link));
+	if (CHECK(iter_fd < 0, "create_iter", "create_iter failed\n"))
+		goto free_link;
+
+	/* do some tests */
+	while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
+		;
+	if (CHECK(len < 0, "read", "read failed: %s\n", strerror(errno)))
+		goto close_iter;
+
+	/* test results */
+	err = bpf_map_lookup_elem(map_fd, &sock_fd, &val);
+	if (CHECK(!err || errno != ENOENT, "bpf_map_lookup_elem",
+		  "map value wasn't deleted (err=%d, errno=%d)\n", err, errno))
+		goto close_iter;
+
+close_iter:
+	close(iter_fd);
+free_link:
+	bpf_link__destroy(link);
+out:
+	if (sock_fd >= 0)
+		close(sock_fd);
+	bpf_iter_bpf_sk_storage_helpers__destroy(skel);
+}
+
 static void test_bpf_sk_storage_map(void)
 {
 	DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
@@ -1067,6 +1129,8 @@ void test_bpf_iter(void)
 		test_bpf_percpu_array_map();
 	if (test__start_subtest("bpf_sk_storage_map"))
 		test_bpf_sk_storage_map();
+	if (test__start_subtest("bpf_sk_storage_delete"))
+		test_bpf_sk_storage_delete();
 	if (test__start_subtest("rdonly-buf-out-of-bound"))
 		test_rdonly_buf_out_of_bound();
 	if (test__start_subtest("buf-neg-offset"))
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
new file mode 100644
index 000000000000..01ff3235e413
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020 Google LLC. */
+#include "bpf_iter.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SK_STORAGE);
+	__uint(map_flags, BPF_F_NO_PREALLOC);
+	__type(key, int);
+	__type(value, int);
+} sk_stg_map SEC(".maps");
+
+SEC("iter/bpf_sk_storage_map")
+int delete_bpf_sk_storage_map(struct bpf_iter__bpf_sk_storage_map *ctx)
+{
+	if (ctx->sk)
+		bpf_sk_storage_delete(&sk_stg_map, ctx->sk);
+
+	return 0;
+}
-- 
2.29.2.299.gdc1121823c-goog

[PATCH v2 5/5] bpf: Add an iterator selftest for bpf_sk_storage_get

[Florent Revest]

From: Florent Revest <revest@google.com>

The eBPF program iterates over all files and tasks. For all socket
files, it stores the tgid of the last task it encountered with a handle
to that socket. This is a heuristic for finding the "owner" of a socket
similar to what's done by lsof, ss, netstat or fuser. Potentially, this
information could be used from a cgroup_skb/*gress hook to try to
associate network traffic with processes.

The test makes sure that a socket it created is tagged with prog_tests's
pid.

Signed-off-by: Florent Revest <revest@google.com>
---
 .../selftests/bpf/prog_tests/bpf_iter.c       | 35 +++++++++++++++++++
 .../progs/bpf_iter_bpf_sk_storage_helpers.c   | 26 ++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index bb4a638f2e6f..4d0626003c03 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -975,6 +975,39 @@ static void test_bpf_sk_storage_delete(void)
 	bpf_iter_bpf_sk_storage_helpers__destroy(skel);
 }
 
+/* The BPF program stores in every socket the tgid of a task owning a handle to
+ * it. The test verifies that a locally-created socket is tagged with its pid
+ */
+static void test_bpf_sk_storage_get(void)
+{
+	struct bpf_iter_bpf_sk_storage_helpers *skel;
+	int err, map_fd, val = -1;
+	int sock_fd = -1;
+
+	skel = bpf_iter_bpf_sk_storage_helpers__open_and_load();
+	if (CHECK(!skel, "bpf_iter_bpf_sk_storage_helpers__open_and_load",
+		  "skeleton open_and_load failed\n"))
+		return;
+
+	sock_fd = socket(AF_INET6, SOCK_STREAM, 0);
+	if (CHECK(sock_fd < 0, "socket", "errno: %d\n", errno))
+		goto out;
+
+	do_dummy_read(skel->progs.fill_socket_owners);
+
+	map_fd = bpf_map__fd(skel->maps.sk_stg_map);
+
+	err = bpf_map_lookup_elem(map_fd, &sock_fd, &val);
+	CHECK(err || val != getpid(), "bpf_map_lookup_elem",
+	      "map value wasn't set correctly (expected %d, got %d, err=%d)\n",
+	      getpid(), val, err);
+
+	if (sock_fd >= 0)
+		close(sock_fd);
+out:
+	bpf_iter_bpf_sk_storage_helpers__destroy(skel);
+}
+
 static void test_bpf_sk_storage_map(void)
 {
 	DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
@@ -1131,6 +1164,8 @@ void test_bpf_iter(void)
 		test_bpf_sk_storage_map();
 	if (test__start_subtest("bpf_sk_storage_delete"))
 		test_bpf_sk_storage_delete();
+	if (test__start_subtest("bpf_sk_storage_get"))
+		test_bpf_sk_storage_get();
 	if (test__start_subtest("rdonly-buf-out-of-bound"))
 		test_rdonly_buf_out_of_bound();
 	if (test__start_subtest("buf-neg-offset"))
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
index 01ff3235e413..7206fd6f09ab 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
@@ -21,3 +21,29 @@ int delete_bpf_sk_storage_map(struct bpf_iter__bpf_sk_storage_map *ctx)
 
 	return 0;
 }
+
+SEC("iter/task_file")
+int fill_socket_owners(struct bpf_iter__task_file *ctx)
+{
+	struct task_struct *task = ctx->task;
+	struct file *file = ctx->file;
+	struct socket *sock;
+	int *sock_tgid;
+
+	if (!task || !file || task->tgid != task->pid)
+		return 0;
+
+	sock = bpf_sock_from_file(file);
+	if (!sock)
+		return 0;
+
+	sock_tgid = bpf_sk_storage_get(&sk_stg_map, sock->sk, 0,
+				       BPF_SK_STORAGE_GET_F_CREATE);
+	if (!sock_tgid)
+		return 0;
+
+	*sock_tgid = task->tgid;
+
+	return 0;
+}
+
-- 
2.29.2.299.gdc1121823c-goog

Re: [PATCH v2 1/5] net: Remove the err argument from sock_from_file

[KP Singh]

I think you meant to send these as [PATCH bpf-next] for bpf-next.

I guess we can do a round of reviews and update the next revision (if
any) with the correct prefixes.

On Thu, Nov 19, 2020 at 5:27 PM Florent Revest <revest@chromium.org> wrote:
>
> From: Florent Revest <revest@google.com>
>
> Currently, the sock_from_file prototype takes an "err" pointer that is
> either not set or set to -ENOTSOCK IFF the returned socket is NULL. This
> makes the error redundant and it is ignored by a few callers.
>
> This patch simplifies the API by letting callers deduce the error based
> on whether the returned socket is NULL or not.
>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Florent Revest <revest@google.com>
> ---
>  fs/eventpoll.c               |  3 +--
>  fs/io_uring.c                | 16 ++++++++--------
>  include/linux/net.h          |  2 +-
>  net/core/netclassid_cgroup.c |  3 +--
>  net/core/netprio_cgroup.c    |  3 +--
>  net/core/sock.c              |  8 +-------
>  net/socket.c                 | 27 ++++++++++++++++-----------
>  7 files changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 4df61129566d..c764d8d5a76a 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -415,12 +415,11 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
>         unsigned int napi_id;
>         struct socket *sock;
>         struct sock *sk;
> -       int err;
>
>         if (!net_busy_loop_on())
>                 return;
>
> -       sock = sock_from_file(epi->ffd.file, &err);
> +       sock = sock_from_file(epi->ffd.file);
>         if (!sock)
>                 return;
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 8018c7076b25..ace99b15cbd3 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -4341,9 +4341,9 @@ static int io_sendmsg(struct io_kiocb *req, bool force_nonblock,
>         unsigned flags;
>         int ret;
>
> -       sock = sock_from_file(req->file, &ret);
> +       sock = sock_from_file(req->file);
>         if (unlikely(!sock))
> -               return ret;
> +               return -ENOTSOCK;
>
>         if (req->async_data) {
>                 kmsg = req->async_data;
> @@ -4390,9 +4390,9 @@ static int io_send(struct io_kiocb *req, bool force_nonblock,
>         unsigned flags;
>         int ret;
>
> -       sock = sock_from_file(req->file, &ret);
> +       sock = sock_from_file(req->file);
>         if (unlikely(!sock))
> -               return ret;
> +               return -ENOTSOCK;
>
>         ret = import_single_range(WRITE, sr->buf, sr->len, &iov, &msg.msg_iter);
>         if (unlikely(ret))
> @@ -4569,9 +4569,9 @@ static int io_recvmsg(struct io_kiocb *req, bool force_nonblock,
>         unsigned flags;
>         int ret, cflags = 0;
>
> -       sock = sock_from_file(req->file, &ret);
> +       sock = sock_from_file(req->file);
>         if (unlikely(!sock))
> -               return ret;
> +               return -ENOTSOCK;
>
>         if (req->async_data) {
>                 kmsg = req->async_data;
> @@ -4632,9 +4632,9 @@ static int io_recv(struct io_kiocb *req, bool force_nonblock,
>         unsigned flags;
>         int ret, cflags = 0;
>
> -       sock = sock_from_file(req->file, &ret);
> +       sock = sock_from_file(req->file);
>         if (unlikely(!sock))
> -               return ret;
> +               return -ENOTSOCK;
>
>         if (req->flags & REQ_F_BUFFER_SELECT) {
>                 kbuf = io_recv_buffer_select(req, !force_nonblock);
> diff --git a/include/linux/net.h b/include/linux/net.h
> index 0dcd51feef02..9e2324efc26a 100644
> --- a/include/linux/net.h
> +++ b/include/linux/net.h
> @@ -240,7 +240,7 @@ int sock_sendmsg(struct socket *sock, struct msghdr *msg);
>  int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
>  struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
>  struct socket *sockfd_lookup(int fd, int *err);
> -struct socket *sock_from_file(struct file *file, int *err);
> +struct socket *sock_from_file(struct file *file);
>  #define                     sockfd_put(sock) fput(sock->file)
>  int net_ratelimit(void);
>
> diff --git a/net/core/netclassid_cgroup.c b/net/core/netclassid_cgroup.c
> index 41b24cd31562..b49c57d35a88 100644
> --- a/net/core/netclassid_cgroup.c
> +++ b/net/core/netclassid_cgroup.c
> @@ -68,9 +68,8 @@ struct update_classid_context {
>
>  static int update_classid_sock(const void *v, struct file *file, unsigned n)
>  {
> -       int err;
>         struct update_classid_context *ctx = (void *)v;
> -       struct socket *sock = sock_from_file(file, &err);
> +       struct socket *sock = sock_from_file(file);
>
>         if (sock) {
>                 spin_lock(&cgroup_sk_update_lock);
> diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
> index 9bd4cab7d510..99a431c56f23 100644
> --- a/net/core/netprio_cgroup.c
> +++ b/net/core/netprio_cgroup.c
> @@ -220,8 +220,7 @@ static ssize_t write_priomap(struct kernfs_open_file *of,
>
>  static int update_netprio(const void *v, struct file *file, unsigned n)
>  {
> -       int err;
> -       struct socket *sock = sock_from_file(file, &err);
> +       struct socket *sock = sock_from_file(file);
>         if (sock) {
>                 spin_lock(&cgroup_sk_update_lock);
>                 sock_cgroup_set_prioidx(&sock->sk->sk_cgrp_data,
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 727ea1cc633c..dd0598d831ef 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2808,14 +2808,8 @@ EXPORT_SYMBOL(sock_no_mmap);
>  void __receive_sock(struct file *file)
>  {
>         struct socket *sock;
> -       int error;
>
> -       /*
> -        * The resulting value of "error" is ignored here since we only
> -        * need to take action when the file is a socket and testing
> -        * "sock" for NULL is sufficient.
> -        */
> -       sock = sock_from_file(file, &error);
> +       sock = sock_from_file(file);
>         if (sock) {
>                 sock_update_netprioidx(&sock->sk->sk_cgrp_data);
>                 sock_update_classid(&sock->sk->sk_cgrp_data);
> diff --git a/net/socket.c b/net/socket.c
> index 6e6cccc2104f..c799d9652a2c 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -445,17 +445,15 @@ static int sock_map_fd(struct socket *sock, int flags)
>  /**
>   *     sock_from_file - Return the &socket bounded to @file.
>   *     @file: file
> - *     @err: pointer to an error code return
>   *
> - *     On failure returns %NULL and assigns -ENOTSOCK to @err.
> + *     On failure returns %NULL.
>   */
>
> -struct socket *sock_from_file(struct file *file, int *err)
> +struct socket *sock_from_file(struct file *file)
>  {
>         if (file->f_op == &socket_file_ops)
>                 return file->private_data;      /* set in sock_map_fd */
>
> -       *err = -ENOTSOCK;
>         return NULL;
>  }
>  EXPORT_SYMBOL(sock_from_file);
> @@ -484,9 +482,11 @@ struct socket *sockfd_lookup(int fd, int *err)
>                 return NULL;
>         }
>
> -       sock = sock_from_file(file, err);
> -       if (!sock)
> +       sock = sock_from_file(file);
> +       if (!sock) {
> +               *err = -ENOTSOCK;
>                 fput(file);
> +       }
>         return sock;
>  }
>  EXPORT_SYMBOL(sockfd_lookup);
> @@ -498,11 +498,12 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
>
>         *err = -EBADF;
>         if (f.file) {
> -               sock = sock_from_file(f.file, err);
> +               sock = sock_from_file(f.file);
>                 if (likely(sock)) {
>                         *fput_needed = f.flags & FDPUT_FPUT;
>                         return sock;
>                 }
> +               *err = -ENOTSOCK;
>                 fdput(f);
>         }
>         return NULL;
> @@ -1715,9 +1716,11 @@ int __sys_accept4_file(struct file *file, unsigned file_flags,
>         if (SOCK_NONBLOCK != O_NONBLOCK && (flags & SOCK_NONBLOCK))
>                 flags = (flags & ~SOCK_NONBLOCK) | O_NONBLOCK;
>
> -       sock = sock_from_file(file, &err);
> -       if (!sock)
> +       sock = sock_from_file(file);
> +       if (!sock) {
> +               err = -ENOTSOCK;
>                 goto out;
> +       }
>
>         err = -ENFILE;
>         newsock = sock_alloc();
> @@ -1840,9 +1843,11 @@ int __sys_connect_file(struct file *file, struct sockaddr_storage *address,
>         struct socket *sock;
>         int err;
>
> -       sock = sock_from_file(file, &err);
> -       if (!sock)
> +       sock = sock_from_file(file);
> +       if (!sock) {
> +               err = -ENOTSOCK;
>                 goto out;
> +       }
>
>         err =
>             security_socket_connect(sock, (struct sockaddr *)address, addrlen);
> --
> 2.29.2.299.gdc1121823c-goog
>

Re: [PATCH v2 2/5] bpf: Add a bpf_sock_from_file helper

[KP Singh]

On Thu, Nov 19, 2020 at 5:27 PM Florent Revest <revest@chromium.org> wrote:
>
> From: Florent Revest <revest@google.com>
>
> While eBPF programs can check whether a file is a socket by file->f_op
> == &socket_file_ops, they cannot convert the void private_data pointer
> to a struct socket BTF pointer. In order to do this a new helper
> wrapping sock_from_file is added.
>
> This is useful to tracing programs but also other program types
> inheriting this set of helpers such as iterators or LSM programs.
>
> Signed-off-by: Florent Revest <revest@google.com>

Acked-by: KP Singh <kpsingh@google.com>

Some minor comments.

> ---
>  include/uapi/linux/bpf.h       |  7 +++++++
>  kernel/trace/bpf_trace.c       | 20 ++++++++++++++++++++
>  scripts/bpf_helpers_doc.py     |  4 ++++
>  tools/include/uapi/linux/bpf.h |  7 +++++++
>  4 files changed, 38 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 162999b12790..7d598f161dc0 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3787,6 +3787,12 @@ union bpf_attr {
>   *             *ARG_PTR_TO_BTF_ID* of type *task_struct*.
>   *     Return
>   *             Pointer to the current task.
> + *
> + * struct socket *bpf_sock_from_file(struct file *file)
> + *     Description
> + *             If the given file contains a socket, returns the associated socket.

"If the given file is a socket" or "represents a socket" would fit better here.

> + *     Return
> + *             A pointer to a struct socket on success or NULL on failure.

NULL if the file is not a socket.

Re: [PATCH v2 3/5] bpf: Expose bpf_sk_storage_* to iterator programs

[KP Singh]

On Thu, Nov 19, 2020 at 5:27 PM Florent Revest <revest@chromium.org> wrote:
>
> From: Florent Revest <revest@google.com>
>
> Iterators are currently used to expose kernel information to userspace
> over fast procfs-like files but iterators could also be used to
> manipulate local storage. For example, the task_file iterator could be
> used to initialize a socket local storage with associations between
> processes and sockets or to selectively delete local storage values.
>
> This exposes both socket local storage helpers to all iterators.
> Alternatively we could expose it to only certain iterators with strcmps
> on prog->aux->attach_func_name.

Since you mentioned the alternative here, maybe you can also
explain why you chose the current approach.

Re: [PATCH v2 2/5] bpf: Add a bpf_sock_from_file helper

[Martin KaFai Lau]

On Thu, Nov 19, 2020 at 05:26:51PM +0100, Florent Revest wrote:
> From: Florent Revest <revest@google.com>
> 
> While eBPF programs can check whether a file is a socket by file->f_op
> == &socket_file_ops, they cannot convert the void private_data pointer
> to a struct socket BTF pointer. In order to do this a new helper
> wrapping sock_from_file is added.
> 
> This is useful to tracing programs but also other program types
> inheriting this set of helpers such as iterators or LSM programs.
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH v2 3/5] bpf: Expose bpf_sk_storage_* to iterator programs

[Martin KaFai Lau]

On Thu, Nov 19, 2020 at 05:26:52PM +0100, Florent Revest wrote:
> From: Florent Revest <revest@google.com>
> 
> Iterators are currently used to expose kernel information to userspace
> over fast procfs-like files but iterators could also be used to
> manipulate local storage. For example, the task_file iterator could be
> used to initialize a socket local storage with associations between
> processes and sockets or to selectively delete local storage values.
> 
> This exposes both socket local storage helpers to all iterators.
> Alternatively we could expose it to only certain iterators with strcmps
> on prog->aux->attach_func_name.
I cannot see any hole to iter the bpf_sk_storage_map and also
bpf_sk_storage_get/delete() itself for now.

I have looked at other iters (e.g. tcp, udp, and sock_map iter).
It will be good if you can double check them also.

I think at least one more test on the tcp iter is needed.

Other than that,

Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH v2 4/5] bpf: Add an iterator selftest for bpf_sk_storage_delete

[Martin KaFai Lau]

On Thu, Nov 19, 2020 at 05:26:53PM +0100, Florent Revest wrote:
> From: Florent Revest <revest@google.com>
> 
> The eBPF program iterates over all entries (well, only one) of a socket
> local storage map and deletes them all. The test makes sure that the
> entry is indeed deleted.
Note that if there are many entries and seq->op->stop() is called (due to
seq_has_overflowed()).  It is possible that not all of the entries will be
iterated (and deleted).  However, I think it is a more generic issue in
resuming the iteration and not specific to this series.

Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH v2 5/5] bpf: Add an iterator selftest for bpf_sk_storage_get

[Martin KaFai Lau]

On Thu, Nov 19, 2020 at 05:26:54PM +0100, Florent Revest wrote:
> From: Florent Revest <revest@google.com>
> 
> The eBPF program iterates over all files and tasks. For all socket
> files, it stores the tgid of the last task it encountered with a handle
> to that socket. This is a heuristic for finding the "owner" of a socket
> similar to what's done by lsof, ss, netstat or fuser. Potentially, this
> information could be used from a cgroup_skb/*gress hook to try to
> associate network traffic with processes.
> 
> The test makes sure that a socket it created is tagged with prog_tests's
> pid.
> 
> Signed-off-by: Florent Revest <revest@google.com>
> ---
>  .../selftests/bpf/prog_tests/bpf_iter.c       | 35 +++++++++++++++++++
>  .../progs/bpf_iter_bpf_sk_storage_helpers.c   | 26 ++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> index bb4a638f2e6f..4d0626003c03 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> @@ -975,6 +975,39 @@ static void test_bpf_sk_storage_delete(void)
>  	bpf_iter_bpf_sk_storage_helpers__destroy(skel);
>  }
>  
> +/* The BPF program stores in every socket the tgid of a task owning a handle to
> + * it. The test verifies that a locally-created socket is tagged with its pid
> + */
> +static void test_bpf_sk_storage_get(void)
> +{
> +	struct bpf_iter_bpf_sk_storage_helpers *skel;
> +	int err, map_fd, val = -1;
> +	int sock_fd = -1;
> +
> +	skel = bpf_iter_bpf_sk_storage_helpers__open_and_load();
> +	if (CHECK(!skel, "bpf_iter_bpf_sk_storage_helpers__open_and_load",
> +		  "skeleton open_and_load failed\n"))
> +		return;
> +
> +	sock_fd = socket(AF_INET6, SOCK_STREAM, 0);
> +	if (CHECK(sock_fd < 0, "socket", "errno: %d\n", errno))
> +		goto out;
> +
> +	do_dummy_read(skel->progs.fill_socket_owners);
> +
> +	map_fd = bpf_map__fd(skel->maps.sk_stg_map);
> +
> +	err = bpf_map_lookup_elem(map_fd, &sock_fd, &val);
> +	CHECK(err || val != getpid(), "bpf_map_lookup_elem",
> +	      "map value wasn't set correctly (expected %d, got %d, err=%d)\n",
> +	      getpid(), val, err);
> +
> +	if (sock_fd >= 0)
> +		close(sock_fd);
> +out:
> +	bpf_iter_bpf_sk_storage_helpers__destroy(skel);
> +}
> +
>  static void test_bpf_sk_storage_map(void)
>  {
>  	DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
> @@ -1131,6 +1164,8 @@ void test_bpf_iter(void)
>  		test_bpf_sk_storage_map();
>  	if (test__start_subtest("bpf_sk_storage_delete"))
>  		test_bpf_sk_storage_delete();
> +	if (test__start_subtest("bpf_sk_storage_get"))
> +		test_bpf_sk_storage_get();
>  	if (test__start_subtest("rdonly-buf-out-of-bound"))
>  		test_rdonly_buf_out_of_bound();
>  	if (test__start_subtest("buf-neg-offset"))
> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
> index 01ff3235e413..7206fd6f09ab 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
> @@ -21,3 +21,29 @@ int delete_bpf_sk_storage_map(struct bpf_iter__bpf_sk_storage_map *ctx)
>  
>  	return 0;
>  }
> +
> +SEC("iter/task_file")
> +int fill_socket_owners(struct bpf_iter__task_file *ctx)
> +{
> +	struct task_struct *task = ctx->task;
> +	struct file *file = ctx->file;
> +	struct socket *sock;
> +	int *sock_tgid;
> +
> +	if (!task || !file || task->tgid != task->pid)
> +		return 0;
> +
> +	sock = bpf_sock_from_file(file);
> +	if (!sock)
> +		return 0;
> +
> +	sock_tgid = bpf_sk_storage_get(&sk_stg_map, sock->sk, 0,
> +				       BPF_SK_STORAGE_GET_F_CREATE);
Does it affect all sk(s) in the system?  Can it be limited to
the sk that the test is testing?

Re: [PATCH v2 5/5] bpf: Add an iterator selftest for bpf_sk_storage_get

[KP Singh]

On Fri, Nov 20, 2020 at 1:32 AM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Thu, Nov 19, 2020 at 05:26:54PM +0100, Florent Revest wrote:
> > From: Florent Revest <revest@google.com>
> >
> > The eBPF program iterates over all files and tasks. For all socket
> > files, it stores the tgid of the last task it encountered with a handle
> > to that socket. This is a heuristic for finding the "owner" of a socket
> > similar to what's done by lsof, ss, netstat or fuser. Potentially, this
> > information could be used from a cgroup_skb/*gress hook to try to
> > associate network traffic with processes.
> >
> > The test makes sure that a socket it created is tagged with prog_tests's
> > pid.
> >
> > Signed-off-by: Florent Revest <revest@google.com>
> > ---
> >  .../selftests/bpf/prog_tests/bpf_iter.c       | 35 +++++++++++++++++++
> >  .../progs/bpf_iter_bpf_sk_storage_helpers.c   | 26 ++++++++++++++
> >  2 files changed, 61 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > index bb4a638f2e6f..4d0626003c03 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
> > @@ -975,6 +975,39 @@ static void test_bpf_sk_storage_delete(void)
> >       bpf_iter_bpf_sk_storage_helpers__destroy(skel);
> >  }
> >
> > +/* The BPF program stores in every socket the tgid of a task owning a handle to
> > + * it. The test verifies that a locally-created socket is tagged with its pid
> > + */
> > +static void test_bpf_sk_storage_get(void)
> > +{
> > +     struct bpf_iter_bpf_sk_storage_helpers *skel;
> > +     int err, map_fd, val = -1;
> > +     int sock_fd = -1;
> > +
> > +     skel = bpf_iter_bpf_sk_storage_helpers__open_and_load();
> > +     if (CHECK(!skel, "bpf_iter_bpf_sk_storage_helpers__open_and_load",
> > +               "skeleton open_and_load failed\n"))
> > +             return;
> > +
> > +     sock_fd = socket(AF_INET6, SOCK_STREAM, 0);
> > +     if (CHECK(sock_fd < 0, "socket", "errno: %d\n", errno))
> > +             goto out;
> > +
> > +     do_dummy_read(skel->progs.fill_socket_owners);
> > +
> > +     map_fd = bpf_map__fd(skel->maps.sk_stg_map);
> > +
> > +     err = bpf_map_lookup_elem(map_fd, &sock_fd, &val);
> > +     CHECK(err || val != getpid(), "bpf_map_lookup_elem",
> > +           "map value wasn't set correctly (expected %d, got %d, err=%d)\n",
> > +           getpid(), val, err);
> > +
> > +     if (sock_fd >= 0)
> > +             close(sock_fd);
> > +out:
> > +     bpf_iter_bpf_sk_storage_helpers__destroy(skel);
> > +}
> > +
> >  static void test_bpf_sk_storage_map(void)
> >  {
> >       DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
> > @@ -1131,6 +1164,8 @@ void test_bpf_iter(void)
> >               test_bpf_sk_storage_map();
> >       if (test__start_subtest("bpf_sk_storage_delete"))
> >               test_bpf_sk_storage_delete();
> > +     if (test__start_subtest("bpf_sk_storage_get"))
> > +             test_bpf_sk_storage_get();
> >       if (test__start_subtest("rdonly-buf-out-of-bound"))
> >               test_rdonly_buf_out_of_bound();
> >       if (test__start_subtest("buf-neg-offset"))
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
> > index 01ff3235e413..7206fd6f09ab 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
> > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_bpf_sk_storage_helpers.c
> > @@ -21,3 +21,29 @@ int delete_bpf_sk_storage_map(struct bpf_iter__bpf_sk_storage_map *ctx)
> >
> >       return 0;
> >  }
> > +
> > +SEC("iter/task_file")
> > +int fill_socket_owners(struct bpf_iter__task_file *ctx)
> > +{
> > +     struct task_struct *task = ctx->task;
> > +     struct file *file = ctx->file;
> > +     struct socket *sock;
> > +     int *sock_tgid;
> > +
> > +     if (!task || !file || task->tgid != task->pid)
> > +             return 0;
> > +
> > +     sock = bpf_sock_from_file(file);
> > +     if (!sock)
> > +             return 0;
> > +
> > +     sock_tgid = bpf_sk_storage_get(&sk_stg_map, sock->sk, 0,
> > +                                    BPF_SK_STORAGE_GET_F_CREATE);
> Does it affect all sk(s) in the system?  Can it be limited to
> the sk that the test is testing?

Yeah, one such way would be to set the socket storage on the socket
from userspace and then "search" for the socket in the iterator and
mark it as found in a shared global variable.

Re: [PATCH v2 5/5] bpf: Add an iterator selftest for bpf_sk_storage_get

[Florent Revest]

On Thu, 2020-11-19 at 16:32 -0800, Martin KaFai Lau wrote:
> Does it affect all sk(s) in the system?  Can it be limited to
> the sk that the test is testing?

Oh I just realized I haven't answered you here yet! Thanks for the
reviews. :D I'm sending a v3 addressing your comments