This patch set adds BPF iterator support for UNIX domain socket. The first
patch implements it, and the second adds "%c" support for BPF_SEQ_PRINTF().
Changelog:
v5:
- Align header line of bpf_iter_unix.c
- Add test for "%c"
v4:
https://lore.kernel.org/netdev/20210810092807.13190-1-kuniyu@amazon.co.jp/
- 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 (4):
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.
selftest/bpf: Extend the bpf_snprintf() test for "%c".
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 ++++
.../selftests/bpf/prog_tests/snprintf.c | 4 +-
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 +
.../selftests/bpf/progs/test_snprintf.c | 7 +-
10 files changed, 259 insertions(+), 5 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c
--
2.30.2
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 <redacted>
Acked-by: Yonghong Song <redacted>
---
include/linux/btf_ids.h | 3 +-
net/unix/af_unix.c | 93 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 95 insertions(+), 1 deletion(-)
/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 <redacted>
Acked-by: Yonghong Song <redacted>
---
kernel/bpf/helpers.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
The iterator can output almost the same result compared to /proc/net/unix.
The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
can be easily overflown.
# cat /sys/fs/bpf/unix
Num RefCount Protocol Flags Type St Inode Path
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
# cat /proc/net/unix
Num RefCount Protocol Flags Type St Inode Path
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @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 <redacted>
Acked-by: Yonghong Song <redacted>
---
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
@@ -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
@@ -313,6 +314,19 @@ static void test_udp6(void)bpf_iter_udp6__destroy(skel);}+staticvoidtest_unix(void)+{+structbpf_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 */staticintdo_read_with_fd(intiter_fd,constchar*expected,boolread_one_char)
@@ -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";++staticlongsock_i_ino(conststructsock*sk)+{+conststructsocket*sk_socket=sk->sk_socket;+conststructinode*inode;+unsignedlongino;++if(!sk_socket)+return0;++inode=&container_of(sk_socket,structsocket_alloc,socket)->vfs_inode;+bpf_probe_read_kernel(&ino,sizeof(ino),&inode->i_ino);+returnino;+}++SEC("iter/unix")+intdump_unix(structbpf_iter__unix*ctx)+{+structunix_sock*unix_sk=ctx->unix_sk;+structsock*sk=(structsock*)unix_sk;+structseq_file*seq;+__u32seq_num;++if(!unix_sk)+return0;++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 %8lu",+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'andcancontain'\0'.Thenullbytes+*shouldbeescapedasdoneinunix_seq_show().+*/+inti,len;++len=unix_sk->addr->len-sizeof(short);++BPF_SEQ_PRINTF(seq," @");++/* unix_mkname() tests this upper bound. */+if(len<sizeof(structsockaddr_un))+for(i=1;i<len;i++)+BPF_SEQ_PRINTF(seq,"%c",+unix_sk->addr->name->sun_path[i]?:+'@');+}+}++BPF_SEQ_PRINTF(seq,"\n");++return0;+}
This patch adds a "positive" pattern for "%c", which intentionally uses a
__u32 value (0x64636261, "dbca") to print a single character "a". If the
implementation went wrong, other 3 bytes might show up as the part of the
latter "%+05s".
Also, this patch adds two "negative" patterns for wide character.
Signed-off-by: Kuniyuki Iwashima <redacted>
---
tools/testing/selftests/bpf/prog_tests/snprintf.c | 4 +++-
tools/testing/selftests/bpf/progs/test_snprintf.c | 7 ++++---
2 files changed, 7 insertions(+), 4 deletions(-)
On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima [off-list ref] wrote:
The iterator can output almost the same result compared to /proc/net/unix.
The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
can be easily overflown.
# cat /sys/fs/bpf/unix
Num RefCount Protocol Flags Type St Inode Path
It's totally my OCD, but why the column name is not aligned with
values? I mean the "Inode" column. It's left aligned, but values
(numbers) are right-aligned? I'd fix that while applying, but I can't
apply due to selftests failures, so please take a look.
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
# cat /proc/net/unix
Num RefCount Protocol Flags Type St Inode Path
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @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 <redacted>
Acked-by: Yonghong Song <redacted>
---
This selftests breaks test_progs-no_alu32 ([0], the error log is super
long and can freeze browser; it looks like an infinite loop and BPF
verifier just keeps reporting it until it runs out of 1mln
instructions or something). Please check what's going on there, I
can't land it as it is right now.
[0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
+ /* 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;
+
no_alu32 variant probably isn't happy about using int for this, it
probably does << 32, >> 32 dance and loses track of actual value in
the loop. You can try using u64 instead.
+ 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++)
if you move above if inside the loop to break out of the loop, does it
change how Clang generates code?
for (i = 1; i < len i++) {
if (i >= sizeof(struct sockaddr_un))
break;
BPF_SEQ_PRINTF(...);
}
On Thu, Aug 12, 2021 at 9:47 AM Kuniyuki Iwashima [off-list ref] wrote:
quoted hunk
This patch adds a "positive" pattern for "%c", which intentionally uses a
__u32 value (0x64636261, "dbca") to print a single character "a". If the
implementation went wrong, other 3 bytes might show up as the part of the
latter "%+05s".
Also, this patch adds two "negative" patterns for wide character.
Signed-off-by: Kuniyuki Iwashima <redacted>
---
tools/testing/selftests/bpf/prog_tests/snprintf.c | 4 +++-
tools/testing/selftests/bpf/progs/test_snprintf.c | 7 ++++---
2 files changed, 7 insertions(+), 4 deletions(-)
@@ -40,6 +40,7 @@ int handler(const void *ctx)/* Convenient values to pretty-print */const__u8ex_ipv4[]={127,0,0,1};const__u8ex_ipv6[]={0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1};+const__u32chr1=0x64636261;/* dcba */staticconstcharstr1[]="str1";staticconstcharlongstr[]="longstr";
@@ -59,9 +60,9 @@ int handler(const void *ctx)/* Kernel pointers */addr_ret=BPF_SNPRINTF(addr_out,sizeof(addr_out),"%pK %px %p",0,0xFFFF00000ADD4E55,0xFFFF00000ADD4E55);-/* Strings embedding */-str_ret=BPF_SNPRINTF(str_out,sizeof(str_out),"%s %+05s",-str1,longstr);+/* Strings and single-byte character embedding */+str_ret=BPF_SNPRINTF(str_out,sizeof(str_out),"%s % 9c %+05s",+str1,chr1,longstr);
Why this hackery with __u32? You are making an endianness assumption
(it will break on big-endian), and you'd never write real code like
that. Just pass 'a', what's wrong with that?
On Fri, Aug 13, 2021 at 4:28 PM Andrii Nakryiko
[off-list ref] wrote:
On Thu, Aug 12, 2021 at 9:47 AM Kuniyuki Iwashima [off-list ref] wrote:
quoted
This patch adds a "positive" pattern for "%c", which intentionally uses a
__u32 value (0x64636261, "dbca") to print a single character "a". If the
implementation went wrong, other 3 bytes might show up as the part of the
latter "%+05s".
Also, this patch adds two "negative" patterns for wide character.
Signed-off-by: Kuniyuki Iwashima <redacted>
---
tools/testing/selftests/bpf/prog_tests/snprintf.c | 4 +++-
tools/testing/selftests/bpf/progs/test_snprintf.c | 7 ++++---
2 files changed, 7 insertions(+), 4 deletions(-)
@@ -40,6 +40,7 @@ int handler(const void *ctx)/* Convenient values to pretty-print */const__u8ex_ipv4[]={127,0,0,1};const__u8ex_ipv6[]={0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1};+const__u32chr1=0x64636261;/* dcba */staticconstcharstr1[]="str1";staticconstcharlongstr[]="longstr";
@@ -59,9 +60,9 @@ int handler(const void *ctx)/* Kernel pointers */addr_ret=BPF_SNPRINTF(addr_out,sizeof(addr_out),"%pK %px %p",0,0xFFFF00000ADD4E55,0xFFFF00000ADD4E55);-/* Strings embedding */-str_ret=BPF_SNPRINTF(str_out,sizeof(str_out),"%s %+05s",-str1,longstr);+/* Strings and single-byte character embedding */+str_ret=BPF_SNPRINTF(str_out,sizeof(str_out),"%s % 9c %+05s",+str1,chr1,longstr);
Can you also add tests for %+2c, %-3c, %04c, %0c? Think outside the box ;)
Why this hackery with __u32? You are making an endianness assumption
(it will break on big-endian), and you'd never write real code like
that. Just pass 'a', what's wrong with that?
On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima [off-list ref] wrote:
quoted
The iterator can output almost the same result compared to /proc/net/unix.
The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
can be easily overflown.
# cat /sys/fs/bpf/unix
Num RefCount Protocol Flags Type St Inode Path
It's totally my OCD, but why the column name is not aligned with
values? I mean the "Inode" column. It's left aligned, but values
(numbers) are right-aligned? I'd fix that while applying, but I can't
apply due to selftests failures, so please take a look.
Ah, honestly, I've felt something strange about the column... will fix it!
quoted
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
# cat /proc/net/unix
Num RefCount Protocol Flags Type St Inode Path
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @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 <redacted>
Acked-by: Yonghong Song <redacted>
---
This selftests breaks test_progs-no_alu32 ([0], the error log is super
long and can freeze browser; it looks like an infinite loop and BPF
verifier just keeps reporting it until it runs out of 1mln
instructions or something). Please check what's going on there, I
can't land it as it is right now.
[0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
+ /* 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;
+
no_alu32 variant probably isn't happy about using int for this, it
probably does << 32, >> 32 dance and loses track of actual value in
the loop. You can try using u64 instead.
Sorry, I missed the no_alu32 test.
Changing int to __u64 fixed the error, thanks!
quoted
+ 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++)
if you move above if inside the loop to break out of the loop, does it
change how Clang generates code?
for (i = 1; i < len i++) {
if (i >= sizeof(struct sockaddr_un))
break;
BPF_SEQ_PRINTF(...);
}
Yes, but there seems little defference.
Which is preferable?
---8<---
before (for inside if) <- -> after (if inside loop)
96: 07 08 00 00 fe ff ff ff r8 += -2 | ; for (i = 1; i < len; i++) {
; if (len < sizeof(struct sockaddr_un)) | 97: bf 81 00 00 00 00 00 00 r1 = r8
97: 25 08 10 00 6d 00 00 00 if r8 > 109 goto +16 <LBB0_21> | 98: 07 01 00 00 fc ff ff ff r1 += -4
; for (i = 1; i < len; i++) | 99: 25 01 12 00 6b 00 00 00 if r1 > 107 goto +18 <LBB0_21>
98: a5 08 0f 00 02 00 00 00 if r8 < 2 goto +15 <LBB0_21> | 100: 07 08 00 00 fe ff ff ff r8 += -2
99: b7 09 00 00 01 00 00 00 r9 = 1 | 101: b7 09 00 00 01 00 00 00 r9 = 1
100: 05 00 16 00 00 00 00 00 goto +22 <LBB0_18> | 102: b7 06 00 00 02 00 00 00 r6 = 2
| 103: 05 00 17 00 00 00 00 00 goto +23 <LBB0_17>
...
111: 85 00 00 00 7e 00 00 00 call 126 | 113: b4 05 00 00 08 00 00 00 w5 = 8
; for (i = 1; i < len; i++) | 114: 85 00 00 00 7e 00 00 00 call 126
112: 07 09 00 00 01 00 00 00 r9 += 1 | ; for (i = 1; i < len; i++) {
113: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_18> | 115: 25 08 02 00 6d 00 00 00 if r8 > 109 goto +2 <LBB0_21>
> 116: 07 09 00 00 01 00 00 00 r9 += 1
> ; for (i = 1; i < len; i++) {
> 117: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_17>
---8<---
On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima [off-list ref] wrote:
quoted
The iterator can output almost the same result compared to /proc/net/unix.
The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
can be easily overflown.
# cat /sys/fs/bpf/unix
Num RefCount Protocol Flags Type St Inode Path
It's totally my OCD, but why the column name is not aligned with
values? I mean the "Inode" column. It's left aligned, but values
(numbers) are right-aligned? I'd fix that while applying, but I can't
apply due to selftests failures, so please take a look.
Ah, honestly, I've felt something strange about the column... will fix it!
quoted
quoted
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
# cat /proc/net/unix
Num RefCount Protocol Flags Type St Inode Path
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @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 <redacted>
Acked-by: Yonghong Song <redacted>
---
This selftests breaks test_progs-no_alu32 ([0], the error log is super
long and can freeze browser; it looks like an infinite loop and BPF
verifier just keeps reporting it until it runs out of 1mln
instructions or something). Please check what's going on there, I
can't land it as it is right now.
[0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
+ /* 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;
+
no_alu32 variant probably isn't happy about using int for this, it
probably does << 32, >> 32 dance and loses track of actual value in
the loop. You can try using u64 instead.
Sorry, I missed the no_alu32 test.
Changing int to __u64 fixed the error, thanks!
quoted
quoted
+ 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++)
if you move above if inside the loop to break out of the loop, does it
change how Clang generates code?
for (i = 1; i < len i++) {
if (i >= sizeof(struct sockaddr_un))
break;
BPF_SEQ_PRINTF(...);
}
Yes, but there seems little defference.
Which is preferable?
---8<---
before (for inside if) <- -> after (if inside loop)
96: 07 08 00 00 fe ff ff ff r8 += -2 | ; for (i = 1; i < len; i++) {
; if (len < sizeof(struct sockaddr_un)) | 97: bf 81 00 00 00 00 00 00 r1 = r8
97: 25 08 10 00 6d 00 00 00 if r8 > 109 goto +16 <LBB0_21> | 98: 07 01 00 00 fc ff ff ff r1 += -4
; for (i = 1; i < len; i++) | 99: 25 01 12 00 6b 00 00 00 if r1 > 107 goto +18 <LBB0_21>
98: a5 08 0f 00 02 00 00 00 if r8 < 2 goto +15 <LBB0_21> | 100: 07 08 00 00 fe ff ff ff r8 += -2
99: b7 09 00 00 01 00 00 00 r9 = 1 | 101: b7 09 00 00 01 00 00 00 r9 = 1
100: 05 00 16 00 00 00 00 00 goto +22 <LBB0_18> | 102: b7 06 00 00 02 00 00 00 r6 = 2
| 103: 05 00 17 00 00 00 00 00 goto +23 <LBB0_17>
...
111: 85 00 00 00 7e 00 00 00 call 126 | 113: b4 05 00 00 08 00 00 00 w5 = 8
; for (i = 1; i < len; i++) | 114: 85 00 00 00 7e 00 00 00 call 126
112: 07 09 00 00 01 00 00 00 r9 += 1 | ; for (i = 1; i < len; i++) {
113: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_18> | 115: 25 08 02 00 6d 00 00 00 if r8 > 109 goto +2 <LBB0_21>
> 116: 07 09 00 00 01 00 00 00 r9 += 1
> ; for (i = 1; i < len; i++) {
> 117: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_17>
---8<---
Have you tried running the variant I proposed on Clang without
Yonghong's recent fix? I wonder if it works without that fix (not that
there is anything wrong about the fix, but if we can avoid depending
on it, it would be great).
On Fri, Aug 13, 2021 at 4:28 PM Andrii Nakryiko
[off-list ref] wrote:
quoted
On Thu, Aug 12, 2021 at 9:47 AM Kuniyuki Iwashima [off-list ref] wrote:
quoted
This patch adds a "positive" pattern for "%c", which intentionally uses a
__u32 value (0x64636261, "dbca") to print a single character "a". If the
implementation went wrong, other 3 bytes might show up as the part of the
latter "%+05s".
Also, this patch adds two "negative" patterns for wide character.
Signed-off-by: Kuniyuki Iwashima <redacted>
---
tools/testing/selftests/bpf/prog_tests/snprintf.c | 4 +++-
tools/testing/selftests/bpf/progs/test_snprintf.c | 7 ++++---
2 files changed, 7 insertions(+), 4 deletions(-)
@@ -40,6 +40,7 @@ int handler(const void *ctx)/* Convenient values to pretty-print */const__u8ex_ipv4[]={127,0,0,1};const__u8ex_ipv6[]={0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1};+const__u32chr1=0x64636261;/* dcba */staticconstcharstr1[]="str1";staticconstcharlongstr[]="longstr";
@@ -59,9 +60,9 @@ int handler(const void *ctx)/* Kernel pointers */addr_ret=BPF_SNPRINTF(addr_out,sizeof(addr_out),"%pK %px %p",0,0xFFFF00000ADD4E55,0xFFFF00000ADD4E55);-/* Strings embedding */-str_ret=BPF_SNPRINTF(str_out,sizeof(str_out),"%s %+05s",-str1,longstr);+/* Strings and single-byte character embedding */+str_ret=BPF_SNPRINTF(str_out,sizeof(str_out),"%s % 9c %+05s",+str1,chr1,longstr);
Can you also add tests for %+2c, %-3c, %04c, %0c? Think outside the box ;)
Sure.
quoted
Why this hackery with __u32? You are making an endianness assumption
(it will break on big-endian), and you'd never write real code like
that. Just pass 'a', what's wrong with that?
In my first implementation of "%c" support, I tried to support "%lc" and
"%llc" also and reused the later int code. Then just testing 'a' was ok,
but it was wrong with the 0x64636261, three bytes of which showed up as
part of the next %s. So, I thought it would be better to test with int.
But exactly it breaks the big-endian case, I'll just pass 'a'.
On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima [off-list ref] wrote:
quoted
The iterator can output almost the same result compared to /proc/net/unix.
The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
can be easily overflown.
# cat /sys/fs/bpf/unix
Num RefCount Protocol Flags Type St Inode Path
It's totally my OCD, but why the column name is not aligned with
values? I mean the "Inode" column. It's left aligned, but values
(numbers) are right-aligned? I'd fix that while applying, but I can't
apply due to selftests failures, so please take a look.
Ah, honestly, I've felt something strange about the column... will fix it!
quoted
quoted
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
# cat /proc/net/unix
Num RefCount Protocol Flags Type St Inode Path
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @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 <redacted>
Acked-by: Yonghong Song <redacted>
---
This selftests breaks test_progs-no_alu32 ([0], the error log is super
long and can freeze browser; it looks like an infinite loop and BPF
verifier just keeps reporting it until it runs out of 1mln
instructions or something). Please check what's going on there, I
can't land it as it is right now.
[0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
+ /* 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;
+
no_alu32 variant probably isn't happy about using int for this, it
probably does << 32, >> 32 dance and loses track of actual value in
the loop. You can try using u64 instead.
Sorry, I missed the no_alu32 test.
Changing int to __u64 fixed the error, thanks!
quoted
quoted
+ 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++)
if you move above if inside the loop to break out of the loop, does it
change how Clang generates code?
for (i = 1; i < len i++) {
if (i >= sizeof(struct sockaddr_un))
break;
BPF_SEQ_PRINTF(...);
}
Yes, but there seems little defference.
Which is preferable?
---8<---
before (for inside if) <- -> after (if inside loop)
96: 07 08 00 00 fe ff ff ff r8 += -2 | ; for (i = 1; i < len; i++) {
; if (len < sizeof(struct sockaddr_un)) | 97: bf 81 00 00 00 00 00 00 r1 = r8
97: 25 08 10 00 6d 00 00 00 if r8 > 109 goto +16 <LBB0_21> | 98: 07 01 00 00 fc ff ff ff r1 += -4
; for (i = 1; i < len; i++) | 99: 25 01 12 00 6b 00 00 00 if r1 > 107 goto +18 <LBB0_21>
98: a5 08 0f 00 02 00 00 00 if r8 < 2 goto +15 <LBB0_21> | 100: 07 08 00 00 fe ff ff ff r8 += -2
99: b7 09 00 00 01 00 00 00 r9 = 1 | 101: b7 09 00 00 01 00 00 00 r9 = 1
100: 05 00 16 00 00 00 00 00 goto +22 <LBB0_18> | 102: b7 06 00 00 02 00 00 00 r6 = 2
| 103: 05 00 17 00 00 00 00 00 goto +23 <LBB0_17>
...
111: 85 00 00 00 7e 00 00 00 call 126 | 113: b4 05 00 00 08 00 00 00 w5 = 8
; for (i = 1; i < len; i++) | 114: 85 00 00 00 7e 00 00 00 call 126
112: 07 09 00 00 01 00 00 00 r9 += 1 | ; for (i = 1; i < len; i++) {
113: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_18> | 115: 25 08 02 00 6d 00 00 00 if r8 > 109 goto +2 <LBB0_21>
> 116: 07 09 00 00 01 00 00 00 r9 += 1
> ; for (i = 1; i < len; i++) {
> 117: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_17>
---8<---
Have you tried running the variant I proposed on Clang without
Yonghong's recent fix? I wonder if it works without that fix (not that
there is anything wrong about the fix, but if we can avoid depending
on it, it would be great).
It was with the fix.
I rebuilt LLVM without the fix, then the if-inside-for code worked well :)
There was no transformation from '<' to '!='.
I'll drop the change in README and respin with your suggestion soon.
---8<---
; for (i = 1; i < len; i++) {
97: bf 81 00 00 00 00 00 00 r1 = r8
98: 07 01 00 00 fc ff ff ff r1 += -4
99: 25 01 12 00 6b 00 00 00 if r1 > 107 goto +18 <LBB0_21>
100: 07 08 00 00 fe ff ff ff r8 += -2
101: b7 09 00 00 01 00 00 00 r9 = 1
102: b7 06 00 00 02 00 00 00 r6 = 2
103: 05 00 17 00 00 00 00 00 goto +23 <LBB0_17>
...
; for (i = 1; i < len; i++) {
115: 25 08 02 00 6d 00 00 00 if r8 > 109 goto +2 <LBB0_21>
116: 07 09 00 00 01 00 00 00 r9 += 1
; for (i = 1; i < len; i++) {
117: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_17>
---8<---
On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima [off-list ref] wrote:
quoted
The iterator can output almost the same result compared to /proc/net/unix.
The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
can be easily overflown.
# cat /sys/fs/bpf/unix
Num RefCount Protocol Flags Type St Inode Path
It's totally my OCD, but why the column name is not aligned with
values? I mean the "Inode" column. It's left aligned, but values
(numbers) are right-aligned? I'd fix that while applying, but I can't
apply due to selftests failures, so please take a look.
Ah, honestly, I've felt something strange about the column... will fix it!
quoted
quoted
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
# cat /proc/net/unix
Num RefCount Protocol Flags Type St Inode Path
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @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 <redacted>
Acked-by: Yonghong Song <redacted>
---
This selftests breaks test_progs-no_alu32 ([0], the error log is super
long and can freeze browser; it looks like an infinite loop and BPF
verifier just keeps reporting it until it runs out of 1mln
instructions or something). Please check what's going on there, I
can't land it as it is right now.
[0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
+ /* 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;
+
no_alu32 variant probably isn't happy about using int for this, it
probably does << 32, >> 32 dance and loses track of actual value in
the loop. You can try using u64 instead.
Sorry, I missed the no_alu32 test.
Changing int to __u64 fixed the error, thanks!
Indeed for no_alu32, the index has << 32 and >> 32, which makes
verifier *equivalent* register tracking not effective, see below:
96: r1 = r8
97: r1 <<= 32
98: r2 = r1
99: r2 >>= 32
100: if r2 > 109 goto +19 <LBB0_21>
101: r1 s>>= 32
102: if r1 s< 2 goto +17 <LBB0_21>
103: r9 = 1
104: r8 <<= 32
105: r8 >>= 32
Because these shifting, r1/r2/r8 equivalence cannot be
easily established, so verifier ends with conservative
r8 and cannot verify program successfully.
Using __u64 for 'i' and 'len', the upper bound is directly
tested:
98: if r8 > 109 goto +16 <LBB0_21>
99: if r8 < 2 goto +15 <LBB0_21>
and verifier is very happy with this.
quoted
quoted
+ 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++)
if you move above if inside the loop to break out of the loop, does it
change how Clang generates code?
for (i = 1; i < len i++) {
if (i >= sizeof(struct sockaddr_un))
break;
BPF_SEQ_PRINTF(...);
}
Yes, but there seems little defference.
Which is preferable?
---8<---
before (for inside if) <- -> after (if inside loop)
96: 07 08 00 00 fe ff ff ff r8 += -2 | ; for (i = 1; i < len; i++) {
; if (len < sizeof(struct sockaddr_un)) | 97: bf 81 00 00 00 00 00 00 r1 = r8
97: 25 08 10 00 6d 00 00 00 if r8 > 109 goto +16 <LBB0_21> | 98: 07 01 00 00 fc ff ff ff r1 += -4
; for (i = 1; i < len; i++) | 99: 25 01 12 00 6b 00 00 00 if r1 > 107 goto +18 <LBB0_21>
98: a5 08 0f 00 02 00 00 00 if r8 < 2 goto +15 <LBB0_21> | 100: 07 08 00 00 fe ff ff ff r8 += -2
99: b7 09 00 00 01 00 00 00 r9 = 1 | 101: b7 09 00 00 01 00 00 00 r9 = 1
100: 05 00 16 00 00 00 00 00 goto +22 <LBB0_18> | 102: b7 06 00 00 02 00 00 00 r6 = 2
| 103: 05 00 17 00 00 00 00 00 goto +23 <LBB0_17>
...
111: 85 00 00 00 7e 00 00 00 call 126 | 113: b4 05 00 00 08 00 00 00 w5 = 8
; for (i = 1; i < len; i++) | 114: 85 00 00 00 7e 00 00 00 call 126
112: 07 09 00 00 01 00 00 00 r9 += 1 | ; for (i = 1; i < len; i++) {
113: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_18> | 115: 25 08 02 00 6d 00 00 00 if r8 > 109 goto +2 <LBB0_21>
> 116: 07 09 00 00 01 00 00 00 r9 += 1
> ; for (i = 1; i < len; i++) {
> 117: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_17>
---8<---
On Thu, Aug 12, 2021 at 9:46 AM Kuniyuki Iwashima [off-list ref] wrote:
quoted
The iterator can output almost the same result compared to /proc/net/unix.
The header line is aligned, and the Inode column uses "%8lu" because "%5lu"
can be easily overflown.
# cat /sys/fs/bpf/unix
Num RefCount Protocol Flags Type St Inode Path
It's totally my OCD, but why the column name is not aligned with
values? I mean the "Inode" column. It's left aligned, but values
(numbers) are right-aligned? I'd fix that while applying, but I can't
apply due to selftests failures, so please take a look.
Ah, honestly, I've felt something strange about the column... will fix it!
quoted
quoted
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @Hello@World@
# cat /proc/net/unix
Num RefCount Protocol Flags Type St Inode Path
ffff963c06689800: 00000002 00000000 00010000 0001 01 18697 private/defer
ffff963c7c979c00: 00000002 00000000 00000000 0001 01 598245 @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 <redacted>
Acked-by: Yonghong Song <redacted>
---
This selftests breaks test_progs-no_alu32 ([0], the error log is super
long and can freeze browser; it looks like an infinite loop and BPF
verifier just keeps reporting it until it runs out of 1mln
instructions or something). Please check what's going on there, I
can't land it as it is right now.
[0] https://github.com/kernel-patches/bpf/runs/3326071112?check_suite_focus=true#step:6:124288
+ /* 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;
+
no_alu32 variant probably isn't happy about using int for this, it
probably does << 32, >> 32 dance and loses track of actual value in
the loop. You can try using u64 instead.
Sorry, I missed the no_alu32 test.
Changing int to __u64 fixed the error, thanks!
Indeed for no_alu32, the index has << 32 and >> 32, which makes
verifier *equivalent* register tracking not effective, see below:
96: r1 = r8
97: r1 <<= 32
98: r2 = r1
99: r2 >>= 32
100: if r2 > 109 goto +19 <LBB0_21>
101: r1 s>>= 32
102: if r1 s< 2 goto +17 <LBB0_21>
103: r9 = 1
104: r8 <<= 32
105: r8 >>= 32
Because these shifting, r1/r2/r8 equivalence cannot be
easily established, so verifier ends with conservative
r8 and cannot verify program successfully.
Using __u64 for 'i' and 'len', the upper bound is directly
tested:
98: if r8 > 109 goto +16 <LBB0_21>
99: if r8 < 2 goto +15 <LBB0_21>
and verifier is very happy with this.
Thanks for explanation!
I understand that the shift dance is to mimic the overflow of int because
actually 64-bit register is allocated to 'i' and 32-bit operations cannot
be used in no_alu32 test, so using __64 to remove the dance resolves it.
quoted
quoted
quoted
+ 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++)
if you move above if inside the loop to break out of the loop, does it
change how Clang generates code?
for (i = 1; i < len i++) {
if (i >= sizeof(struct sockaddr_un))
break;
BPF_SEQ_PRINTF(...);
}
Yes, but there seems little defference.
Which is preferable?
---8<---
before (for inside if) <- -> after (if inside loop)
96: 07 08 00 00 fe ff ff ff r8 += -2 | ; for (i = 1; i < len; i++) {
; if (len < sizeof(struct sockaddr_un)) | 97: bf 81 00 00 00 00 00 00 r1 = r8
97: 25 08 10 00 6d 00 00 00 if r8 > 109 goto +16 <LBB0_21> | 98: 07 01 00 00 fc ff ff ff r1 += -4
; for (i = 1; i < len; i++) | 99: 25 01 12 00 6b 00 00 00 if r1 > 107 goto +18 <LBB0_21>
98: a5 08 0f 00 02 00 00 00 if r8 < 2 goto +15 <LBB0_21> | 100: 07 08 00 00 fe ff ff ff r8 += -2
99: b7 09 00 00 01 00 00 00 r9 = 1 | 101: b7 09 00 00 01 00 00 00 r9 = 1
100: 05 00 16 00 00 00 00 00 goto +22 <LBB0_18> | 102: b7 06 00 00 02 00 00 00 r6 = 2
| 103: 05 00 17 00 00 00 00 00 goto +23 <LBB0_17>
...
111: 85 00 00 00 7e 00 00 00 call 126 | 113: b4 05 00 00 08 00 00 00 w5 = 8
; for (i = 1; i < len; i++) | 114: 85 00 00 00 7e 00 00 00 call 126
112: 07 09 00 00 01 00 00 00 r9 += 1 | ; for (i = 1; i < len; i++) {
113: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_18> | 115: 25 08 02 00 6d 00 00 00 if r8 > 109 goto +2 <LBB0_21>
> 116: 07 09 00 00 01 00 00 00 r9 += 1
> ; for (i = 1; i < len; i++) {
> 117: ad 89 09 00 00 00 00 00 if r9 < r8 goto +9 <LBB0_17>
---8<---