| Message ID | 20211021034516.4400-1-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | extend task comm from 16 to 24 for CONFIG_BASE_FULL | expand |
On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao <laoar.shao@gmail.com> wrote: > This patchset changes files among many subsystems. I don't know which > tree it should be applied to, so I just base it on Linus's tree. I can do that ;) > There're many truncated kthreads in the kernel, which may make trouble > for the user, for example, the user can't get detailed device > information from the task comm. That sucked of us. > This patchset tries to improve this problem fundamentally by extending > the task comm size from 16 to 24. In order to do that, we have to do > some cleanups first. It's at v5 and there's no evidence of review activity? C'mon, folks! > 1. Make the copy of task comm always safe no matter what the task > comm size is. For example, > > Unsafe Safe > strlcpy strscpy_pad > strncpy strscpy_pad > bpf_probe_read_kernel bpf_probe_read_kernel_str > bpf_core_read_str > bpf_get_current_comm > perf_event__prepare_comm > prctl(2) > > 2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to > make it more grepable. > > 3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it > as 16 for CONFIG_BASE_SMALL. Is this justified? How much simpler/more reliable/more maintainable/ would the code be if we were to make CONFIG_BASE_SMALL suffer with the extra 8 bytes? > 4. Print a warning if the kthread comm is still truncated.
On Thu, Oct 21, 2021 at 08:52:22PM -0700, Andrew Morton wrote: > On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao <laoar.shao@gmail.com> wrote: > > > This patchset changes files among many subsystems. I don't know which > > tree it should be applied to, so I just base it on Linus's tree. > > I can do that ;) > > > There're many truncated kthreads in the kernel, which may make trouble > > for the user, for example, the user can't get detailed device > > information from the task comm. > > That sucked of us. > > > This patchset tries to improve this problem fundamentally by extending > > the task comm size from 16 to 24. In order to do that, we have to do > > some cleanups first. > > It's at v5 and there's no evidence of review activity? C'mon, folks! It's on my list! :) It's a pretty subtle area that rarely changes, so I want to make sure I'm a full coffee to do the review. :) > > 1. Make the copy of task comm always safe no matter what the task > > comm size is. For example, > > > > Unsafe Safe > > strlcpy strscpy_pad > > strncpy strscpy_pad > > bpf_probe_read_kernel bpf_probe_read_kernel_str > > bpf_core_read_str > > bpf_get_current_comm > > perf_event__prepare_comm > > prctl(2) > > > > 2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to > > make it more grepable. > > > > 3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it > > as 16 for CONFIG_BASE_SMALL. > > Is this justified? How much simpler/more reliable/more maintainable/ > would the code be if we were to make CONFIG_BASE_SMALL suffer with the > extra 8 bytes? Does anyone "own" CONFIG_BASE_SMALL? Gonna go with "no": $ git ann init/Kconfig| grep 'config BASE_SMALL' 1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 2054)config BASE_SMALL And it looks mostly unused: $ git grep CONFIG_BASE_SMALL | cut -d: -f1 | sort -u | xargs -n1 git ann -f | grep 'CONFIG_BASE_SMALL' b2af018ff26f1 (Ingo Molnar 2009-01-28 17:36:56 +0100 18)#if CONFIG_BASE_SMALL == 0 fcdba07ee390d ( Jiri Olsa 2011-02-07 19:31:25 +0100 54)#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE) Blaming lines: 100% (46/46), done. 1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 28)#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000) 1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 34)#define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \ Blaming lines: 100% (162/162), done. f86dcc5aa8c79 (Eric Dumazet 2009-10-07 00:37:59 +0000 31)#define UDP_HTABLE_SIZE_MIN (CONFIG_BASE_SMALL ? 128 : 256) 02c02bf12c5d8 (Matthew Wilcox 2017-11-03 23:09:45 -0400 1110)#define XA_CHUNK_SHIFT (CONFIG_BASE_SMALL ? 4 : 6) a52b89ebb6d44 (Davidlohr Bueso 2014-01-12 15:31:23 -0800 4249)#if CONFIG_BASE_SMALL 7b44ab978b77a (Eric W. Biederman 2011-11-16 23:20:58 -0800 78)#define UIDHASH_BITS (CONFIG_BASE_SMALL ? 3 : 7)
On Fri, Oct 22, 2021 at 12:00 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Oct 21, 2021 at 08:52:22PM -0700, Andrew Morton wrote: > > On Thu, 21 Oct 2021 03:45:07 +0000 Yafang Shao <laoar.shao@gmail.com> wrote: > > > > > This patchset changes files among many subsystems. I don't know which > > > tree it should be applied to, so I just base it on Linus's tree. > > > > I can do that ;) > > > > > There're many truncated kthreads in the kernel, which may make trouble > > > for the user, for example, the user can't get detailed device > > > information from the task comm. > > > > That sucked of us. > > > > > This patchset tries to improve this problem fundamentally by extending > > > the task comm size from 16 to 24. In order to do that, we have to do > > > some cleanups first. > > > > It's at v5 and there's no evidence of review activity? C'mon, folks! > > It's on my list! :) It's a pretty subtle area that rarely changes, so I > want to make sure I'm a full coffee to do the review. :) > > > > 1. Make the copy of task comm always safe no matter what the task > > > comm size is. For example, > > > > > > Unsafe Safe > > > strlcpy strscpy_pad > > > strncpy strscpy_pad > > > bpf_probe_read_kernel bpf_probe_read_kernel_str > > > bpf_core_read_str > > > bpf_get_current_comm > > > perf_event__prepare_comm > > > prctl(2) > > > > > > 2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to > > > make it more grepable. > > > > > > 3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it > > > as 16 for CONFIG_BASE_SMALL. > > > > Is this justified? How much simpler/more reliable/more maintainable/ > > would the code be if we were to make CONFIG_BASE_SMALL suffer with the > > extra 8 bytes? > > Does anyone "own" CONFIG_BASE_SMALL? Gonna go with "no": > > $ git ann init/Kconfig| grep 'config BASE_SMALL' > 1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 2054)config BASE_SMALL > > And it looks mostly unused: > > $ git grep CONFIG_BASE_SMALL | cut -d: -f1 | sort -u | xargs -n1 git ann -f | grep 'CONFIG_BASE_SMALL' > b2af018ff26f1 (Ingo Molnar 2009-01-28 17:36:56 +0100 18)#if CONFIG_BASE_SMALL == 0 > fcdba07ee390d ( Jiri Olsa 2011-02-07 19:31:25 +0100 54)#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE) > Blaming lines: 100% (46/46), done. > 1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 28)#define PID_MAX_DEFAULT (CONFIG_BASE_SMALL ? 0x1000 : 0x8000) > 1da177e4c3f41 (Linus Torvalds 2005-04-16 15:20:36 -0700 34)#define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \ > Blaming lines: 100% (162/162), done. > f86dcc5aa8c79 (Eric Dumazet 2009-10-07 00:37:59 +0000 31)#define UDP_HTABLE_SIZE_MIN (CONFIG_BASE_SMALL ? 128 : 256) > 02c02bf12c5d8 (Matthew Wilcox 2017-11-03 23:09:45 -0400 1110)#define XA_CHUNK_SHIFT (CONFIG_BASE_SMALL ? 4 : 6) > a52b89ebb6d44 (Davidlohr Bueso 2014-01-12 15:31:23 -0800 4249)#if CONFIG_BASE_SMALL > 7b44ab978b77a (Eric W. Biederman 2011-11-16 23:20:58 -0800 78)#define UIDHASH_BITS (CONFIG_BASE_SMALL ? 3 : 7) > > > -- Right. CONFIG_BASE_SMALL is seldomly used in the kernel. As you have already removed 64 bytes from task_struct, I think we can extend the 8 bytes for CONFIG_BASE_SMALL as well.
This patchset changes files among many subsystems. I don't know which tree it should be applied to, so I just base it on Linus's tree. There're many truncated kthreads in the kernel, which may make trouble for the user, for example, the user can't get detailed device information from the task comm. This patchset tries to improve this problem fundamentally by extending the task comm size from 16 to 24. In order to do that, we have to do some cleanups first. 1. Make the copy of task comm always safe no matter what the task comm size is. For example, Unsafe Safe strlcpy strscpy_pad strncpy strscpy_pad bpf_probe_read_kernel bpf_probe_read_kernel_str bpf_core_read_str bpf_get_current_comm perf_event__prepare_comm prctl(2) 2. Replace the old hard-coded 16 with a new macro TASK_COMM_LEN_16 to make it more grepable. 3. Extend the task comm size to 24 for CONFIG_BASE_FULL case and keep it as 16 for CONFIG_BASE_SMALL. 4. Print a warning if the kthread comm is still truncated. Changes since v4: - introduce TASK_COMM_LEN_16 and TASK_COMM_LEN_24 per Steven - replace hard-coded 16 with TASK_COMM_LEN_16 per Kees - use strscpy_pad() instead of strlcpy()/strncpy() per Kees - make perf test adopt to task comm size change per Arnaldo and Mathieu - fix warning reported by kernel test robot Changes since v3: - fixes -Wstringop-truncation warning reported by kernel test robot Changes since v2: - avoid change UAPI code per Kees - remove the description of out of tree code from commit log per Peter Changes since v1: - extend task comm to 24bytes, per Petr - improve the warning per Petr - make the checkpatch warning a separate patch Yafang Shao (15): fs/exec: make __set_task_comm always set a nul ternimated string fs/exec: make __get_task_comm always get a nul terminated string sched.h: introduce TASK_COMM_LEN_16 cn_proc: make connector comm always nul ternimated drivers/infiniband: make setup_ctxt always get a nul terminated task comm elfcore: make prpsinfo always get a nul terminated task comm samples/bpf/kern: use TASK_COMM_LEN instead of hard-coded 16 samples/bpf/user: use TASK_COMM_LEN_16 instead of hard-coded 16 tools/include: introduce TASK_COMM_LEN_16 tools/lib/perf: use TASK_COMM_LEN_16 instead of hard-coded 16 tools/bpf/bpftool: use TASK_COMM_LEN_16 instead of hard-coded 16 tools/perf/test: make perf test adopt to task comm size change tools/testing/selftests/bpf: use TASK_COMM_LEN_16 instead of hard-coded 16 sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL kernel/kthread: show a warning if kthread's comm is truncated drivers/connector/cn_proc.c | 5 +++- drivers/infiniband/hw/qib/qib.h | 4 +-- drivers/infiniband/hw/qib/qib_file_ops.c | 2 +- fs/binfmt_elf.c | 2 +- fs/exec.c | 5 ++-- include/linux/elfcore-compat.h | 3 ++- include/linux/elfcore.h | 4 +-- include/linux/sched.h | 11 +++++++- include/uapi/linux/cn_proc.h | 7 ++++- kernel/kthread.c | 7 ++++- samples/bpf/offwaketime_kern.c | 10 +++---- samples/bpf/offwaketime_user.c | 6 ++--- samples/bpf/test_overhead_kprobe_kern.c | 11 ++++---- samples/bpf/test_overhead_tp_kern.c | 5 ++-- samples/bpf/tracex2_kern.c | 3 ++- samples/bpf/tracex2_user.c | 7 ++--- tools/bpf/bpftool/Makefile | 1 + tools/bpf/bpftool/main.h | 3 ++- tools/bpf/bpftool/skeleton/pid_iter.bpf.c | 4 +-- tools/bpf/bpftool/skeleton/pid_iter.h | 4 ++- tools/include/linux/sched/task.h | 3 +++ tools/lib/perf/include/perf/event.h | 5 ++-- tools/perf/tests/evsel-tp-sched.c | 26 ++++++++++++++----- tools/testing/selftests/bpf/Makefile | 2 +- .../selftests/bpf/prog_tests/ringbuf.c | 3 ++- .../selftests/bpf/prog_tests/ringbuf_multi.c | 3 ++- .../bpf/prog_tests/sk_storage_tracing.c | 3 ++- .../selftests/bpf/prog_tests/test_overhead.c | 3 ++- .../bpf/prog_tests/trampoline_count.c | 3 ++- tools/testing/selftests/bpf/progs/profiler.h | 7 ++--- .../selftests/bpf/progs/profiler.inc.h | 8 +++--- tools/testing/selftests/bpf/progs/pyperf.h | 4 +-- .../testing/selftests/bpf/progs/strobemeta.h | 6 ++--- .../bpf/progs/test_core_reloc_kernel.c | 3 ++- .../selftests/bpf/progs/test_ringbuf.c | 3 ++- .../selftests/bpf/progs/test_ringbuf_multi.c | 3 ++- .../bpf/progs/test_sk_storage_tracing.c | 5 ++-- .../selftests/bpf/progs/test_skb_helpers.c | 5 ++-- .../selftests/bpf/progs/test_stacktrace_map.c | 5 ++-- .../selftests/bpf/progs/test_tracepoint.c | 5 ++-- 40 files changed, 135 insertions(+), 74 deletions(-)