| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 30 Jul 2021 09:22:32 -0700
From: Yonghong Song <yhs@...com>
To: Kuniyuki Iwashima <kuniyu@...zon.co.jp>
CC: <andrii@...nel.org>, <ast@...nel.org>, <benh@...zon.com>,
<bpf@...r.kernel.org>, <daniel@...earbox.net>,
<davem@...emloft.net>, <john.fastabend@...il.com>, <kafai@...com>,
<kpsingh@...nel.org>, <kuba@...nel.org>, <kuni1840@...il.com>,
<netdev@...r.kernel.org>, <songliubraving@...com>
Subject: Re: [PATCH bpf-next 2/2] selftest/bpf: Implement sample UNIX domain
On 7/30/21 12:58 AM, Kuniyuki Iwashima wrote:
> From: Yonghong Song <yhs@...com>
> Date: Thu, 29 Jul 2021 23:54:26 -0700
>> On 7/29/21 4:36 PM, Kuniyuki Iwashima wrote:
>>> If there are no abstract sockets, this prog can output the same result
>>> compared to /proc/net/unix.
>>>
>>> # cat /sys/fs/bpf/unix | head -n 2
>>> Num RefCount Protocol Flags Type St Inode Path
>>> ffff9ab7122db000: 00000002 00000000 00010000 0001 01 10623 private/defer
>>>
>>> # cat /proc/net/unix | head -n 2
>>> Num RefCount Protocol Flags Type St Inode Path
>>> ffff9ab7122db000: 00000002 00000000 00010000 0001 01 10623 private/defer
>>>
>>> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.co.jp>
>>> ---
>>> .../selftests/bpf/prog_tests/bpf_iter.c | 17 +++++
>>> .../selftests/bpf/progs/bpf_iter_unix.c | 75 +++++++++++++++++++
>>> 2 files changed, 92 insertions(+)
>>> create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_unix.c
>>>
>>> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
>>> index 1f1aade56504..4746bac68d36 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,20 @@ 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 (CHECK(!skel, "bpf_iter_unix__open_and_load",
>>> + "skeleton open_and_load failed\n"))
>>> + 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 +1270,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_unix.c b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
>>> new file mode 100644
>>> index 000000000000..285ec2f7944d
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_unix.c
>>> @@ -0,0 +1,75 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Copyright Amazon.com Inc. or its affiliates. */
>>> +#include "bpf_iter.h"
>>
>> Could you add bpf_iter__unix to bpf_iter.h similar to bpf_iter__sockmap?
>> The main purpose is to make test tolerating with old vmlinux.h.
>
> Thank you for explanation!
> I've understood why it is needed even when the same struct is defined.
> I'll add it in the next spin.
>
>
>>
>>> +#include "bpf_tracing_net.h"
>>> +#include <bpf/bpf_helpers.h>
>>> +#include <bpf/bpf_endian.h>
>>> +
>>> +char _license[] SEC("license") = "GPL";
>>> +
>>> +#define __SO_ACCEPTCON (1 << 16)
>>> +#define UNIX_HASH_SIZE 256
>>> +#define UNIX_ABSTRACT(unix_sk) (unix_sk->addr->hash < UNIX_HASH_SIZE)
>>
>> Could you add the above three define's in bpf_tracing_net.h?
>> We try to keep all these common defines in a common header for
>> potential reusability.
>
> Will do.
>
>
>>
>>> +
>>> +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))
>>> + /* Abstract UNIX domain socket can contain '\0' in
>>> + * the path, and it should be escaped. However, it
>>> + * requires loops and the BPF verifier rejects it.
>>> + * So here, print only the escaped first byte to
>>> + * indicate it is an abstract UNIX domain socket.
>>> + * (See: unix_seq_show() and commit e7947ea770d0d)
>>> + */
>>> + BPF_SEQ_PRINTF(seq, " @");
>>> + else
>>> + BPF_SEQ_PRINTF(seq, " %s", unix_sk->addr->name->sun_path);
>>> + }
>>
>> I looked at af_unix.c, for the above "if (unix_sk->addr) { ... }" code,
>> the following is the kernel source code,
>>
>> if (u->addr) { // under unix_table_lock here
>> int i, len;
>> seq_putc(seq, ' ');
>>
>> i = 0;
>> len = u->addr->len - sizeof(short);
>> if (!UNIX_ABSTRACT(s))
>> len--;
>> else {
>> seq_putc(seq, '@');
>> i++;
>> }
>> for ( ; i < len; i++)
>> seq_putc(seq, u->addr->name->sun_path[i] ?:
>> '@');
>> }
>>
>> It does not seem to match bpf program non UNIX_ABSTRACT case.
>> I am not familiar with unix socket so it would be good if you can
>> explain a little more.
>
> There is three kinds of unix sockets: pathname, unnamed, abstract. The
> first two terminate the addr with `\0`, but abstract must start with `\0`
> and can contain `\0` anywhere in addr. The `\0` in addr of abstract socket
> does not have special meaning. [1]
>
> They are inserted into the same hash table in unix_bind(), so the bpf prog
> matches all of them.
>
> ``` net/unix/af_unix.c
> 1114 if (sun_path[0])
> 1115 err = unix_bind_bsd(sk, addr);
> 1116 else
> 1117 err = unix_bind_abstract(sk, addr);
> ```
>
> [1]: https://man7.org/linux/man-pages/man7/unix.7.html
>
>
>>
>> For verifier issue with loops, do we have a maximum upper bound for
>> u->addr->len? If yes, does bounded loop work?
>
> That has a maximum length in unix_mkname(): sizeof(struct sockaddr_un).
>
> ``` net/unix/af_unix.c
> 223 /*
> 224 * Check unix socket name:
> 225 * - should be not zero length.
> 226 * - if started by not zero, should be NULL terminated (FS object)
> 227 * - if started by zero, it is abstract name.
> 228 */
> 229
> 230 static int unix_mkname(struct sockaddr_un *sunaddr, int len, unsigned int *hashp)
> 231 {
> ...
> 234 if (len <= sizeof(short) || len > sizeof(*sunaddr))
> 235 return -EINVAL;
> ...
> 253 }
> ```
>
> So, I rewrote the test like this, but it still causes an error.
>
> ```
> if (unix_sk->addr) {
> int i, len;
>
> len = unix_sk->addr->len - sizeof(short);
>
> if (!UNIX_ABSTRACT(unix_sk)) {
> BPF_SEQ_PRINTF(seq, " %s", unix_sk->addr->name->sun_path);
> } else {
> BPF_SEQ_PRINTF(seq, " @");
> i++;
i++ is not useful here and "i" is not initialized earlier.
>
> if (len < sizeof(struct sockaddr_un)) {
> for (i = 1 ; i < len; i++)
> BPF_SEQ_PRINTF(seq, "%c",
> unix_sk->addr->name->sun_path[i] ?:
> '@');
> }
> }
> }
> ```
>
> ```
> processed 196505 insns (limit 1000000) max_states_per_insn 4 total_states 1830 peak_states 1830 mark_read 3
> ```
I did some debugging, the main reason is that llvm compiler used "!="
instead of "<" for "i < len" comparison.
107: b4 05 00 00 08 00 00 00 w5 = 8
108: 85 00 00 00 7e 00 00 00 call 126
; for (i = 1 ; i < len; i++)
109: 07 09 00 00 01 00 00 00 r9 += 1
110: 5d 98 09 00 00 00 00 00 if r8 != r9 goto +9 <LBB0_18>
Considering "len" is not a constant, for verifier, r8 will never be
equal to r9 in the above.
Digging into llvm compilation, it is llvm pass Induction Variable
simplication pass made this code change. I will try to dig more and
find a solution. In your next revision, could you add the above code
as a comment so once llvm code gen is improved, we can have proper
implementation to match /proc/net/unix?
>
>>
>>> +
>>> + BPF_SEQ_PRINTF(seq, "\n");
>>> +
>>> + return 0;
>>> +}
Powered by blists - more mailing lists