Re: [PATCH v5 3/3] treewide: Switch from tsk->comm to tsk->comm_str which is 64 bytes long

From: Bhupesh Sharma
Date: Wed Jul 16 2025 - 16:18:56 EST

Next message: Alex Williamson: "Re: [PATCH v2] vfio/mlx5: fix possible overflow in tracking max message size"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH v3 1/8] perf annotate: Rename to __hist_entry__tui_annotate()"
In reply to: Andrii Nakryiko: "Re: [PATCH v5 3/3] treewide: Switch from tsk->comm to tsk->comm_str which is 64 bytes long"
Next in thread: Andrii Nakryiko: "Re: [PATCH v5 3/3] treewide: Switch from tsk->comm to tsk->comm_str which is 64 bytes long"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 7/16/25 11:40 PM, Andrii Nakryiko wrote:

On Wed, Jul 16, 2025 at 5:40 AM Bhupesh <bhupesh@xxxxxxxxxx> wrote:

Historically due to the 16-byte length of TASK_COMM_LEN, the
users of 'tsk->comm' are restricted to use a fixed-size target
buffer also of TASK_COMM_LEN for 'memcpy()' like use-cases.

To fix the same, Kees suggested in [1] that we can replace tsk->comm,
with tsk->comm_str, inside 'task_struct':
union {
char comm_str[TASK_COMM_EXT_LEN];
};

where TASK_COMM_EXT_LEN is 64-bytes.

Do we absolutely have to rename task->comm field? I understand as an
intermediate step to not miss any existing users in the kernel
sources, but once that all is done, we can rename that back to comm,
right?

The reason I'm asking is because accessing task->comm is *very common*
with all sorts of BPF programs and scripts. Yes, we have way to deal
with that with BPF CO-RE, but every single use case would need to be
updated now to work both with task->comm name on old kernels and
task->comm_str on new kernels (because BPF programs are written in
more or less kernel version agnostic way, so they have to handle many
kernel releases).

So, unless absolutely necessary, can we please keep the field name the
same? Changing the size of that field is not really a problem for BPF,
so no objections against that.

So, as a background we have had several previous discussions regarding renaming the 'tsk->comm' to 'task->comm_str' on the list (please see [1] and [2] for details), and as per Kees's recommendations we have taken this approach in the v5 patchset (may be Kees can add further details if I have missed adding something in the log message above).

That being said, ideally one would not like to break any exiting ABI (which includes existing / older BPF programs). I was having a look at the BPF CO_RE reference guide (see [3]), and was able to make out that BPF CO_RE has a definition of |s||truct task_struct|which doesn't need to match the kernel's struct task_struct definition exactly (as [3] mentions -> only a necessary subset of fields have to be present and compatible):

|struct task_struct { intpid; charcomm[16]; struct task_struct*group_leader; } __attribute__((preserve_access_index)); |

So, if we add a check here to add '|charcomm[16]' or||charcomm_str[16]' to BPF CO RE's internal 'struct task_struct' on basis of the underlying kernel version being used (something like using 'KERNEL_VERSION(x, y, 0)' for example), will that suffice? I have ||used and seen these checks being used in the user-space applications (for example, see [4]) at several occasions.

Please let me know your views.

|[1]. https://lore.kernel.org/all/202505222041.B639D482FB@keescook/
[2]. https://lore.kernel.org/all/ba4ddf27-91e7-0ecc-95d5-c139f6978812@xxxxxxxxxx/
[3]. https://nakryiko.com/posts/bpf-core-reference-guide/
[4]. https://github.com/crash-utility/crash/blob/master/memory_driver/crash.c#L41C25-L41C49

Thanks.

And then modify 'get_task_comm()' to pass 'tsk->comm_str'
to the existing users.

This would mean that ABI is maintained while ensuring that:

- Existing users of 'get_task_comm'/ 'set_task_comm' will get 'tsk->comm_str'
truncated to a maximum of 'TASK_COMM_LEN' (16-bytes) to maintain ABI,
- New / Modified users of 'get_task_comm'/ 'set_task_comm' will get
'tsk->comm_str' supported for a maximum of 'TASK_COMM_EXTLEN' (64-bytes).

Note, that the existing users have not been modified to migrate to
'TASK_COMM_EXT_LEN', in case they have hard-coded expectations of
dealing with only a 'TASK_COMM_LEN' long 'tsk->comm_str'.

[1]. https://lore.kernel.org/all/202505231346.52F291C54@keescook/

Signed-off-by: Bhupesh <bhupesh@xxxxxxxxxx>
---
arch/arm64/kernel/traps.c | 2 +-
arch/arm64/kvm/mmu.c | 2 +-
block/blk-core.c | 2 +-
block/bsg.c | 2 +-
drivers/char/random.c | 2 +-
drivers/hid/hid-core.c | 6 +++---
drivers/mmc/host/tmio_mmc_core.c | 6 +++---
drivers/pci/pci-sysfs.c | 2 +-
drivers/scsi/scsi_ioctl.c | 2 +-
drivers/tty/serial/serial_core.c | 2 +-
drivers/tty/tty_io.c | 8 ++++----
drivers/usb/core/devio.c | 16 ++++++++--------
drivers/usb/core/message.c | 2 +-
drivers/vfio/group.c | 2 +-
drivers/vfio/vfio_iommu_type1.c | 2 +-
drivers/vfio/vfio_main.c | 2 +-
drivers/xen/evtchn.c | 2 +-
drivers/xen/grant-table.c | 2 +-
fs/binfmt_elf.c | 2 +-
fs/coredump.c | 4 ++--
fs/drop_caches.c | 2 +-
fs/exec.c | 8 ++++----
fs/ext4/dir.c | 2 +-
fs/ext4/inode.c | 2 +-
fs/ext4/namei.c | 2 +-
fs/ext4/super.c | 12 ++++++------
fs/hugetlbfs/inode.c | 2 +-
fs/ioctl.c | 2 +-
fs/iomap/direct-io.c | 2 +-
fs/jbd2/transaction.c | 2 +-
fs/locks.c | 2 +-
fs/netfs/internal.h | 2 +-
fs/proc/base.c | 2 +-
fs/read_write.c | 2 +-
fs/splice.c | 2 +-
include/linux/coredump.h | 2 +-
include/linux/filter.h | 2 +-
include/linux/ratelimit.h | 2 +-
include/linux/sched.h | 11 ++++++++---
init/init_task.c | 2 +-
ipc/sem.c | 2 +-
kernel/acct.c | 2 +-
kernel/audit.c | 4 ++--
kernel/auditsc.c | 10 +++++-----
kernel/bpf/helpers.c | 2 +-
kernel/capability.c | 4 ++--
kernel/cgroup/cgroup-v1.c | 2 +-
kernel/cred.c | 4 ++--
kernel/events/core.c | 2 +-
kernel/exit.c | 6 +++---
kernel/fork.c | 9 +++++++--
kernel/freezer.c | 4 ++--
kernel/futex/waitwake.c | 2 +-
kernel/hung_task.c | 10 +++++-----
kernel/irq/manage.c | 2 +-
kernel/kthread.c | 2 +-
kernel/locking/rtmutex.c | 2 +-
kernel/printk/printk.c | 2 +-
kernel/sched/core.c | 22 +++++++++++-----------
kernel/sched/debug.c | 4 ++--
kernel/signal.c | 6 +++---
kernel/sys.c | 6 +++---
kernel/sysctl.c | 2 +-
kernel/time/itimer.c | 4 ++--
kernel/time/posix-cpu-timers.c | 2 +-
kernel/tsacct.c | 2 +-
kernel/workqueue.c | 6 +++---
lib/dump_stack.c | 2 +-
lib/nlattr.c | 6 +++---
mm/compaction.c | 2 +-
mm/filemap.c | 4 ++--
mm/gup.c | 2 +-
mm/memfd.c | 2 +-
mm/memory-failure.c | 10 +++++-----
mm/memory.c | 2 +-
mm/mmap.c | 4 ++--
mm/oom_kill.c | 18 +++++++++---------
mm/page_alloc.c | 4 ++--
mm/util.c | 2 +-
net/core/sock.c | 2 +-
net/dns_resolver/internal.h | 2 +-
net/ipv4/raw.c | 2 +-
net/ipv4/tcp.c | 2 +-
net/socket.c | 2 +-
security/lsm_audit.c | 4 ++--
85 files changed, 171 insertions(+), 161 deletions(-)

[...]

Next message: Alex Williamson: "Re: [PATCH v2] vfio/mlx5: fix possible overflow in tracking max message size"
Previous message: Arnaldo Carvalho de Melo: "Re: [PATCH v3 1/8] perf annotate: Rename to __hist_entry__tui_annotate()"
In reply to: Andrii Nakryiko: "Re: [PATCH v5 3/3] treewide: Switch from tsk->comm to tsk->comm_str which is 64 bytes long"
Next in thread: Andrii Nakryiko: "Re: [PATCH v5 3/3] treewide: Switch from tsk->comm to tsk->comm_str which is 64 bytes long"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]