| Message ID | 20210810092807.13190-3-kuniyu@amazon.co.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | BPF iterator for UNIX domain socket. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 10 of 10 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 6 this patch: 6 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 6 this patch: 6 |
| netdev/header_inline | success | Link |
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>
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 >
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
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
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
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; }
/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(+)