Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

[Dmitry V. Levin]

On Fri, May 10, 2024 at 03:04:32PM +0100, Vincent Donnefort wrote:
[...]
> diff --git a/include/uapi/linux/trace_mmap.h b/include/uapi/linux/trace_mmap.h
> index b682e9925539..bd1066754220 100644
> --- a/include/uapi/linux/trace_mmap.h
> +++ b/include/uapi/linux/trace_mmap.h
> @@ -43,4 +43,6 @@ struct trace_buffer_meta {
>  	__u64	Reserved2;
>  };
>  
> +#define TRACE_MMAP_IOCTL_GET_READER		_IO('T', 0x1)
> +

I'm sorry but among all the numbers this one was probably the least
fortunate choice because it collides with TCGETS on most of architectures.

For example, this is how strace output would look like when
TRACE_MMAP_IOCTL_GET_READER support is added:

$ strace -e ioctl stty
ioctl(0, TCGETS or TRACE_MMAP_IOCTL_GET_READER, {c_iflag=ICRNL|IXON, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD, c_lflag=ISIG|ICANON|ECHO|ECHOE|ECHOK|IEXTEN|ECHOCTL|ECHOKE, ...}) = 0

Even though ioctl numbers are inherently not unique, TCGETS is
a very traditional one, so it would be great if you could change
TRACE_MMAP_IOCTL_GET_READER to avoid this collision.

Given that _IO('T', 0x1) is _IOC(_IOC_NONE, 'T', 0x1, 0),
something like _IOC(_IOC_NONE, 'T', 0x1, 0x1) should be OK.


-- 
ldv

Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

[Steven Rostedt]

On Sun, 30 Jun 2024 13:53:23 +0300
"Dmitry V. Levin" <ldv@strace.io> wrote:

> On Fri, May 10, 2024 at 03:04:32PM +0100, Vincent Donnefort wrote:
> [...]
> > diff --git a/include/uapi/linux/trace_mmap.h b/include/uapi/linux/trace_mmap.h
> > index b682e9925539..bd1066754220 100644
> > --- a/include/uapi/linux/trace_mmap.h
> > +++ b/include/uapi/linux/trace_mmap.h
> > @@ -43,4 +43,6 @@ struct trace_buffer_meta {
> >  	__u64	Reserved2;
> >  };
> >  
> > +#define TRACE_MMAP_IOCTL_GET_READER		_IO('T', 0x1)
> > +  
> 
> I'm sorry but among all the numbers this one was probably the least
> fortunate choice because it collides with TCGETS on most of architectures.

Hmm, that is unfortunate.

> 
> For example, this is how strace output would look like when
> TRACE_MMAP_IOCTL_GET_READER support is added:
> 
> $ strace -e ioctl stty
> ioctl(0, TCGETS or TRACE_MMAP_IOCTL_GET_READER, {c_iflag=ICRNL|IXON, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD, c_lflag=ISIG|ICANON|ECHO|ECHOE|ECHOK|IEXTEN|ECHOCTL|ECHOKE, ...}) = 0
> 
> Even though ioctl numbers are inherently not unique, TCGETS is
> a very traditional one, so it would be great if you could change
> TRACE_MMAP_IOCTL_GET_READER to avoid this collision.
> 
> Given that _IO('T', 0x1) is _IOC(_IOC_NONE, 'T', 0x1, 0),
> something like _IOC(_IOC_NONE, 'T', 0x1, 0x1) should be OK.

Well, it may not be too late to update this as it hasn't been
officially released in 6.10 yet. It's still only in the -rc and the
library doesn't rely on this yet (I've been holding off until 6.10 was
officially released before releasing the library that uses it).

I can send a patch this week to update it. Or feel free to send a patch
yourself.

Thanks,

-- Steve

Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

[Mathieu Desnoyers]

On 2024-06-30 08:40, Steven Rostedt wrote:
> On Sun, 30 Jun 2024 13:53:23 +0300
> "Dmitry V. Levin" <ldv@strace.io> wrote:
> 
>> On Fri, May 10, 2024 at 03:04:32PM +0100, Vincent Donnefort wrote:
>> [...]
>>> diff --git a/include/uapi/linux/trace_mmap.h b/include/uapi/linux/trace_mmap.h
>>> index b682e9925539..bd1066754220 100644
>>> --- a/include/uapi/linux/trace_mmap.h
>>> +++ b/include/uapi/linux/trace_mmap.h
>>> @@ -43,4 +43,6 @@ struct trace_buffer_meta {
>>>   	__u64	Reserved2;
>>>   };
>>>   
>>> +#define TRACE_MMAP_IOCTL_GET_READER		_IO('T', 0x1)
>>> +
>>
>> I'm sorry but among all the numbers this one was probably the least
>> fortunate choice because it collides with TCGETS on most of architectures.
> 
> Hmm, that is unfortunate.
> 
>>
>> For example, this is how strace output would look like when
>> TRACE_MMAP_IOCTL_GET_READER support is added:
>>
>> $ strace -e ioctl stty
>> ioctl(0, TCGETS or TRACE_MMAP_IOCTL_GET_READER, {c_iflag=ICRNL|IXON, c_oflag=NL0|CR0|TAB0|BS0|VT0|FF0|OPOST|ONLCR, c_cflag=B38400|CS8|CREAD, c_lflag=ISIG|ICANON|ECHO|ECHOE|ECHOK|IEXTEN|ECHOCTL|ECHOKE, ...}) = 0
>>
>> Even though ioctl numbers are inherently not unique, TCGETS is
>> a very traditional one, so it would be great if you could change
>> TRACE_MMAP_IOCTL_GET_READER to avoid this collision.
>>
>> Given that _IO('T', 0x1) is _IOC(_IOC_NONE, 'T', 0x1, 0),
>> something like _IOC(_IOC_NONE, 'T', 0x1, 0x1) should be OK.
> 
> Well, it may not be too late to update this as it hasn't been
> officially released in 6.10 yet. It's still only in the -rc and the
> library doesn't rely on this yet (I've been holding off until 6.10 was
> officially released before releasing the library that uses it).
> 
> I can send a patch this week to update it. Or feel free to send a patch
> yourself.

You need to reserve an unused ioctl Code and Seq# range within:

Documentation/userspace-api/ioctl/ioctl-number.rst

Otherwise this duplicate will confuse all system call instrumentation
tooling.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

[Steven Rostedt]

On Tue, 2 Jul 2024 10:36:03 -0400
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> > I can send a patch this week to update it. Or feel free to send a patch
> > yourself.  
> 
> You need to reserve an unused ioctl Code and Seq# range within:
> 
> Documentation/userspace-api/ioctl/ioctl-number.rst

Ug, it's been so long, I completely forgot about that file.

Thanks for catching this.

> 
> Otherwise this duplicate will confuse all system call instrumentation
> tooling.

Agreed, what if we did this then:

-- Steve

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index a141e8e65c5d..9a97030c6c8d 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -186,6 +186,7 @@ Code  Seq#    Include File                                           Comments
 'Q'   all    linux/soundcard.h
 'R'   00-1F  linux/random.h                                          conflict!
 'R'   01     linux/rfkill.h                                          conflict!
+'R'   20-2F  linux/trace_mmap.h
 'R'   C0-DF  net/bluetooth/rfcomm.h
 'R'   E0     uapi/linux/fsl_mc.h
 'S'   all    linux/cdrom.h                                           conflict!
diff --git a/include/uapi/linux/trace_mmap.h b/include/uapi/linux/trace_mmap.h
index bd1066754220..c102ef35d11e 100644
--- a/include/uapi/linux/trace_mmap.h
+++ b/include/uapi/linux/trace_mmap.h
@@ -43,6 +43,6 @@ struct trace_buffer_meta {
 	__u64	Reserved2;
 };
 
-#define TRACE_MMAP_IOCTL_GET_READER		_IO('T', 0x1)
+#define TRACE_MMAP_IOCTL_GET_READER		_IO('R', 0x20)
 
 #endif /* _TRACE_MMAP_H_ */

Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

[Mathieu Desnoyers]

On 2024-07-02 11:18, Steven Rostedt wrote:
> On Tue, 2 Jul 2024 10:36:03 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
>>> I can send a patch this week to update it. Or feel free to send a patch
>>> yourself.
>>
>> You need to reserve an unused ioctl Code and Seq# range within:
>>
>> Documentation/userspace-api/ioctl/ioctl-number.rst
> 
> Ug, it's been so long, I completely forgot about that file.
> 
> Thanks for catching this.
> 
>>
>> Otherwise this duplicate will confuse all system call instrumentation
>> tooling.
> 
> Agreed, what if we did this then:
> 
> -- Steve
> 
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index a141e8e65c5d..9a97030c6c8d 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -186,6 +186,7 @@ Code  Seq#    Include File                                           Comments
>   'Q'   all    linux/soundcard.h
>   'R'   00-1F  linux/random.h                                          conflict!
>   'R'   01     linux/rfkill.h                                          conflict!
> +'R'   20-2F  linux/trace_mmap.h
>   'R'   C0-DF  net/bluetooth/rfcomm.h
>   'R'   E0     uapi/linux/fsl_mc.h
>   'S'   all    linux/cdrom.h                                           conflict!
> diff --git a/include/uapi/linux/trace_mmap.h b/include/uapi/linux/trace_mmap.h
> index bd1066754220..c102ef35d11e 100644
> --- a/include/uapi/linux/trace_mmap.h
> +++ b/include/uapi/linux/trace_mmap.h
> @@ -43,6 +43,6 @@ struct trace_buffer_meta {
>   	__u64	Reserved2;
>   };
>   
> -#define TRACE_MMAP_IOCTL_GET_READER		_IO('T', 0x1)
> +#define TRACE_MMAP_IOCTL_GET_READER		_IO('R', 0x20)

Note that user events also has this issue: the ioctl is not reserved in
the ioctl-number.rst list. See include/uapi/linux/user_events.h:

#define DIAG_IOC_MAGIC '*'

/* Request to register a user_event */
#define DIAG_IOCSREG _IOWR(DIAG_IOC_MAGIC, 0, struct user_reg *)

/* Request to delete a user_event */
#define DIAG_IOCSDEL _IOW(DIAG_IOC_MAGIC, 1, char *)

/* Requests to unregister a user_event */
#define DIAG_IOCSUNREG _IOW(DIAG_IOC_MAGIC, 2, struct user_unreg*)

Where '*' maps to Code 0x2A. Looking at the list I don't see any
conflicts there, but we should definitely add it.

If we use '*' for user events already, perhaps we'd want to consider
using the same range for the ring buffer ioctls ? Arguably one is
about instrumentation and the other is about ring buffer interaction
(data transport), but those are both related to tracing.

Thanks,

Mathieu

>   
>   #endif /* _TRACE_MMAP_H_ */

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

[Dmitry V. Levin]

On Tue, Jul 02, 2024 at 11:32:53AM -0400, Mathieu Desnoyers wrote:
[...]
> Note that user events also has this issue: the ioctl is not reserved in
> the ioctl-number.rst list. See include/uapi/linux/user_events.h:
> 
> #define DIAG_IOC_MAGIC '*'
> 
> /* Request to register a user_event */
> #define DIAG_IOCSREG _IOWR(DIAG_IOC_MAGIC, 0, struct user_reg *)
> 
> /* Request to delete a user_event */
> #define DIAG_IOCSDEL _IOW(DIAG_IOC_MAGIC, 1, char *)
> 
> /* Requests to unregister a user_event */
> #define DIAG_IOCSUNREG _IOW(DIAG_IOC_MAGIC, 2, struct user_unreg*)
> 
> Where '*' maps to Code 0x2A. Looking at the list I don't see any
> conflicts there, but we should definitely add it.
> 
> If we use '*' for user events already, perhaps we'd want to consider
> using the same range for the ring buffer ioctls ? Arguably one is
> about instrumentation and the other is about ring buffer interaction
> (data transport), but those are both related to tracing.

FWIW, I've checked the list of ioctls known to strace and can confirm that
currently there are no more users of 0x2a ioctl code besides these three
DIAG_* ioctls.


-- 
ldv

Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

[Steven Rostedt]

On Tue, 2 Jul 2024 11:32:53 -0400
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> If we use '*' for user events already, perhaps we'd want to consider
> using the same range for the ring buffer ioctls ? Arguably one is
> about instrumentation and the other is about ring buffer interaction
> (data transport), but those are both related to tracing.

Yeah, but I still rather keep them separate.

Beau, care to send a patch adding an entry into that ioctl document for
user events?

-- Steve

Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

[Dmitry V. Levin]

On Tue, Jul 02, 2024 at 11:18:07AM -0400, Steven Rostedt wrote:
> On Tue, 2 Jul 2024 10:36:03 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> > > I can send a patch this week to update it. Or feel free to send a patch
> > > yourself.  
> > 
> > You need to reserve an unused ioctl Code and Seq# range within:
> > 
> > Documentation/userspace-api/ioctl/ioctl-number.rst
> 
> Ug, it's been so long, I completely forgot about that file.
> 
> Thanks for catching this.
> 
> > 
> > Otherwise this duplicate will confuse all system call instrumentation
> > tooling.
> 
> Agreed, what if we did this then:
> 
> -- Steve
> 
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index a141e8e65c5d..9a97030c6c8d 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -186,6 +186,7 @@ Code  Seq#    Include File                                           Comments
>  'Q'   all    linux/soundcard.h
>  'R'   00-1F  linux/random.h                                          conflict!
>  'R'   01     linux/rfkill.h                                          conflict!
> +'R'   20-2F  linux/trace_mmap.h
>  'R'   C0-DF  net/bluetooth/rfcomm.h
>  'R'   E0     uapi/linux/fsl_mc.h
>  'S'   all    linux/cdrom.h                                           conflict!

Just in case, I've checked the list of ioctls known to strace and can
confirm that there are no users of 'R' ioctl code in 0x20..0x2f range yet.

By the way, this file is definitely not up to date, the 'R' part of it
should have contained the following:

'R'   00-1F  uapi/linux/random.h                                     conflict!
'R'   01-02  uapi/linux/rfkill.h                                     conflict!
'R'   01-0D  uapi/misc/fastrpc.h                                     conflict!
'R'   C0-DF  net/bluetooth/rfcomm.h
'R'   E0     uapi/linux/fsl_mc.h


-- 
ldv

Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

[Mathieu Desnoyers]

On 2024-07-02 12:51, Steven Rostedt wrote:
> On Tue, 2 Jul 2024 11:32:53 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
>> If we use '*' for user events already, perhaps we'd want to consider
>> using the same range for the ring buffer ioctls ? Arguably one is
>> about instrumentation and the other is about ring buffer interaction
>> (data transport), but those are both related to tracing.
> 
> Yeah, but I still rather keep them separate.

No objection, I'm OK either way.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [PATCH v23 3/5] tracing: Allow user-space mapping of the ring-buffer

[Beau Belgrave]

On Tue, Jul 02, 2024 at 12:51:26PM -0400, Steven Rostedt wrote:
> On Tue, 2 Jul 2024 11:32:53 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> > If we use '*' for user events already, perhaps we'd want to consider
> > using the same range for the ring buffer ioctls ? Arguably one is
> > about instrumentation and the other is about ring buffer interaction
> > (data transport), but those are both related to tracing.
> 
> Yeah, but I still rather keep them separate.
> 
> Beau, care to send a patch adding an entry into that ioctl document for
> user events?
> 
> -- Steve

Sure thing, sent one out [1].

Thanks,
-Beau

1. https://lore.kernel.org/linux-doc/20240703222501.1547-1-beaub@linux.microsoft.com/