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(-)
[...]
