----- On Oct 14, 2021, at 9:11 AM, Yafang Shao [email protected] wrote:
> On Thu, Oct 14, 2021 at 9:09 PM Mathieu Desnoyers
> <[email protected]> wrote:
>>
>> ----- On Oct 14, 2021, at 9:05 AM, Yafang Shao [email protected] wrote:
>> [...]
>> >> If it happens that this ABI break is noticed by more than an in-tree test
>> >> program, then
>> >> the kernel's ABI rules will require that this trace field size stays unchanged.
>> >> This brings
>> >> up once more the whole topic of "Tracepoints ABI" which has been discussed
>> >> repeatedly in
>> >> the past.
>> >>
>> >
>> > I will check if any other in-tree tools depends on TASK_COMM_LEN.
>>
>> That's a start, but given this is a userspace ABI, out-of-tree userland
>> tools which depend of this to be fixed-size are also relevant.
>>
>
> TASK_COMM_LEN isn't defined in include/uapi/ directory, so it seems
> that it isn't the uerspace ABI?
One case where this 16 bytes size is expected by userspace is prctl(2) PR_GET_NAME
and PR_SET_NAME.
The other case I am referring to is with ftrace and perf:
mount -t tracefs nodev /sys/kernel/tracing
cat /sys/kernel/tracing/events/sched/sched_switch/format
name: sched_switch
ID: 314
format:
[...]
field:char prev_comm[16]; offset:8; size:16; signed:1;
[...]
field:char next_comm[16]; offset:40; size:16; signed:1;
Both of those fields expose a fixed-size of 16 bytes.
AFAIK Steven's intent was that by parsing this file, trace viewers could adapt to
changes in the event field layout. Unfortunately, there have been cases where
trace viewers had a hard expectation on the field layout. Hopefully those have
all been fixed a while ago.
Thanks,
Mathieu
>
>
> --
> Thanks
> Yafang
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Thu, Oct 14, 2021 at 9:50 PM Mathieu Desnoyers
<[email protected]> wrote:
>
> ----- On Oct 14, 2021, at 9:11 AM, Yafang Shao [email protected] wrote:
>
> > On Thu, Oct 14, 2021 at 9:09 PM Mathieu Desnoyers
> > <[email protected]> wrote:
> >>
> >> ----- On Oct 14, 2021, at 9:05 AM, Yafang Shao [email protected] wrote:
> >> [...]
> >> >> If it happens that this ABI break is noticed by more than an in-tree test
> >> >> program, then
> >> >> the kernel's ABI rules will require that this trace field size stays unchanged.
> >> >> This brings
> >> >> up once more the whole topic of "Tracepoints ABI" which has been discussed
> >> >> repeatedly in
> >> >> the past.
> >> >>
> >> >
> >> > I will check if any other in-tree tools depends on TASK_COMM_LEN.
> >>
> >> That's a start, but given this is a userspace ABI, out-of-tree userland
> >> tools which depend of this to be fixed-size are also relevant.
> >>
> >
> > TASK_COMM_LEN isn't defined in include/uapi/ directory, so it seems
> > that it isn't the uerspace ABI?
>
> One case where this 16 bytes size is expected by userspace is prctl(2) PR_GET_NAME
> and PR_SET_NAME.
>
the prctl(2) man page says that:
: PR_SET_NAME
: If the length of the string, including the terminating
: null byte, exceeds 16 bytes, the string is silently truncated.
: PR_GET_NAME
: The buffer should allow space for up to 16 bytes
: the returned string will be null-terminated.
As the string returned to user space is null-terminated, extending
task comm won't break the prctl(2) user code.
> The other case I am referring to is with ftrace and perf:
>
> mount -t tracefs nodev /sys/kernel/tracing
> cat /sys/kernel/tracing/events/sched/sched_switch/format
>
> name: sched_switch
> ID: 314
> format:
> [...]
> field:char prev_comm[16]; offset:8; size:16; signed:1;
> [...]
> field:char next_comm[16]; offset:40; size:16; signed:1;
>
> Both of those fields expose a fixed-size of 16 bytes.
>
> AFAIK Steven's intent was that by parsing this file, trace viewers could adapt to
> changes in the event field layout. Unfortunately, there have been cases where
> trace viewers had a hard expectation on the field layout. Hopefully those have
> all been fixed a while ago.
>
I don't have a clear idea what will happen to trace viewers if we
extend task comm.
Steven, do you have any suggestions ?
--
Thanks
Yafang
On Thu, Oct 14, 2021 at 10:40:04PM +0800, Yafang Shao wrote:
> On Thu, Oct 14, 2021 at 9:50 PM Mathieu Desnoyers
> <[email protected]> wrote:
> >
> > ----- On Oct 14, 2021, at 9:11 AM, Yafang Shao [email protected] wrote:
> >
> > > On Thu, Oct 14, 2021 at 9:09 PM Mathieu Desnoyers
> > > <[email protected]> wrote:
> > >>
> > >> ----- On Oct 14, 2021, at 9:05 AM, Yafang Shao [email protected] wrote:
> > >> [...]
> > >> >> If it happens that this ABI break is noticed by more than an in-tree test
> > >> >> program, then
> > >> >> the kernel's ABI rules will require that this trace field size stays unchanged.
> > >> >> This brings
> > >> >> up once more the whole topic of "Tracepoints ABI" which has been discussed
> > >> >> repeatedly in
> > >> >> the past.
> > >> >>
> > >> >
> > >> > I will check if any other in-tree tools depends on TASK_COMM_LEN.
> > >>
> > >> That's a start, but given this is a userspace ABI, out-of-tree userland
> > >> tools which depend of this to be fixed-size are also relevant.
> > >>
> > >
> > > TASK_COMM_LEN isn't defined in include/uapi/ directory, so it seems
> > > that it isn't the uerspace ABI?
> >
> > One case where this 16 bytes size is expected by userspace is prctl(2) PR_GET_NAME
> > and PR_SET_NAME.
> >
>
> the prctl(2) man page says that:
> : PR_SET_NAME
> : If the length of the string, including the terminating
> : null byte, exceeds 16 bytes, the string is silently truncated.
> : PR_GET_NAME
> : The buffer should allow space for up to 16 bytes
> : the returned string will be null-terminated.
>
> As the string returned to user space is null-terminated, extending
> task comm won't break the prctl(2) user code.
If userspace was:
char comm[16];
int important_values;
...
prctl(PR_GET_NAME, pid, comm);
the kernel will clobber "important_values", so prctl() must enforce the
old size (and terminate it correctly). It's not okay for us to expect
that userspace get recompiled -- old binaries must continue to work
correctly on kernel kernels.
case PR_GET_NAME:
get_task_comm(comm, me);
if (copy_to_user((char __user *)arg2, comm, sizeof(comm)))
return -EFAULT;
break;
This looks like it needs to be explicitly NUL terminated at 16 as well.
I'd say we need a TASK_COMM_LEN_V1 to use in all the old hard-coded
places.
-Kees
>
> > The other case I am referring to is with ftrace and perf:
> >
> > mount -t tracefs nodev /sys/kernel/tracing
> > cat /sys/kernel/tracing/events/sched/sched_switch/format
> >
> > name: sched_switch
> > ID: 314
> > format:
> > [...]
> > field:char prev_comm[16]; offset:8; size:16; signed:1;
> > [...]
> > field:char next_comm[16]; offset:40; size:16; signed:1;
> >
> > Both of those fields expose a fixed-size of 16 bytes.
> >
> > AFAIK Steven's intent was that by parsing this file, trace viewers could adapt to
> > changes in the event field layout. Unfortunately, there have been cases where
> > trace viewers had a hard expectation on the field layout. Hopefully those have
> > all been fixed a while ago.
> >
>
> I don't have a clear idea what will happen to trace viewers if we
> extend task comm.
>
> Steven, do you have any suggestions ?
>
> --
> Thanks
> Yafang
--
Kees Cook
On Fri, Oct 15, 2021 at 12:14 AM Kees Cook <[email protected]> wrote:
>
> On Thu, Oct 14, 2021 at 10:40:04PM +0800, Yafang Shao wrote:
> > On Thu, Oct 14, 2021 at 9:50 PM Mathieu Desnoyers
> > <[email protected]> wrote:
> > >
> > > ----- On Oct 14, 2021, at 9:11 AM, Yafang Shao [email protected] wrote:
> > >
> > > > On Thu, Oct 14, 2021 at 9:09 PM Mathieu Desnoyers
> > > > <[email protected]> wrote:
> > > >>
> > > >> ----- On Oct 14, 2021, at 9:05 AM, Yafang Shao [email protected] wrote:
> > > >> [...]
> > > >> >> If it happens that this ABI break is noticed by more than an in-tree test
> > > >> >> program, then
> > > >> >> the kernel's ABI rules will require that this trace field size stays unchanged.
> > > >> >> This brings
> > > >> >> up once more the whole topic of "Tracepoints ABI" which has been discussed
> > > >> >> repeatedly in
> > > >> >> the past.
> > > >> >>
> > > >> >
> > > >> > I will check if any other in-tree tools depends on TASK_COMM_LEN.
> > > >>
> > > >> That's a start, but given this is a userspace ABI, out-of-tree userland
> > > >> tools which depend of this to be fixed-size are also relevant.
> > > >>
> > > >
> > > > TASK_COMM_LEN isn't defined in include/uapi/ directory, so it seems
> > > > that it isn't the uerspace ABI?
> > >
> > > One case where this 16 bytes size is expected by userspace is prctl(2) PR_GET_NAME
> > > and PR_SET_NAME.
> > >
> >
> > the prctl(2) man page says that:
> > : PR_SET_NAME
> > : If the length of the string, including the terminating
> > : null byte, exceeds 16 bytes, the string is silently truncated.
> > : PR_GET_NAME
> > : The buffer should allow space for up to 16 bytes
> > : the returned string will be null-terminated.
> >
> > As the string returned to user space is null-terminated, extending
> > task comm won't break the prctl(2) user code.
>
> If userspace was:
>
> char comm[16];
> int important_values;
>
> ...
>
> prctl(PR_GET_NAME, pid, comm);
>
> the kernel will clobber "important_values", so prctl() must enforce the
> old size (and terminate it correctly). It's not okay for us to expect
> that userspace get recompiled -- old binaries must continue to work
> correctly on kernel kernels.
>
> case PR_GET_NAME:
> get_task_comm(comm, me);
> if (copy_to_user((char __user *)arg2, comm, sizeof(comm)))
> return -EFAULT;
> break;
>
> This looks like it needs to be explicitly NUL terminated at 16 as well.
>
Right. After replacing strncpy with strscpy_pad() in
__get_task_comm(), the string will be NUL terminated.
> I'd say we need a TASK_COMM_LEN_V1 to use in all the old hard-coded
> places.
>
Sure, it will be easy to grep then.
> >
> > > The other case I am referring to is with ftrace and perf:
> > >
> > > mount -t tracefs nodev /sys/kernel/tracing
> > > cat /sys/kernel/tracing/events/sched/sched_switch/format
> > >
> > > name: sched_switch
> > > ID: 314
> > > format:
> > > [...]
> > > field:char prev_comm[16]; offset:8; size:16; signed:1;
> > > [...]
> > > field:char next_comm[16]; offset:40; size:16; signed:1;
> > >
> > > Both of those fields expose a fixed-size of 16 bytes.
> > >
> > > AFAIK Steven's intent was that by parsing this file, trace viewers could adapt to
> > > changes in the event field layout. Unfortunately, there have been cases where
> > > trace viewers had a hard expectation on the field layout. Hopefully those have
> > > all been fixed a while ago.
> > >
> >
> > I don't have a clear idea what will happen to trace viewers if we
> > extend task comm.
> >
> > Steven, do you have any suggestions ?
> >
> > --
> > Thanks
> > Yafang
>
> --
> Kees Cook
--
Thanks
Yafang