[RFC][PATCH] tracing: Define "fake" struct trace_pid_list

[RFC][PATCH] tracing: Define "fake" struct trace_pid_list

[Steven Rostedt]

[ Note, this is on top of my tree in ftrace/core, but wanted to ask if
  this is the proper "fix". I moved the struct trace_pid_list into a
  separate file to have more control over it, and only declare the
  structure to be passed by pointers. It is protected by RCU sched, and
  uses the rcu_dereference_sched() to retrieve the pointer, but while
  testing it against gcc 8, it gave the error below. It compiles fine
  on gcc 10. The issue is that on gcc 8, the "typeof(*p)" used in
  rcu_dereference_sched() causes the "incomplete type" error, because
  it does a "*p" where p is a pointer to the undefined struct
  trace_pid_list. To get around this error, I declared struct
  trace_pid_list as the following:

    struct trace_pid_list {
	volatile void *ignore;
    };

  With a #ifdef around it to allow it to be declared properly where it
  is modified, but all other uses has this fake structure pointer.
  This is obviously a hack workaround. But since we support gcc 8, and
  I don't want to expose this structure for anything else, is this OK
  to do?
]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

Some compilers give this error:

kernel/trace/ftrace.c: In function 'ftrace_filter_pid_sched_switch_probe':
include/linux/rcupdate.h:389:9: error: dereferencing pointer to incomplete type 'struct trace_pid_list'
  typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); \
         ^
include/linux/rcupdate.h:558:2: note: in expansion of macro '__rcu_dereference_check'
  __rcu_dereference_check((p), (c) || rcu_read_lock_sched_held(), \
  ^~~~~~~~~~~~~~~~~~~~~~~
include/linux/rcupdate.h:612:34: note: in expansion of macro 'rcu_dereference_sched_check'
 #define rcu_dereference_sched(p) rcu_dereference_sched_check(p, 0)
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
kernel/trace/ftrace.c:7101:13: note: in expansion of macro 'rcu_dereference_sched'
  pid_list = rcu_dereference_sched(tr->function_pids);
             ^~~~~~~~~~~~~~~~~~~~~

The reason is that rcu_dereference_sched() has a check that uses
typeof(*p) of the pointer passed to it. But here, the pointer is of type
"struct trace_pid_list *" which is abstracted out, and nothing outside of
pid_list.c should care what the content of it is. But the check uses
typeof(*p) and on some (not all) compilers, it errors with the
dereferencing pointer to incomplete type, which is totally bogus here.

Instead of just declaring "struct trace_pid_list", define it as a
structure with a volatile pointer (just to keep the compiler from doing any
optimization tricks).

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/pid_list.c |  1 +
 kernel/trace/trace.h    | 14 +++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/pid_list.c b/kernel/trace/pid_list.c
index 6d1e6cda6973..c4b1bbc59b7b 100644
--- a/kernel/trace/pid_list.c
+++ b/kernel/trace/pid_list.c
@@ -5,6 +5,7 @@
 #include <linux/spinlock.h>
 #include <linux/irq_work.h>
 #include <linux/slab.h>
+#define DEFINED_PID_LIST
 #include "trace.h"
 
 /*
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index fe13a0542486..46323ceed8e8 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -188,7 +188,19 @@ struct trace_options {
 	struct trace_option_dentry	*topts;
 };
 
-struct trace_pid_list;
+#ifndef DEFINED_PID_LIST
+/*
+ * rcu_dereference_sched() does typeof(*p) on struct trace_pid_list *, and that
+ * causes some compiler versions to error, with "dereferencing pointer to incomplete type"
+ * because the "*p" of "typeof(*p)" dereferences the pointer to trace_pid_list.
+ * As nothing should need to know that structure, and it should remain
+ * abstracted, define a fake structure for all uses, and define it where it is
+ * actually updated.
+ */
+struct trace_pid_list {
+	volatile void *ignore;	/* Add volatile just to keep from any tricky optimizations */
+};
+#endif
 
 struct trace_pid_list *trace_pid_list_alloc(void);
 void trace_pid_list_free(struct trace_pid_list *pid_list);
-- 
2.31.1


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity and top posting.

Re: [RFC][PATCH] tracing: Define "fake" struct trace_pid_list

[Linus Torvalds]

On Sat, Oct 2, 2021 at 1:04 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
>
> [ Note, this is on top of my tree in ftrace/core, but wanted to ask if
>   this is the proper "fix".

Ugh, please no. This is going to be very confusing, and it's going to
mess with anything that does things based on type (eg traditionally
module signatures etc).

I'd rather you just expose the proper type, if that is what it takes.

> Some compilers give this error:

Only some? Which ones? And what did you do to make it appear? Sounds
like whatever change wasn't worth it.

The advantage of some "opaque type" does _not_ override the
disadvantage of then having to make up these kinds of horrific
workarounds that actively lie to the compiler.

We have tons of structures (and occasionally single structure members)
that we don't want people to access directly, and instead use a
wrapper function. That doesn't mean that they can't be exposed as a
type.

> The reason is that rcu_dereference_sched() has a check that uses
> typeof(*p) of the pointer passed to it.

Sadly, we do that for a reason - we do a

     typeof(*p) *__local_p;

to drop the address space specifiers from (or add them to) the pointer.

That said, I wonder how many of them are actually needed. At least
some of them are purely for sparse

So at least some could probably just use

     typeof(p) __local_p;

instead, which would avoid the problem with a pointer to an incomplete
type (and keep it as a pointer to an incomplete type).

So one option might be to work on the RCU accessor macros instead.

               Linus

Re: [RFC][PATCH] tracing: Define "fake" struct trace_pid_list

[Steven Rostedt]

On Sat, 2 Oct 2021 15:39:45 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, Oct 2, 2021 at 1:04 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >
> > [ Note, this is on top of my tree in ftrace/core, but wanted to ask if
> >   this is the proper "fix".  
> 
> Ugh, please no. This is going to be very confusing, and it's going to
> mess with anything that does things based on type (eg traditionally
> module signatures etc).

This is why I asked ;-)

> 
> I'd rather you just expose the proper type, if that is what it takes.

I can, as I'm currently the only one using this, it should be fine. I
just like to not expose structures that shouldn't be touched, but
that's my preference, but that's not really that common of a habit in
the kernel anyway.

> 
> > Some compilers give this error:  
> 
> Only some? Which ones? And what did you do to make it appear? Sounds
> like whatever change wasn't worth it.

Yes, this is what surprised me. It worked on all my machines and for
most of my tests, which are pretty much all gcc 10.X. But for one of my
tests, I compile with gcc 8.1.0 (one I pulled down from kernel.org a
while ago), and that's the one that blew up.

It has nothing to do with the config (the same config compiles fine
with gcc 10.x). And if I didn't have a test that compiled with 8.1.0, I
would never had know this was an issue.

> 
> The advantage of some "opaque type" does _not_ override the
> disadvantage of then having to make up these kinds of horrific
> workarounds that actively lie to the compiler.

I felt uncomfortable with the change, and that's why I wanted to get
your opinion before having you first see it in a pull request.

> 
> We have tons of structures (and occasionally single structure members)
> that we don't want people to access directly, and instead use a
> wrapper function. That doesn't mean that they can't be exposed as a
> type.
> 
> > The reason is that rcu_dereference_sched() has a check that uses
> > typeof(*p) of the pointer passed to it.  
> 
> Sadly, we do that for a reason - we do a
> 
>      typeof(*p) *__local_p;
> 
> to drop the address space specifiers from (or add them to) the pointer.
> 
> That said, I wonder how many of them are actually needed. At least
> some of them are purely for sparse
> 
> So at least some could probably just use
> 
>      typeof(p) __local_p;
> 
> instead, which would avoid the problem with a pointer to an incomplete
> type (and keep it as a pointer to an incomplete type).
> 
> So one option might be to work on the RCU accessor macros instead.

I looked at changing them, but for the one place:

	((typeof(*p) __force __kernel *)(_________p1)); 

Where there's a separation from the type and adding of "__force __kernel"
before the pointer. Not sure if it matters or not.

I'll do a little investigation, and see if tweaks to these RCU macros
will fix it, otherwise, I'll just move the structure back out to being
public.

Thanks for the feedback,

-- Steve