[PATCH v4 bpf-next 0/3] BPF iterator for UNIX domain socket.

[PATCH v4 bpf-next 0/3] BPF iterator for UNIX domain socket.

[Kuniyuki Iwashima]

This patch set adds BPF iterator support for UNIX domain socket.  The first
patch implements it, the second adds "%c" support for BPF_SEQ_PRINTF(), and
the third adds a selftest.


Changelog:
  v4:
  - Check IS_BUILTIN(CONFIG_UNIX)
  - Support "%c" in BPF_SEQ_PRINTF()
  - Uncomment the code to print the name of the abstract socket
  - Mention the LLVM fix in README.rst
  - Remove the 'aligned' attribute in bpf_iter.h
  - Keep the format string on a single line

  v3:
  https://lore.kernel.org/netdev/20210804070851.97834-1-kuniyu@amazon.co.jp/
  - Export some functions for CONFIG_UNIX=m

  v2:
  https://lore.kernel.org/netdev/20210803011110.21205-1-kuniyu@amazon.co.jp/
  - Implement bpf_iter specific seq_ops->stop()
  - Add bpf_iter__unix in bpf_iter.h
  - Move common definitions in selftest to bpf_tracing_net.h
  - Include the code for abstract UNIX domain socket as comment in selftest
  - Use ASSERT_OK_PTR() instead of CHECK()
  - Make ternary operators on single line

  v1:
  https://lore.kernel.org/netdev/20210729233645.4869-1-kuniyu@amazon.co.jp/


Kuniyuki Iwashima (3):
  bpf: af_unix: Implement BPF iterator for UNIX domain socket.
  bpf: Support "%c" in bpf_bprintf_prepare().
  selftest/bpf: Implement sample UNIX domain socket iterator program.

 include/linux/btf_ids.h                       |  3 +-
 kernel/bpf/helpers.c                          | 14 +++
 net/unix/af_unix.c                            | 93 +++++++++++++++++++
 tools/testing/selftests/bpf/README.rst        | 38 ++++++++
 .../selftests/bpf/prog_tests/bpf_iter.c       | 16 ++++
 tools/testing/selftests/bpf/progs/bpf_iter.h  |  8 ++
 .../selftests/bpf/progs/bpf_iter_unix.c       | 77 +++++++++++++++
 .../selftests/bpf/progs/bpf_tracing_net.h     |  4 +
 8 files changed, 252 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c

-- 
2.30.2

[PATCH v4 bpf-next 1/3] bpf: af_unix: Implement BPF iterator for UNIX domain socket.

[Kuniyuki Iwashima]

This patch implements the BPF iterator for the UNIX domain socket.

Currently, the batch optimisation introduced for the TCP iterator in the
commit 04c7820b776f ("bpf: tcp: Bpf iter batching and lock_sock") is not
used for the UNIX domain socket.  It will require replacing the big lock
for the hash table with small locks for each hash list not to block other
processes.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
---
 include/linux/btf_ids.h |  3 +-
 net/unix/af_unix.c      | 93 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index 57890b357f85..bed4b9964581 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -172,7 +172,8 @@ extern struct btf_id_set name;
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP_TW, tcp_timewait_sock)		\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)			\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)			\
-	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)
+	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)			\
+	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UNIX, unix_sock)
 
 enum {
 #define BTF_SOCK_TYPE(name, str) name,
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 256c4e31132e..15e1f1bc22a8 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -113,6 +113,7 @@
 #include <linux/security.h>
 #include <linux/freezer.h>
 #include <linux/file.h>
+#include <linux/btf_ids.h>
 
 #include "scm.h"
 
@@ -2982,6 +2983,64 @@ static const struct seq_operations unix_seq_ops = {
 	.stop   = unix_seq_stop,
 	.show   = unix_seq_show,
 };
+
+#if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL)
+struct bpf_iter__unix {
+	__bpf_md_ptr(struct bpf_iter_meta *, meta);
+	__bpf_md_ptr(struct unix_sock *, unix_sk);
+	uid_t uid __aligned(8);
+};
+
+static int unix_prog_seq_show(struct bpf_prog *prog, struct bpf_iter_meta *meta,
+			      struct unix_sock *unix_sk, uid_t uid)
+{
+	struct bpf_iter__unix ctx;
+
+	meta->seq_num--;  /* skip SEQ_START_TOKEN */
+	ctx.meta = meta;
+	ctx.unix_sk = unix_sk;
+	ctx.uid = uid;
+	return bpf_iter_run_prog(prog, &ctx);
+}
+
+static int bpf_iter_unix_seq_show(struct seq_file *seq, void *v)
+{
+	struct bpf_iter_meta meta;
+	struct bpf_prog *prog;
+	struct sock *sk = v;
+	uid_t uid;
+
+	if (v == SEQ_START_TOKEN)
+		return 0;
+
+	uid = from_kuid_munged(seq_user_ns(seq), sock_i_uid(sk));
+	meta.seq = seq;
+	prog = bpf_iter_get_info(&meta, false);
+	return unix_prog_seq_show(prog, &meta, v, uid);
+}
+
+static void bpf_iter_unix_seq_stop(struct seq_file *seq, void *v)
+{
+	struct bpf_iter_meta meta;
+	struct bpf_prog *prog;
+
+	if (!v) {
+		meta.seq = seq;
+		prog = bpf_iter_get_info(&meta, true);
+		if (prog)
+			(void)unix_prog_seq_show(prog, &meta, v, 0);
+	}
+
+	unix_seq_stop(seq, v);
+}
+
+static const struct seq_operations bpf_iter_unix_seq_ops = {
+	.start	= unix_seq_start,
+	.next	= unix_seq_next,
+	.stop	= bpf_iter_unix_seq_stop,
+	.show	= bpf_iter_unix_seq_show,
+};
+#endif
 #endif
 
 static const struct net_proto_family unix_family_ops = {
@@ -3022,6 +3081,35 @@ static struct pernet_operations unix_net_ops = {
 	.exit = unix_net_exit,
 };
 
+#if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
+DEFINE_BPF_ITER_FUNC(unix, struct bpf_iter_meta *meta,
+		     struct unix_sock *unix_sk, uid_t uid)
+
+static const struct bpf_iter_seq_info unix_seq_info = {
+	.seq_ops		= &bpf_iter_unix_seq_ops,
+	.init_seq_private	= bpf_iter_init_seq_net,
+	.fini_seq_private	= bpf_iter_fini_seq_net,
+	.seq_priv_size		= sizeof(struct seq_net_private),
+};
+
+static struct bpf_iter_reg unix_reg_info = {
+	.target			= "unix",
+	.ctx_arg_info_size	= 1,
+	.ctx_arg_info		= {
+		{ offsetof(struct bpf_iter__unix, unix_sk),
+		  PTR_TO_BTF_ID_OR_NULL },
+	},
+	.seq_info		= &unix_seq_info,
+};
+
+static void __init bpf_iter_register(void)
+{
+	unix_reg_info.ctx_arg_info[0].btf_id = btf_sock_ids[BTF_SOCK_TYPE_UNIX];
+	if (bpf_iter_reg_target(&unix_reg_info))
+		pr_warn("Warning: could not register bpf iterator unix\n");
+}
+#endif
+
 static int __init af_unix_init(void)
 {
 	int rc = -1;
@@ -3037,6 +3125,11 @@ static int __init af_unix_init(void)
 	sock_register(&unix_family_ops);
 	register_pernet_subsys(&unix_net_ops);
 	unix_bpf_build_proto();
+
+#if IS_BUILTIN(CONFIG_UNIX) && defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_PROC_FS)
+	bpf_iter_register();
+#endif
+
 out:
 	return rc;
 }
-- 
2.30.2

[PATCH v4 bpf-next 2/3] bpf: Support "%c" in bpf_bprintf_prepare().

[Kuniyuki Iwashima]

/proc/net/unix uses "%c" to print a single-byte character to escape '\0' in
the name of the abstract UNIX domain socket.  The following selftest uses
it, so this patch adds support for "%c".  Note that it does not support
wide character ("%lc" and "%llc") for simplicity.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
---
 kernel/bpf/helpers.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 15746f779fe1..6d3aaf94e9ac 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -907,6 +907,20 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
 			tmp_buf += err;
 			num_spec++;
 
+			continue;
+		} else if (fmt[i] == 'c') {
+			if (!tmp_buf)
+				goto nocopy_fmt;
+
+			if (tmp_buf_end == tmp_buf) {
+				err = -ENOSPC;
+				goto out;
+			}
+
+			*tmp_buf = raw_args[num_spec];
+			tmp_buf++;
+			num_spec++;
+
 			continue;
 		}
 
-- 
2.30.2

[PATCH v4 bpf-next 3/3] selftest/bpf: Implement sample UNIX domain socket iterator program.

[Kuniyuki Iwashima]

The iterator can output the same result compared to /proc/net/unix.

  # cat /sys/fs/bpf/unix
  Num       RefCount Protocol Flags    Type St Inode Path
  ffff9ab7122db000: 00000002 00000000 00010000 0001 01 10623 private/defer
  ffff9fca0023d000: 00000002 00000000 00000000 0001 01 11058 @Hello@World@

  # cat /proc/net/unix
  Num       RefCount Protocol Flags    Type St Inode Path
  ffff9ab7122db000: 00000002 00000000 00010000 0001 01 10623 private/defer
  ffff9fca0023d000: 00000002 00000000 00000000 0001 01 11058 @Hello@World@

Note that this prog requires the patch ([0]) for LLVM code gen.  Thanks to
Yonghong Song for analysing and fixing.

[0] https://reviews.llvm.org/D107483

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
---
 tools/testing/selftests/bpf/README.rst        | 38 +++++++++
 .../selftests/bpf/prog_tests/bpf_iter.c       | 16 ++++
 tools/testing/selftests/bpf/progs/bpf_iter.h  |  8 ++
 .../selftests/bpf/progs/bpf_iter_unix.c       | 77 +++++++++++++++++++
 .../selftests/bpf/progs/bpf_tracing_net.h     |  4 +
 5 files changed, 143 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c

diff --git a/tools/testing/selftests/bpf/README.rst b/tools/testing/selftests/bpf/README.rst
index 9b17f2867488..314a63b69399 100644
--- a/tools/testing/selftests/bpf/README.rst
+++ b/tools/testing/selftests/bpf/README.rst
@@ -228,3 +228,41 @@ To fix this issue, user newer libbpf.
 .. Links
 .. _clang reloc patch: https://reviews.llvm.org/D102712
 .. _kernel llvm reloc: /Documentation/bpf/llvm_reloc.rst
+
+bpf_iter_unix.o test failure and LLVM optimisation
+==================================================
+
+The ``bpf_iter/unix`` subtest requires `the fix`__ to prevent the LLVM compiler
+from transforming the loop exit condition.
+
+Without the fix, the compiler generates the following code:
+
+.. code-block:: c
+
+  ; 				 for (i = 1; i < len; i++)
+       110:	07 09 00 00 01 00 00 00	r9 += 1
+       111:	5d 98 09 00 00 00 00 00	if r8 != r9 goto +9 <LBB0_18>
+
+The loop exit condition, where the upper bound is not a constant, is
+changed from ``<`` to ``!=``.  Thus, the verifier always evaluates it as true,
+misleading to an infinite loop.
+
+.. code-block:: c
+
+  The sequence of 8193 jumps is too complex.
+  processed 196506 insns (limit 1000000) max_states_per_insn 4 total_states 1830 peak_states 1830 mark_read 3
+
+The fix prevents the optimisation by estimating its cost higher in such a
+case:
+
+.. code-block:: c
+
+  ; 				 for (i = 1; i < len; i++)
+       110:	07 09 00 00 01 00 00 00	r9 += 1
+       111:	ad 89 09 00 00 00 00 00	if r9 < r8 goto +9 <LBB0_18>
+
+The patch is available in LLVM 14 trunk and has been `backported`__ to the LLVM
+13.x release branch.
+
+__ https://reviews.llvm.org/D107483
+__ https://bugs.llvm.org/show_bug.cgi?id=51363
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
index 1f1aade56504..77ac24b191d4 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
@@ -13,6 +13,7 @@
 #include "bpf_iter_tcp6.skel.h"
 #include "bpf_iter_udp4.skel.h"
 #include "bpf_iter_udp6.skel.h"
+#include "bpf_iter_unix.skel.h"
 #include "bpf_iter_test_kern1.skel.h"
 #include "bpf_iter_test_kern2.skel.h"
 #include "bpf_iter_test_kern3.skel.h"
@@ -313,6 +314,19 @@ static void test_udp6(void)
 	bpf_iter_udp6__destroy(skel);
 }
 
+static void test_unix(void)
+{
+	struct bpf_iter_unix *skel;
+
+	skel = bpf_iter_unix__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "bpf_iter_unix__open_and_load"))
+		return;
+
+	do_dummy_read(skel->progs.dump_unix);
+
+	bpf_iter_unix__destroy(skel);
+}
+
 /* The expected string is less than 16 bytes */
 static int do_read_with_fd(int iter_fd, const char *expected,
 			   bool read_one_char)
@@ -1255,6 +1269,8 @@ void test_bpf_iter(void)
 		test_udp4();
 	if (test__start_subtest("udp6"))
 		test_udp6();
+	if (test__start_subtest("unix"))
+		test_unix();
 	if (test__start_subtest("anon"))
 		test_anon_iter(false);
 	if (test__start_subtest("anon-read-one-char"))
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter.h b/tools/testing/selftests/bpf/progs/bpf_iter.h
index 3d83b185c4bc..8cfaeba1ddbf 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter.h
+++ b/tools/testing/selftests/bpf/progs/bpf_iter.h
@@ -12,6 +12,7 @@
 #define tcp6_sock tcp6_sock___not_used
 #define bpf_iter__udp bpf_iter__udp___not_used
 #define udp6_sock udp6_sock___not_used
+#define bpf_iter__unix bpf_iter__unix___not_used
 #define bpf_iter__bpf_map_elem bpf_iter__bpf_map_elem___not_used
 #define bpf_iter__bpf_sk_storage_map bpf_iter__bpf_sk_storage_map___not_used
 #define bpf_iter__sockmap bpf_iter__sockmap___not_used
@@ -32,6 +33,7 @@
 #undef tcp6_sock
 #undef bpf_iter__udp
 #undef udp6_sock
+#undef bpf_iter__unix
 #undef bpf_iter__bpf_map_elem
 #undef bpf_iter__bpf_sk_storage_map
 #undef bpf_iter__sockmap
@@ -103,6 +105,12 @@ struct udp6_sock {
 	struct ipv6_pinfo inet6;
 } __attribute__((preserve_access_index));
 
+struct bpf_iter__unix {
+	struct bpf_iter_meta *meta;
+	struct unix_sock *unix_sk;
+	uid_t uid;
+} __attribute__((preserve_access_index));
+
 struct bpf_iter__bpf_map_elem {
 	struct bpf_iter_meta *meta;
 	struct bpf_map *map;
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_unix.c b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
new file mode 100644
index 000000000000..06e417ff718b
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright Amazon.com Inc. or its affiliates. */
+#include "bpf_iter.h"
+#include "bpf_tracing_net.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+char _license[] SEC("license") = "GPL";
+
+static long sock_i_ino(const struct sock *sk)
+{
+	const struct socket *sk_socket = sk->sk_socket;
+	const struct inode *inode;
+	unsigned long ino;
+
+	if (!sk_socket)
+		return 0;
+
+	inode = &container_of(sk_socket, struct socket_alloc, socket)->vfs_inode;
+	bpf_probe_read_kernel(&ino, sizeof(ino), &inode->i_ino);
+	return ino;
+}
+
+SEC("iter/unix")
+int dump_unix(struct bpf_iter__unix *ctx)
+{
+	struct unix_sock *unix_sk = ctx->unix_sk;
+	struct sock *sk = (struct sock *)unix_sk;
+	struct seq_file *seq;
+	__u32 seq_num;
+
+	if (!unix_sk)
+		return 0;
+
+	seq = ctx->meta->seq;
+	seq_num = ctx->meta->seq_num;
+	if (seq_num == 0)
+		BPF_SEQ_PRINTF(seq, "Num       RefCount Protocol Flags    Type St Inode Path\n");
+
+	BPF_SEQ_PRINTF(seq, "%pK: %08X %08X %08X %04X %02X %5lu",
+		       unix_sk,
+		       sk->sk_refcnt.refs.counter,
+		       0,
+		       sk->sk_state == TCP_LISTEN ? __SO_ACCEPTCON : 0,
+		       sk->sk_type,
+		       sk->sk_socket ?
+		       (sk->sk_state == TCP_ESTABLISHED ? SS_CONNECTED : SS_UNCONNECTED) :
+		       (sk->sk_state == TCP_ESTABLISHED ? SS_CONNECTING : SS_DISCONNECTING),
+		       sock_i_ino(sk));
+
+	if (unix_sk->addr) {
+		if (!UNIX_ABSTRACT(unix_sk)) {
+			BPF_SEQ_PRINTF(seq, " %s", unix_sk->addr->name->sun_path);
+		} else {
+			/* The name of the abstract UNIX domain socket starts
+			 * with '\0' and can contain '\0'.  The null bytes
+			 * should be escaped as done in unix_seq_show().
+			 */
+			 int i, len;
+
+			 len = unix_sk->addr->len - sizeof(short);
+
+			 BPF_SEQ_PRINTF(seq, " @");
+
+			 /* unix_mkname() tests this upper bound. */
+			 if (len < sizeof(struct sockaddr_un))
+				 for (i = 1; i < len; i++)
+					 BPF_SEQ_PRINTF(seq, "%c",
+							unix_sk->addr->name->sun_path[i] ?:
+							'@');
+		}
+	}
+
+	BPF_SEQ_PRINTF(seq, "\n");
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
index 3af0998a0623..eef5646ddb19 100644
--- a/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
+++ b/tools/testing/selftests/bpf/progs/bpf_tracing_net.h
@@ -5,6 +5,10 @@
 #define AF_INET			2
 #define AF_INET6		10
 
+#define __SO_ACCEPTCON		(1 << 16)
+#define UNIX_HASH_SIZE		256
+#define UNIX_ABSTRACT(unix_sk)	(unix_sk->addr->hash < UNIX_HASH_SIZE)
+
 #define SOL_TCP			6
 #define TCP_CONGESTION		13
 #define TCP_CA_NAME_MAX		16
-- 
2.30.2

Re: [PATCH v4 bpf-next 1/3] bpf: af_unix: Implement BPF iterator for UNIX domain socket.

[Yonghong Song]

On 8/10/21 2:28 AM, Kuniyuki Iwashima wrote:
> This patch implements the BPF iterator for the UNIX domain socket.
> 
> Currently, the batch optimisation introduced for the TCP iterator in the
> commit 04c7820b776f ("bpf: tcp: Bpf iter batching and lock_sock") is not
> used for the UNIX domain socket.  It will require replacing the big lock
> for the hash table with small locks for each hash list not to block other
> processes.
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>

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

Re: [PATCH v4 bpf-next 2/3] bpf: Support "%c" in bpf_bprintf_prepare().

[Yonghong Song]

On 8/10/21 2:28 AM, Kuniyuki Iwashima wrote:
> /proc/net/unix uses "%c" to print a single-byte character to escape '\0' in
> the name of the abstract UNIX domain socket.  The following selftest uses
> it, so this patch adds support for "%c".  Note that it does not support
> wide character ("%lc" and "%llc") for simplicity.
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>

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

Re: [PATCH v4 bpf-next 3/3] selftest/bpf: Implement sample UNIX domain socket iterator program.

[Yonghong Song]

On 8/10/21 2:28 AM, Kuniyuki Iwashima wrote:
> The iterator can output the same result compared to /proc/net/unix.
> 
>    # cat /sys/fs/bpf/unix
>    Num       RefCount Protocol Flags    Type St Inode Path
>    ffff9ab7122db000: 00000002 00000000 00010000 0001 01 10623 private/defer

There seems a misalignment between header line and data line.
I know /proc/net/unix having this issue as well. But can we adjust 
spacing in bpf program to make header/data properly aligned?

>    ffff9fca0023d000: 00000002 00000000 00000000 0001 01 11058 @Hello@World@
> 
>    # cat /proc/net/unix
>    Num       RefCount Protocol Flags    Type St Inode Path
>    ffff9ab7122db000: 00000002 00000000 00010000 0001 01 10623 private/defer
>    ffff9fca0023d000: 00000002 00000000 00000000 0001 01 11058 @Hello@World@
> 
> Note that this prog requires the patch ([0]) for LLVM code gen.  Thanks to
> Yonghong Song for analysing and fixing.
> 
> [0] https://reviews.llvm.org/D107483
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>

LGTM. Thanks!

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

Re: [PATCH v4 bpf-next 3/3] selftest/bpf: Implement sample UNIX domain socket iterator program.

[Kuniyuki Iwashima]

From:   Yonghong Song <yhs@fb.com>
Date:   Tue, 10 Aug 2021 16:46:49 -0700
> On 8/10/21 2:28 AM, Kuniyuki Iwashima wrote:
> > The iterator can output the same result compared to /proc/net/unix.
> > 
> >    # cat /sys/fs/bpf/unix
> >    Num       RefCount Protocol Flags    Type St Inode Path
> >    ffff9ab7122db000: 00000002 00000000 00010000 0001 01 10623 private/defer
> 
> There seems a misalignment between header line and data line.
> I know /proc/net/unix having this issue as well. But can we adjust 
> spacing in bpf program to make header/data properly aligned?

I just followed /proc/net/unix not to break old parsers :)
I'll align it cleanly in the next spin.


> 
> >    ffff9fca0023d000: 00000002 00000000 00000000 0001 01 11058 @Hello@World@
> > 
> >    # cat /proc/net/unix
> >    Num       RefCount Protocol Flags    Type St Inode Path
> >    ffff9ab7122db000: 00000002 00000000 00010000 0001 01 10623 private/defer
> >    ffff9fca0023d000: 00000002 00000000 00000000 0001 01 11058 @Hello@World@
> > 
> > Note that this prog requires the patch ([0]) for LLVM code gen.  Thanks to
> > Yonghong Song for analysing and fixing.
> > 
> > [0] https://reviews.llvm.org/D107483
> > 
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> 
> LGTM. Thanks!
> 
> Acked-by: Yonghong Song <yhs@fb.com>

Thank you, too!!

Re: [PATCH v4 bpf-next 2/3] bpf: Support "%c" in bpf_bprintf_prepare().

[Andrii Nakryiko]

On Tue, Aug 10, 2021 at 2:29 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>
> /proc/net/unix uses "%c" to print a single-byte character to escape '\0' in
> the name of the abstract UNIX domain socket.  The following selftest uses
> it, so this patch adds support for "%c".  Note that it does not support
> wide character ("%lc" and "%llc") for simplicity.
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> ---
>  kernel/bpf/helpers.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 15746f779fe1..6d3aaf94e9ac 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -907,6 +907,20 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
>                         tmp_buf += err;
>                         num_spec++;
>
> +                       continue;
> +               } else if (fmt[i] == 'c') {

you are adding new features to printk-like helpers, please add
corresponding tests as well. I'm particularly curious how something
like "% 9c" (which is now allowed, along with a few other unusual
combinations) will work.

> +                       if (!tmp_buf)
> +                               goto nocopy_fmt;
> +
> +                       if (tmp_buf_end == tmp_buf) {
> +                               err = -ENOSPC;
> +                               goto out;
> +                       }
> +
> +                       *tmp_buf = raw_args[num_spec];
> +                       tmp_buf++;
> +                       num_spec++;
> +
>                         continue;
>                 }
>
> --
> 2.30.2
>

Re: [PATCH v4 bpf-next 2/3] bpf: Support "%c" in bpf_bprintf_prepare().

[Kuniyuki Iwashima]

From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Wed, 11 Aug 2021 14:15:50 -0700
> On Tue, Aug 10, 2021 at 2:29 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> >
> > /proc/net/unix uses "%c" to print a single-byte character to escape '\0' in
> > the name of the abstract UNIX domain socket.  The following selftest uses
> > it, so this patch adds support for "%c".  Note that it does not support
> > wide character ("%lc" and "%llc") for simplicity.
> >
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > ---
> >  kernel/bpf/helpers.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 15746f779fe1..6d3aaf94e9ac 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -907,6 +907,20 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
> >                         tmp_buf += err;
> >                         num_spec++;
> >
> > +                       continue;
> > +               } else if (fmt[i] == 'c') {
> 
> you are adding new features to printk-like helpers, please add
> corresponding tests as well. I'm particularly curious how something
> like "% 9c" (which is now allowed, along with a few other unusual
> combinations) will work.

I see. I'll add a test.
I'm now thinking of test like:
  1. pin the bpf prog that outputs "% 9c" and other format strings.
  2. read and validate it

Is there any related test ?
and is there other complicated fomat strings to test ?

Also, "% 9c" worked as is :)

---8<---
$ sudo ./tools/bpftool/bpftool iter pin ./bpf_iter_unix.o /sys/fs/bpf/unix
$ sudo cat /sys/fs/bpf/unix | head -n 1
        a
$ git diff
diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_unix.c b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
index ad397e2962cf..8a7d5aa4c054 100644
--- a/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
+++ b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
@@ -34,8 +34,10 @@ int dump_unix(struct bpf_iter__unix *ctx)
 
        seq = ctx->meta->seq;
        seq_num = ctx->meta->seq_num;
-       if (seq_num == 0)
+       if (seq_num == 0) {
+               BPF_SEQ_PRINTF(seq, "% 9c\n", 'a');
                BPF_SEQ_PRINTF(seq, "Num               RefCount Protocol Flags    Type St Inode    Path\n");
+       }
 
        BPF_SEQ_PRINTF(seq, "%pK: %08X %08X %08X %04X %02X %8lu",
                       unix_sk,
---8<---



> 
> > +                       if (!tmp_buf)
> > +                               goto nocopy_fmt;
> > +
> > +                       if (tmp_buf_end == tmp_buf) {
> > +                               err = -ENOSPC;
> > +                               goto out;
> > +                       }
> > +
> > +                       *tmp_buf = raw_args[num_spec];
> > +                       tmp_buf++;
> > +                       num_spec++;
> > +
> >                         continue;
> >                 }
> >
> > --
> > 2.30.2

Re: [PATCH v4 bpf-next 2/3] bpf: Support "%c" in bpf_bprintf_prepare().

[Andrii Nakryiko]

On Wed, Aug 11, 2021 at 7:15 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
>
> From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Date:   Wed, 11 Aug 2021 14:15:50 -0700
> > On Tue, Aug 10, 2021 at 2:29 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> > >
> > > /proc/net/unix uses "%c" to print a single-byte character to escape '\0' in
> > > the name of the abstract UNIX domain socket.  The following selftest uses
> > > it, so this patch adds support for "%c".  Note that it does not support
> > > wide character ("%lc" and "%llc") for simplicity.
> > >
> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > > ---
> > >  kernel/bpf/helpers.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > index 15746f779fe1..6d3aaf94e9ac 100644
> > > --- a/kernel/bpf/helpers.c
> > > +++ b/kernel/bpf/helpers.c
> > > @@ -907,6 +907,20 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
> > >                         tmp_buf += err;
> > >                         num_spec++;
> > >
> > > +                       continue;
> > > +               } else if (fmt[i] == 'c') {
> >
> > you are adding new features to printk-like helpers, please add
> > corresponding tests as well. I'm particularly curious how something
> > like "% 9c" (which is now allowed, along with a few other unusual
> > combinations) will work.
>
> I see. I'll add a test.
> I'm now thinking of test like:
>   1. pin the bpf prog that outputs "% 9c" and other format strings.
>   2. read and validate it

Simpler. Use bpf_snprintf() to test all this logic.
bpf_trace_printk(), bpf_snprintf() and bpf_seq_printf() share the same
"backend" in kernel. No need to use bpf_iter program for testing this.
Look for other snprintf() tests and just extend them.

>
> Is there any related test ?
> and is there other complicated fomat strings to test ?
>
> Also, "% 9c" worked as is :)
>
> ---8<---
> $ sudo ./tools/bpftool/bpftool iter pin ./bpf_iter_unix.o /sys/fs/bpf/unix
> $ sudo cat /sys/fs/bpf/unix | head -n 1
>         a
> $ git diff
> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_unix.c b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> index ad397e2962cf..8a7d5aa4c054 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> @@ -34,8 +34,10 @@ int dump_unix(struct bpf_iter__unix *ctx)
>
>         seq = ctx->meta->seq;
>         seq_num = ctx->meta->seq_num;
> -       if (seq_num == 0)
> +       if (seq_num == 0) {
> +               BPF_SEQ_PRINTF(seq, "% 9c\n", 'a');
>                 BPF_SEQ_PRINTF(seq, "Num               RefCount Protocol Flags    Type St Inode    Path\n");
> +       }
>
>         BPF_SEQ_PRINTF(seq, "%pK: %08X %08X %08X %04X %02X %8lu",
>                        unix_sk,
> ---8<---
>
>
>
> >
> > > +                       if (!tmp_buf)
> > > +                               goto nocopy_fmt;
> > > +
> > > +                       if (tmp_buf_end == tmp_buf) {
> > > +                               err = -ENOSPC;
> > > +                               goto out;
> > > +                       }
> > > +
> > > +                       *tmp_buf = raw_args[num_spec];
> > > +                       tmp_buf++;
> > > +                       num_spec++;
> > > +
> > >                         continue;
> > >                 }
> > >
> > > --
> > > 2.30.2

Re: [PATCH v4 bpf-next 2/3] bpf: Support "%c" in bpf_bprintf_prepare().

[Kuniyuki Iwashima]

From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Wed, 11 Aug 2021 21:24:31 -0700
> On Wed, Aug 11, 2021 at 7:15 PM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> >
> > From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Date:   Wed, 11 Aug 2021 14:15:50 -0700
> > > On Tue, Aug 10, 2021 at 2:29 AM Kuniyuki Iwashima <kuniyu@amazon.co.jp> wrote:
> > > >
> > > > /proc/net/unix uses "%c" to print a single-byte character to escape '\0' in
> > > > the name of the abstract UNIX domain socket.  The following selftest uses
> > > > it, so this patch adds support for "%c".  Note that it does not support
> > > > wide character ("%lc" and "%llc") for simplicity.
> > > >
> > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.co.jp>
> > > > ---
> > > >  kernel/bpf/helpers.c | 14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > > index 15746f779fe1..6d3aaf94e9ac 100644
> > > > --- a/kernel/bpf/helpers.c
> > > > +++ b/kernel/bpf/helpers.c
> > > > @@ -907,6 +907,20 @@ int bpf_bprintf_prepare(char *fmt, u32 fmt_size, const u64 *raw_args,
> > > >                         tmp_buf += err;
> > > >                         num_spec++;
> > > >
> > > > +                       continue;
> > > > +               } else if (fmt[i] == 'c') {
> > >
> > > you are adding new features to printk-like helpers, please add
> > > corresponding tests as well. I'm particularly curious how something
> > > like "% 9c" (which is now allowed, along with a few other unusual
> > > combinations) will work.
> >
> > I see. I'll add a test.
> > I'm now thinking of test like:
> >   1. pin the bpf prog that outputs "% 9c" and other format strings.
> >   2. read and validate it
> 
> Simpler. Use bpf_snprintf() to test all this logic.
> bpf_trace_printk(), bpf_snprintf() and bpf_seq_printf() share the same
> "backend" in kernel. No need to use bpf_iter program for testing this.
> Look for other snprintf() tests and just extend them.

I'll extend prog_tests/snprintf.c.
Thank you!


> 
> >
> > Is there any related test ?
> > and is there other complicated fomat strings to test ?
> >
> > Also, "% 9c" worked as is :)
> >
> > ---8<---
> > $ sudo ./tools/bpftool/bpftool iter pin ./bpf_iter_unix.o /sys/fs/bpf/unix
> > $ sudo cat /sys/fs/bpf/unix | head -n 1
> >         a
> > $ git diff
> > diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_unix.c b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> > index ad397e2962cf..8a7d5aa4c054 100644
> > --- a/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> > +++ b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
> > @@ -34,8 +34,10 @@ int dump_unix(struct bpf_iter__unix *ctx)
> >
> >         seq = ctx->meta->seq;
> >         seq_num = ctx->meta->seq_num;
> > -       if (seq_num == 0)
> > +       if (seq_num == 0) {
> > +               BPF_SEQ_PRINTF(seq, "% 9c\n", 'a');
> >                 BPF_SEQ_PRINTF(seq, "Num               RefCount Protocol Flags    Type St Inode    Path\n");
> > +       }
> >
> >         BPF_SEQ_PRINTF(seq, "%pK: %08X %08X %08X %04X %02X %8lu",
> >                        unix_sk,
> > ---8<---
> >
> >
> >
> > >
> > > > +                       if (!tmp_buf)
> > > > +                               goto nocopy_fmt;
> > > > +
> > > > +                       if (tmp_buf_end == tmp_buf) {
> > > > +                               err = -ENOSPC;
> > > > +                               goto out;
> > > > +                       }
> > > > +
> > > > +                       *tmp_buf = raw_args[num_spec];
> > > > +                       tmp_buf++;
> > > > +                       num_spec++;
> > > > +
> > > >                         continue;
> > > >                 }
> > > >
> > > > --
> > > > 2.30.2