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
| ||
|
Message-ID: <CAEf4Bzb6AcoiKotHD5Fg1gT3psAC34s54Yo7fmGUZtwcGBW2zQ@mail.gmail.com> Date: Fri, 22 Oct 2021 16:41:26 -0700 From: Andrii Nakryiko <andrii.nakryiko@...il.com> To: Yafang Shao <laoar.shao@...il.com> Cc: Kees Cook <keescook@...omium.org>, Steven Rostedt <rostedt@...dmis.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>, Petr Mladek <pmladek@...e.com>, Peter Ziljstra <peterz@...radead.org>, Al Viro <viro@...iv.linux.org.uk>, Andrew Morton <akpm@...ux-foundation.org>, Valentin Schneider <valentin.schneider@....com>, Qiang Zhang <qiang.zhang@...driver.com>, robdclark <robdclark@...omium.org>, Christian Brauner <christian@...uner.io>, Dietmar Eggemann <dietmar.eggemann@....com>, Ingo Molnar <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>, "David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>, Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>, john fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>, Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>, "linux-perf-use." <linux-perf-users@...r.kernel.org>, linux-fsdevel@...r.kernel.org, open list <linux-kernel@...r.kernel.org>, kernel test robot <oliver.sang@...el.com>, kbuild test robot <lkp@...el.com> Subject: Re: [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16 On Thu, Oct 21, 2021 at 11:36 PM Yafang Shao <laoar.shao@...il.com> wrote: > > On Fri, Oct 22, 2021 at 6:44 AM Andrii Nakryiko > <andrii.nakryiko@...il.com> wrote: > > > > On Wed, Oct 20, 2021 at 8:46 PM Yafang Shao <laoar.shao@...il.com> wrote: > > > > > > The hard-coded 16 is used in various bpf progs. These progs get task > > > comm either via bpf_get_current_comm() or prctl() or > > > bpf_core_read_str(), all of which can work well even if the task comm size > > > is changed. > > > Below is the detailed information, > > > > > > bpf_get_current_comm: > > > progs/test_ringbuf.c > > > progs/test_ringbuf_multi.c > > > > > > prctl: > > > prog_tests/test_overhead.c > > > prog_tests/trampoline_count.c > > > > > > bpf_core_read_str: > > > progs/test_core_reloc_kernel.c > > > progs/test_sk_storage_tracing.c > > > > > > We'd better replace the hard-coded 16 with TASK_COMM_LEN_16 to make it > > > more grepable. > > > > > > Signed-off-by: Yafang Shao <laoar.shao@...il.com> > > > Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com> > > > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@...il.com> > > > Cc: Peter Zijlstra <peterz@...radead.org> > > > Cc: Steven Rostedt <rostedt@...dmis.org> > > > Cc: Kees Cook <keescook@...omium.org> > > > Cc: Al Viro <viro@...iv.linux.org.uk> > > > Cc: Petr Mladek <pmladek@...e.com> > > > --- > > > tools/testing/selftests/bpf/Makefile | 2 +- > > > tools/testing/selftests/bpf/prog_tests/ringbuf.c | 3 ++- > > > tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c | 3 ++- > > > .../testing/selftests/bpf/prog_tests/sk_storage_tracing.c | 3 ++- > > > tools/testing/selftests/bpf/prog_tests/test_overhead.c | 3 ++- > > > tools/testing/selftests/bpf/prog_tests/trampoline_count.c | 3 ++- > > > tools/testing/selftests/bpf/progs/profiler.h | 7 ++++--- > > > tools/testing/selftests/bpf/progs/profiler.inc.h | 8 ++++---- > > > tools/testing/selftests/bpf/progs/pyperf.h | 4 ++-- > > > tools/testing/selftests/bpf/progs/strobemeta.h | 6 +++--- > > > .../testing/selftests/bpf/progs/test_core_reloc_kernel.c | 3 ++- > > > tools/testing/selftests/bpf/progs/test_ringbuf.c | 3 ++- > > > tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 3 ++- > > > .../testing/selftests/bpf/progs/test_sk_storage_tracing.c | 5 +++-- > > > tools/testing/selftests/bpf/progs/test_skb_helpers.c | 5 ++--- > > > tools/testing/selftests/bpf/progs/test_stacktrace_map.c | 5 +++-- > > > tools/testing/selftests/bpf/progs/test_tracepoint.c | 5 +++-- > > > 17 files changed, 41 insertions(+), 30 deletions(-) > > > > > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > > > index 799b88152e9e..5e72d783d3fe 100644 > > > --- a/tools/testing/selftests/bpf/Makefile > > > +++ b/tools/testing/selftests/bpf/Makefile > > > @@ -279,7 +279,7 @@ MENDIAN=$(if $(IS_LITTLE_ENDIAN),-mlittle-endian,-mbig-endian) > > > > > > CLANG_SYS_INCLUDES = $(call get_sys_includes,$(CLANG)) > > > BPF_CFLAGS = -g -D__TARGET_ARCH_$(SRCARCH) $(MENDIAN) \ > > > - -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) \ > > > + -I$(INCLUDE_DIR) -I$(CURDIR) -I$(APIDIR) -I${TOOLSINCDIR} \ > > > > please don't add new include paths unnecessarily. See my comment on > > another patch, if you add those new constants as enums, they will be > > automatically available in vmlinux BTF and thus in auto-generated > > vmlinux.h header (for those programs using it). > > Yes, after converting it to enum, the BPF programs can get it from the > generated vmlinux.h. > > > For others, I'd just > > leave hard-coded 16 or re-defined TASK_COMM_LEN_16 where appropriate. > > > > It seems not all the BPF programs can include the vmlinux.h. > What we really care about here is the copy of task comm should be with > a nul terminator, if we can assure it, then the size used by the BPF > is not important. > I have checked the copy of task comm in all these BPF programs one by > one, and replaced the unsafe bpf_probe_read_kernel() with > bpf_probe_read_kernel_str(), after that change, I think we can leave > hard-coded 16 for the progs which can't include vmlinux.h. SGTM, thanks. > > > > -I$(abspath $(OUTPUT)/../usr/include) > > > > > > CLANG_CFLAGS = $(CLANG_SYS_INCLUDES) \ > > > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c > > > index 4706cee84360..ac82d57c09dc 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c > > > @@ -12,6 +12,7 @@ > > > #include <sys/sysinfo.h> > > > #include <linux/perf_event.h> > > > #include <linux/ring_buffer.h> > > > +#include <linux/sched/task.h> > > > #include "test_ringbuf.lskel.h" > > > > > > #define EDONE 7777 > > > @@ -22,7 +23,7 @@ struct sample { > > > int pid; > > > int seq; > > > long value; > > > - char comm[16]; > > > + char comm[TASK_COMM_LEN_16]; > > > > how much value is in this "grep-ability", really? I'm not convinced > > all this code churn is justified. > > > > > }; > > > > > > static int sample_cnt; > > > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c > > > index 167cd8a2edfd..f0748305ffd6 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c > > > @@ -2,6 +2,7 @@ > > > #define _GNU_SOURCE > > > #include <test_progs.h> > > > #include <sys/epoll.h> > > > +#include <linux/sched/task.h> > > > #include "test_ringbuf_multi.skel.h" > > > > > > static int duration = 0; > > > > [...] > > > -- > Thanks > Yafang
Powered by blists - more mailing lists