linux-kernel - Re: [PATCH v5 13/15] tools/testing/selftests/bpf: use TASK_COMM_LEN

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEf4BzYTEoDwWzXd91MeMH5Qr9L853Ff3Qq8_wnwfJ8GK0oLnw@mail.gmail.com>
Date:   Thu, 21 Oct 2021 15:44:15 -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@...driver.com, robdclark@...omium.org,
        Christian Brauner <christian@...uner.io>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Ingo Molnar <mingo@...hat.com>, 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>,
        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 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). For others, I'd just
leave hard-coded 16 or re-defined TASK_COMM_LEN_16 where appropriate.

>              -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;

[...]