[PATCH] tracing: do not export user_events uapi

[PATCH] tracing: do not export user_events uapi

[Mathieu Desnoyers]

In addition to mark the USER_EVENTS feature BROKEN until all interested
parties figure out the user-space API, do not install the uapi header.

This prevents situations where a non-final uapi header would end up
being installed into a distribution image and used to build user-space
programs that would then run against newer kernels that will implement
user events with a different ABI.

Link: https://lore.kernel.org/all/20220330155835.5e1f6669@gandalf.local.home

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 include/uapi/Kbuild | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/uapi/Kbuild b/include/uapi/Kbuild
index 61ee6e59c930..425ea8769ddc 100644
--- a/include/uapi/Kbuild
+++ b/include/uapi/Kbuild
@@ -12,3 +12,6 @@ ifeq ($(wildcard $(objtree)/arch/$(SRCARCH)/include/generated/uapi/asm/kvm_para.
 no-export-headers += linux/kvm_para.h
 endif
 endif
+
+# API is not finalized
+no-export-headers += linux/user_events.h
-- 
2.20.1

Re: [PATCH] tracing: do not export user_events uapi

[Steven Rostedt]

Adding the build maintainers.

-- Steve

On Wed, 30 Mar 2022 16:17:55 -0400
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> In addition to mark the USER_EVENTS feature BROKEN until all interested
> parties figure out the user-space API, do not install the uapi header.
> 
> This prevents situations where a non-final uapi header would end up
> being installed into a distribution image and used to build user-space
> programs that would then run against newer kernels that will implement
> user events with a different ABI.
> 
> Link: https://lore.kernel.org/all/20220330155835.5e1f6669@gandalf.local.home
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
>  include/uapi/Kbuild | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/uapi/Kbuild b/include/uapi/Kbuild
> index 61ee6e59c930..425ea8769ddc 100644
> --- a/include/uapi/Kbuild
> +++ b/include/uapi/Kbuild
> @@ -12,3 +12,6 @@ ifeq ($(wildcard $(objtree)/arch/$(SRCARCH)/include/generated/uapi/asm/kvm_para.
>  no-export-headers += linux/kvm_para.h
>  endif
>  endif
> +
> +# API is not finalized
> +no-export-headers += linux/user_events.h

Re: [PATCH] tracing: do not export user_events uapi

[Masahiro Yamada]

On Thu, Mar 31, 2022 at 5:21 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
> Adding the build maintainers.
>
> -- Steve
>
> On Wed, 30 Mar 2022 16:17:55 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
> > In addition to mark the USER_EVENTS feature BROKEN until all interested
> > parties figure out the user-space API, do not install the uapi header.
> >
> > This prevents situations where a non-final uapi header would end up
> > being installed into a distribution image and used to build user-space
> > programs that would then run against newer kernels that will implement
> > user events with a different ABI.
> >
> > Link: https://lore.kernel.org/all/20220330155835.5e1f6669@gandalf.local.home
> >
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > ---
> >  include/uapi/Kbuild | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/include/uapi/Kbuild b/include/uapi/Kbuild
> > index 61ee6e59c930..425ea8769ddc 100644
> > --- a/include/uapi/Kbuild
> > +++ b/include/uapi/Kbuild
> > @@ -12,3 +12,6 @@ ifeq ($(wildcard $(objtree)/arch/$(SRCARCH)/include/generated/uapi/asm/kvm_para.
> >  no-export-headers += linux/kvm_para.h
> >  endif
> >  endif
> > +
> > +# API is not finalized
> > +no-export-headers += linux/user_events.h
>


Well, the intended usage of no-export-headers is to
cater to the UAPI supported by only some architectures.
We have kvm(_para).h here because not all architectures
support kvm.

If you do not want to export the UAPI,
you should not put it in include/uapi/.

After the API is finalized, you can move it to
include/uapi.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] tracing: do not export user_events uapi

[Steven Rostedt]

On Thu, 31 Mar 2022 16:29:30 +0900
Masahiro Yamada <masahiroy@kernel.org> wrote:

> Well, the intended usage of no-export-headers is to
> cater to the UAPI supported by only some architectures.
> We have kvm(_para).h here because not all architectures
> support kvm.
> 
> If you do not want to export the UAPI,
> you should not put it in include/uapi/.
> 
> After the API is finalized, you can move it to
> include/uapi.

So a little bit of background. I and a few others thought it was done, and
pushed it to Linus. Then when it made it into his tree (and mentioned on
LWN) it got a wider audience that had concerns. After they brought up those
concerns, we agreed that this needs a bit more work. I was hoping not to do
a full revert and simply marked the change for broken so that it can be
worked on upstream with the wider audience. Linus appears to be fine with
this approach, as he helped me with my "mark for BROKEN" patch.

Mathieu's concern is that this header file could be used in older distros
with newer kernels that have it implemented and added this to keep out of
those older distros.

The options to make Mathieu sleep better at night are:

1) this patch

2) move this file out of uapi.

3) revert the entire thing.

I really do not want to do #3 but I am willing to do 1 or 2.

-- Steve

Re: [PATCH] tracing: do not export user_events uapi

[Masahiro Yamada]

On Thu, Mar 31, 2022 at 9:13 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Thu, 31 Mar 2022 16:29:30 +0900
> Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> > Well, the intended usage of no-export-headers is to
> > cater to the UAPI supported by only some architectures.
> > We have kvm(_para).h here because not all architectures
> > support kvm.
> >
> > If you do not want to export the UAPI,
> > you should not put it in include/uapi/.
> >
> > After the API is finalized, you can move it to
> > include/uapi.
>
> So a little bit of background. I and a few others thought it was done, and
> pushed it to Linus. Then when it made it into his tree (and mentioned on
> LWN) it got a wider audience that had concerns. After they brought up those
> concerns, we agreed that this needs a bit more work. I was hoping not to do
> a full revert and simply marked the change for broken so that it can be
> worked on upstream with the wider audience. Linus appears to be fine with
> this approach, as he helped me with my "mark for BROKEN" patch.
>
> Mathieu's concern is that this header file could be used in older distros
> with newer kernels that have it implemented and added this to keep out of
> those older distros.
>
> The options to make Mathieu sleep better at night are:
>
> 1) this patch
>
> 2) move this file out of uapi.
>
> 3) revert the entire thing.
>
> I really do not want to do #3 but I am willing to do 1 or 2.

I see.

Either 1 or 2 is OK
if  you are sure this will be fixed sooner or later.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] tracing: do not export user_events uapi

[Steven Rostedt]

On Thu, 31 Mar 2022 23:41:34 +0900
Masahiro Yamada <masahiroy@kernel.org> wrote:

> Either 1 or 2 is OK
> if  you are sure this will be fixed sooner or later.

Thanks,

Then I'll go and pull in Mathieu's patch.

I want this done too, and I believe Beau has a vested interest to get this
correctly done as well, thus it should be worked on and hopefully we will
have something solid by the next merge window.

-- Steve

Re: [PATCH] tracing: do not export user_events uapi

[Mathieu Desnoyers]

----- On Mar 31, 2022, at 8:13 AM, rostedt rostedt@goodmis.org wrote:

> On Thu, 31 Mar 2022 16:29:30 +0900
> Masahiro Yamada <masahiroy@kernel.org> wrote:
> 
>> Well, the intended usage of no-export-headers is to
>> cater to the UAPI supported by only some architectures.
>> We have kvm(_para).h here because not all architectures
>> support kvm.
>> 
>> If you do not want to export the UAPI,
>> you should not put it in include/uapi/.
>> 
>> After the API is finalized, you can move it to
>> include/uapi.
> 
> So a little bit of background. I and a few others thought it was done, and
> pushed it to Linus. Then when it made it into his tree (and mentioned on
> LWN) it got a wider audience that had concerns. After they brought up those
> concerns, we agreed that this needs a bit more work. I was hoping not to do
> a full revert and simply marked the change for broken so that it can be
> worked on upstream with the wider audience. Linus appears to be fine with
> this approach, as he helped me with my "mark for BROKEN" patch.
> 
> Mathieu's concern is that this header file could be used in older distros
> with newer kernels that have it implemented and added this to keep out of
> those older distros.
> 
> The options to make Mathieu sleep better at night are:
> 
> 1) this patch
> 
> 2) move this file out of uapi.

I would be fine with this approach as well. This is simple enough:

git mv include/uapi/linux/user_events.h include/linux/

and:

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index 8b3d241a31c2..823d7b09dcba 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -18,7 +18,7 @@
 #include <linux/tracefs.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
-#include <uapi/linux/user_events.h>
+#include <linux/user_events.h>
 #include "trace.h"
 #include "trace_dynevent.h"

Including <linux/user_events.h> will continue to work even when the header is
moved to uapi in the future.

Thanks,

Mathieu


> 
> 3) revert the entire thing.
> 
> I really do not want to do #3 but I am willing to do 1 or 2.
> 
> -- Steve

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

Re: [PATCH] tracing: do not export user_events uapi

[Steven Rostedt]

On Thu, 31 Mar 2022 12:07:28 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 8b3d241a31c2..823d7b09dcba 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
> @@ -18,7 +18,7 @@
>  #include <linux/tracefs.h>
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
> -#include <uapi/linux/user_events.h>
> +#include <linux/user_events.h>
>  #include "trace.h"
>  #include "trace_dynevent.h"
> 
> Including <linux/user_events.h> will continue to work even when the header is
> moved to uapi in the future.

Actually, when I thought of this, I was thinking more of:

diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
index 846c27bc7aef..0f3aa855cf72 100644
--- a/kernel/trace/trace_events_user.c
+++ b/kernel/trace/trace_events_user.c
@@ -18,7 +18,11 @@
 #include <linux/tracefs.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
+#ifdef CONFIG_COMPILE_TEST
+#include <linux/user_events.h>
+#else
 #include <uapi/linux/user_events.h>
+#endif
 #include "trace.h"
 #include "trace_dynevent.h"
 
That way, when we take it out of broken state, it will fail to compile, and
remind us to put it back into the uapi directory.

-- Steve

Re: [PATCH] tracing: do not export user_events uapi

[Masami Hiramatsu]

On Wed, 30 Mar 2022 16:17:55 -0400
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> In addition to mark the USER_EVENTS feature BROKEN until all interested
> parties figure out the user-space API, do not install the uapi header.
> 
> This prevents situations where a non-final uapi header would end up
> being installed into a distribution image and used to build user-space
> programs that would then run against newer kernels that will implement
> user events with a different ABI.
> 
> Link: https://lore.kernel.org/all/20220330155835.5e1f6669@gandalf.local.home
> 

Looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
>  include/uapi/Kbuild | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/uapi/Kbuild b/include/uapi/Kbuild
> index 61ee6e59c930..425ea8769ddc 100644
> --- a/include/uapi/Kbuild
> +++ b/include/uapi/Kbuild
> @@ -12,3 +12,6 @@ ifeq ($(wildcard $(objtree)/arch/$(SRCARCH)/include/generated/uapi/asm/kvm_para.
>  no-export-headers += linux/kvm_para.h
>  endif
>  endif
> +
> +# API is not finalized
> +no-export-headers += linux/user_events.h
> -- 
> 2.20.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>