From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-10-05 13:47:35
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
After moving a structure into a local file and only declaring it for users
of that structure to have just a pointer to it, some compilers gave 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.
Since the logic creates a pointer of each of these instances of
typeof(*p), just use typeof(p).
That is, instead of declaring: typeof(*p) *_p; just do:
typeof(p) _p;
Also had to update a lot of the function pointer initialization in the
networking code, as a function address must be passed as an argument in
RCU_INIT_POINTER() and not just the function name, otherwise the following
error occurs:
In file included from ./arch/x86/include/generated/asm/rwonce.h:1,
from ./include/linux/compiler.h:266,
from ./include/linux/init.h:5,
from ./include/linux/netfilter.h:5,
from ./net/netfilter/nf_conntrack_core.c:15:
./net/netfilter/nf_conntrack_core.c: In function 'nf_conntrack_init_end':
./include/linux/rcupdate.h:411:28: error: cast specifies function type
#define RCU_INITIALIZER(v) (typeof((v)) __force __rcu)(v)
^
./include/asm-generic/rwonce.h:55:33: note: in definition of macro '__WRITE_ONCE'
*(volatile typeof(x) *)&(x) = (val); \
^~~
./include/linux/rcupdate.h:853:3: note: in expansion of macro 'WRITE_ONCE'
WRITE_ONCE(p, RCU_INITIALIZER(v)); \
^~~~~~~~~~
./include/linux/rcupdate.h:853:17: note: in expansion of macro 'RCU_INITIALIZER'
WRITE_ONCE(p, RCU_INITIALIZER(v)); \
^~~~~~~~~~~~~~~
./net/netfilter/nf_conntrack_core.c:2736:2: note: in expansion of macro 'RCU_INIT_POINTER'
RCU_INIT_POINTER(ip_ct_attach, nf_conntrack_attach);
^~~~~~~~~~~~~~~~
Link: https://lore.kernel.org/all/20211002195718.0b5d9b67@oasis.local.home/
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
include/linux/rcupdate.h | 20 ++++++++++----------
net/ipv4/netfilter/nf_nat_h323.c | 18 +++++++++---------
net/ipv4/netfilter/nf_nat_pptp.c | 8 ++++----
net/ipv4/netfilter/nf_nat_snmp_basic_main.c | 2 +-
net/netfilter/nf_conntrack_core.c | 2 +-
net/netfilter/nf_nat_amanda.c | 2 +-
net/netfilter/nf_nat_ftp.c | 2 +-
net/netfilter/nf_nat_irc.c | 2 +-
net/netfilter/nf_nat_tftp.c | 2 +-
net/netfilter/nfnetlink_cttimeout.c | 4 ++--
10 files changed, 31 insertions(+), 31 deletions(-)
----- On Oct 5, 2021, at 9:47 AM, rostedt rostedt@goodmis.org wrote:
[...]
#define rcu_dereference_raw(p) \
({ \
/* Dependency order vs. p above. */ \
typeof(p) ________p1 = READ_ONCE(p); \
- ((typeof(*p) __force __kernel *)(________p1)); \
+ ((typeof(p) __force __kernel)(________p1)); \
})
AFAIU doing so removes validation that @p is indeed a pointer, so a user might mistakenly
try to use rcu_dereference() on an integer, and get away with it. I'm not sure we want to
loosen this check. I wonder if there might be another way to achieve the same check without
requiring the structure to be declared, e.g. with __builtin_types_compatible_p ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-10-05 15:58:23
On Tue, 5 Oct 2021 11:15:12 -0400 (EDT)
Mathieu Desnoyers [off-list ref] wrote:
----- On Oct 5, 2021, at 9:47 AM, rostedt rostedt@goodmis.org wrote:
[...]
quoted
#define rcu_dereference_raw(p) \
({ \
/* Dependency order vs. p above. */ \
typeof(p) ________p1 = READ_ONCE(p); \
- ((typeof(*p) __force __kernel *)(________p1)); \
+ ((typeof(p) __force __kernel)(________p1)); \
})
AFAIU doing so removes validation that @p is indeed a pointer, so a user might mistakenly
try to use rcu_dereference() on an integer, and get away with it. I'm not sure we want to
loosen this check. I wonder if there might be another way to achieve the same check without
requiring the structure to be declared, e.g. with __builtin_types_compatible_p ?
Is that really an issue? Because you would be assigning it to an integer.
x = rcu_dereference_raw(y);
And that just makes 'x' a copy of 'y' and not really a reference to it, thus
if you don't have a pointer, it's just a fancy READ_ONCE(y).
-- Steve
----- On Oct 5, 2021, at 11:58 AM, rostedt rostedt@goodmis.org wrote:
On Tue, 5 Oct 2021 11:15:12 -0400 (EDT)
Mathieu Desnoyers [off-list ref] wrote:
quoted
----- On Oct 5, 2021, at 9:47 AM, rostedt rostedt@goodmis.org wrote:
[...]
quoted
#define rcu_dereference_raw(p) \
({ \
/* Dependency order vs. p above. */ \
typeof(p) ________p1 = READ_ONCE(p); \
- ((typeof(*p) __force __kernel *)(________p1)); \
+ ((typeof(p) __force __kernel)(________p1)); \
})
AFAIU doing so removes validation that @p is indeed a pointer, so a user might
mistakenly
try to use rcu_dereference() on an integer, and get away with it. I'm not sure
we want to
loosen this check. I wonder if there might be another way to achieve the same
check without
requiring the structure to be declared, e.g. with __builtin_types_compatible_p ?
Is that really an issue? Because you would be assigning it to an integer.
x = rcu_dereference_raw(y);
And that just makes 'x' a copy of 'y' and not really a reference to it, thus
if you don't have a pointer, it's just a fancy READ_ONCE(y).
See Documentation/RCU/arrayRCU.rst:
"It might be tempting to consider use
of RCU to instead protect the index into an array, however, this use
case is **not** supported. The problem with RCU-protected indexes into
arrays is that compilers can play way too many optimization games with
integers, which means that the rules governing handling of these indexes
are far more trouble than they are worth. If RCU-protected indexes into
arrays prove to be particularly valuable (which they have not thus far),
explicit cooperation from the compiler will be required to permit them
to be safely used."
So AFAIU validation that rcu_dereference receives a pointer as parameter
is done on purpose.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
On Tue, Oct 5, 2021 at 6:47 AM Steven Rostedt [off-list ref] wrote:
Also had to update a lot of the function pointer initialization in the
networking code, as a function address must be passed as an argument in
RCU_INIT_POINTER() and not just the function name, otherwise the following
error occurs:
Ugh.
I think this is a sign of why we did it the way we did with that odd
"typeof(*p)*" thing in the first place.
The thing is, in any normal C, the function name should just stand in
for the pointer to the function, so having to add a '&' to get the
function pointer is somehow odd..
So I think you should just expose your type to anybody who uses a pointer to it.
Linus
From: "Paul E. McKenney" <paulmck@kernel.org> Date: 2021-10-05 16:29:20
On Tue, Oct 05, 2021 at 12:15:04PM -0400, Mathieu Desnoyers wrote:
----- On Oct 5, 2021, at 11:58 AM, rostedt rostedt@goodmis.org wrote:
quoted
On Tue, 5 Oct 2021 11:15:12 -0400 (EDT)
Mathieu Desnoyers [off-list ref] wrote:
quoted
----- On Oct 5, 2021, at 9:47 AM, rostedt rostedt@goodmis.org wrote:
[...]
quoted
#define rcu_dereference_raw(p) \
({ \
/* Dependency order vs. p above. */ \
typeof(p) ________p1 = READ_ONCE(p); \
- ((typeof(*p) __force __kernel *)(________p1)); \
+ ((typeof(p) __force __kernel)(________p1)); \
})
AFAIU doing so removes validation that @p is indeed a pointer, so a user might
mistakenly
try to use rcu_dereference() on an integer, and get away with it. I'm not sure
we want to
loosen this check. I wonder if there might be another way to achieve the same
check without
requiring the structure to be declared, e.g. with __builtin_types_compatible_p ?
Is that really an issue? Because you would be assigning it to an integer.
x = rcu_dereference_raw(y);
And that just makes 'x' a copy of 'y' and not really a reference to it, thus
if you don't have a pointer, it's just a fancy READ_ONCE(y).
See Documentation/RCU/arrayRCU.rst:
"It might be tempting to consider use
of RCU to instead protect the index into an array, however, this use
case is **not** supported. The problem with RCU-protected indexes into
arrays is that compilers can play way too many optimization games with
integers, which means that the rules governing handling of these indexes
are far more trouble than they are worth. If RCU-protected indexes into
arrays prove to be particularly valuable (which they have not thus far),
explicit cooperation from the compiler will be required to permit them
to be safely used."
So AFAIU validation that rcu_dereference receives a pointer as parameter
is done on purpose.
What Mathieu said!
On the other hand, I am starting to believe that explicit cooperation
from compilers might actually be forthcoming in my lifetime, so there
might well be that...
Thanx, Paul
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-10-05 16:40:24
On Tue, 5 Oct 2021 12:15:04 -0400 (EDT)
Mathieu Desnoyers [off-list ref] wrote:
See Documentation/RCU/arrayRCU.rst:
"It might be tempting to consider use
of RCU to instead protect the index into an array, however, this use
Ah, array indexes. Now that makes sense.
case is **not** supported. The problem with RCU-protected indexes into
arrays is that compilers can play way too many optimization games with
integers, which means that the rules governing handling of these indexes
are far more trouble than they are worth. If RCU-protected indexes into
arrays prove to be particularly valuable (which they have not thus far),
explicit cooperation from the compiler will be required to permit them
to be safely used."
So AFAIU validation that rcu_dereference receives a pointer as parameter
is done on purpose.
Thanks for looking at this. I'll go punt and just expose the structure.
It's not a big deal, but I like abstraction of structures when they can be,
just to keep from the temptation of tweaking them directly, and causing
updates later to be more difficult.
Too bad that the failure here is not RCU or the macros, but what I would
call a bug in a specific compiler.
-- Steve
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-10-05 16:42:16
On Tue, 5 Oct 2021 09:18:04 -0700
Linus Torvalds [off-list ref] wrote:
On Tue, Oct 5, 2021 at 6:47 AM Steven Rostedt [off-list ref] wrote:
quoted
Also had to update a lot of the function pointer initialization in the
networking code, as a function address must be passed as an argument in
RCU_INIT_POINTER() and not just the function name, otherwise the following
error occurs:
Ugh.
I think this is a sign of why we did it the way we did with that odd
"typeof(*p)*" thing in the first place.
Oh, which was also compiler specific.
On my fedora box, which has:
gcc version 10.3.1 20210422 (Red Hat 10.3.1-1) (GCC)
It compiled fine, with no errors.
But on my Debian box with:
gcc version 10.2.1 20210110 (Debian 10.2.1-6)
The function pointers were an issue :-/
-- Steve
On Tue, Oct 5, 2021 at 9:37 AM Steven Rostedt [off-list ref] wrote:
Oh well, it was a fun exercise. Too bad we failed due to inconsistencies in
compilers :-(
I'm admittedly surprised that something like this would be a
"different compiler versions" issue. But "typeof()" isn't exactly
standard C, so the fact that some version of gcc did something
slightly different is annoying but I guess not _that_ surprising.
Oh well indeed,
Linus
That is, instead of declaring: typeof(*p) *_p; just do:
typeof(p) _p;
Also had to update a lot of the function pointer initialization in the
networking code, as a function address must be passed as an argument in
RCU_INIT_POINTER()
I would think that one could avoid that churn by saying
typeof((p) + 0)
instead of just "typeof(p)", to force the decay to a pointer.
Rasmus
----- On Oct 5, 2021, at 2:01 PM, Rasmus Villemoes linux@rasmusvillemoes.dk wrote:
On 05/10/2021 15.47, Steven Rostedt wrote:
quoted
That is, instead of declaring: typeof(*p) *_p; just do:
typeof(p) _p;
Also had to update a lot of the function pointer initialization in the
networking code, as a function address must be passed as an argument in
RCU_INIT_POINTER()
I would think that one could avoid that churn by saying
typeof((p) + 0)
instead of just "typeof(p)", to force the decay to a pointer.
If the type of @p is an integer, (p) + 0 is still valid, so it will not
prevent users from passing an integer type as argument, which is what
the current implementation prevents.
Also, AFAIU, the compiler wants to know the sizeof(p) in order to evaluate
(p + 0). Steven's goal is to hide the structure declaration, so that would
not work either.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
From: Jan Engelhardt <hidden> Date: 2021-10-05 18:29:01
On Tuesday 2021-10-05 20:06, Mathieu Desnoyers wrote:
quoted
instead of just "typeof(p)", to force the decay to a pointer.
If the type of @p is an integer, (p) + 0 is still valid, so it will not
prevent users from passing an integer type as argument, which is what
the current implementation prevents.
Also, AFAIU, the compiler wants to know the sizeof(p) in order to evaluate
(p + 0). Steven's goal is to hide the structure declaration, so that would
not work either.
#define static_cast(type, expr) ((struct { type x; }){(expr)}.x)
typeof(p) p1 = (typeof(p) __force)static_cast(void *, READ_ONCE(p));
Let the name not fool you; it's absolutely _not_ the same as C++'s
static_cast, but still: it does emit a warning when you do pass an
integer, which is better than no warning at all in that case.
*flies away*
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-10-05 18:40:09
On Tue, 5 Oct 2021 20:28:54 +0200 (CEST)
Jan Engelhardt [off-list ref] wrote:
On Tuesday 2021-10-05 20:06, Mathieu Desnoyers wrote:
quoted
quoted
instead of just "typeof(p)", to force the decay to a pointer.
If the type of @p is an integer, (p) + 0 is still valid, so it will not
prevent users from passing an integer type as argument, which is what
the current implementation prevents.
Also, AFAIU, the compiler wants to know the sizeof(p) in order to evaluate
(p + 0). Steven's goal is to hide the structure declaration, so that would
not work either.
#define static_cast(type, expr) ((struct { type x; }){(expr)}.x)
typeof(p) p1 = (typeof(p) __force)static_cast(void *, READ_ONCE(p));
Let the name not fool you; it's absolutely _not_ the same as C++'s
static_cast, but still: it does emit a warning when you do pass an
integer, which is better than no warning at all in that case.
*flies away*
Are you suggesting I should continue this exercise ;-)
-- Steve
----- On Oct 5, 2021, at 2:01 PM, Rasmus Villemoes linux@rasmusvillemoes.dk wrote:
quoted
I would think that one could avoid that churn by saying
typeof((p) + 0)
instead of just "typeof(p)", to force the decay to a pointer.
Also, AFAIU, the compiler wants to know the sizeof(p) in order to evaluate
(p + 0). Steven's goal is to hide the structure declaration, so that would
not work either.
Gah, you're right. I was hoping the frontend would see that +0 could be
optimized away and only affect the type of the expression, but it does
give 'error: invalid use of undefined type ‘struct abc’'. Sorry for the
noise.
Rasmus
#define static_cast(type, expr) ((struct { type x; }){(expr)}.x)
typeof(p) p1 = (typeof(p) __force)static_cast(void *, READ_ONCE(p));
Let the name not fool you; it's absolutely _not_ the same as C++'s
static_cast, but still: it does emit a warning when you do pass an
integer, which is better than no warning at all in that case.
*flies away*
Are you suggesting I should continue this exercise ;-)
#define static_cast(type, expr) ((struct { type x; }){(expr)}.x)
typeof(p) p1 = (typeof(p) __force)static_cast(void *, READ_ONCE(p));
Let the name not fool you; it's absolutely _not_ the same as C++'s
static_cast, but still: it does emit a warning when you do pass an
integer, which is better than no warning at all in that case.
*flies away*
Are you suggesting I should continue this exercise ;-)
I may try it, because exposing the structure I want to hide, is pulling out
a lot of other crap with it :-p
struct trace_pid_list {
raw_spinlock_t lock;
struct irq_work refill_irqwork;
union upper_chunk *upper[UPPER1_SIZE]; // 1 or 2K in size
union upper_chunk *upper_list;
union lower_chunk *lower_list;
int free_upper_chunks;
int free_lower_chunks;
};
I can still abstract out the unions, but this means I need to also pull out
the define of "UPPER1_SIZE". Not to mention, I need to make sure irq_work
and spin locks are defined.
Another approach is to have it return:
struct trace_pid_list {
unsigned long ignore;
};
Rename the above struct trace_pid_list to struct trace_pid_internal.
And internally have:
union trace_pid_data {
struct trace_pid_list external;
struct trace_pid_internal internal;
};
Then use the internal version within the C file that modifies it, and just
return a pointer to the external part.
That should follow the "C standard".
-- Steve
On Tue, Oct 5, 2021 at 12:40 PM Steven Rostedt [off-list ref] wrote:
I may try it, because exposing the structure I want to hide, is pulling out
a lot of other crap with it :-p
One option is just "don't do rcu_access of a pointer that you're not
supposed to touch in a file that isn't supposed to touch it".
IOW, why are you doing that
pid_list = rcu_dereference_sched(tr->function_pids);
in a place that isn't supposed to look at the pid_list in the first place?
Yeah, yeah, I see how you just pass it to trace_ignore_this_task() as
an argument, but maybe the real fix is to just pass that trace_array
pointer instead?
IOW, if you want to keep that structure private, maybe you really just
shouldn't have non-private users of it randomly doing RCU lookups of
it?
Linus
#define static_cast(type, expr) ((struct { type x; }){(expr)}.x)
typeof(p) p1 = (typeof(p) __force)static_cast(void *, READ_ONCE(p));
Let the name not fool you; it's absolutely _not_ the same as C++'s
static_cast, but still: it does emit a warning when you do pass an
integer, which is better than no warning at all in that case.
*flies away*
Are you suggesting I should continue this exercise ;-)
I may try it, because exposing the structure I want to hide, is pulling out
a lot of other crap with it :-p
I like the static_cast() approach above. It is neat way to validate that the
argument is a pointer without need to dereference the pointer.
I would also be open to consider this trick for liburcu's userspace API.
About the other proposed solution based on __builtin_classify_type, I am
reluctant to use something designed specifically for varargs in a context
where they are not used.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-10-05 20:03:00
On Tue, 5 Oct 2021 12:46:43 -0700
Linus Torvalds [off-list ref] wrote:
On Tue, Oct 5, 2021 at 12:40 PM Steven Rostedt [off-list ref] wrote:
quoted
I may try it, because exposing the structure I want to hide, is pulling out
a lot of other crap with it :-p
One option is just "don't do rcu_access of a pointer that you're not
supposed to touch in a file that isn't supposed to touch it".
The problem is, the RCU isn't for touching it, it is for knowing it exists.
IOW, why are you doing that
pid_list = rcu_dereference_sched(tr->function_pids);
in a place that isn't supposed to look at the pid_list in the first place?
Yeah, yeah, I see how you just pass it to trace_ignore_this_task() as
an argument, but maybe the real fix is to just pass that trace_array
pointer instead?
IOW, if you want to keep that structure private, maybe you really just
shouldn't have non-private users of it randomly doing RCU lookups of
it?
Ideally, I wanted to keep the logic of the pid lists separate, and not have
it know about the trace array at all.
And this was the best "incremental" approach I had, as the code is
currently all just open coded.
The RCU lookups are not an internal use functionality of the pid lists. The
updates to the pid list are done by allocating a new pid_list, copying the
old pid_list with the new updates and then swapping the pointers. The logic
of the pid_list is orthogonal to the RCU update. It's just "allocate some
random thing" and use RCU to swap it with the old random thing.
That is, the logic to synchronize updates is left to the user not the pid
list itself.
I also want to limit function calls, as this is called from the function
tracer (every function being traced) and if the pid_list pointer is NULL it
skips it. Hence, I want to avoid calling a function to know if the pointer
is NULL or not.
-- Steve
#define static_cast(type, expr) ((struct { type x; }){(expr)}.x)
typeof(p) p1 = (typeof(p) __force)static_cast(void *, READ_ONCE(p));
Let the name not fool you; it's absolutely _not_ the same as C++'s
static_cast, but still: it does emit a warning when you do pass an
integer, which is better than no warning at all in that case.
*flies away*
Are you suggesting I should continue this exercise ;-)
I may try it, because exposing the structure I want to hide, is pulling out
a lot of other crap with it :-p
I like the static_cast() approach above. It is neat way to validate that the
argument is a pointer without need to dereference the pointer.
I would also be open to consider this trick for liburcu's userspace API.
About the other proposed solution based on __builtin_classify_type, I am
reluctant to use something designed specifically for varargs in a context
where they are not used.
Unfortunately, it doesn't solve the Debian gcc 10 compiler failing when
passing the function name instead of a pointer to the function in
RCU_INIT_POINTER() That alone makes me feel like I shouldn't touch that
macro :-(
And who knows what other version of gcc will fail on passing the address :-p
-- Steve
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-10-05 20:38:03
On Tue, 5 Oct 2021 15:40:29 -0400
Steven Rostedt [off-list ref] wrote:
struct trace_pid_list {
unsigned long ignore;
};
Rename the above struct trace_pid_list to struct trace_pid_internal.
And internally have:
union trace_pid_data {
struct trace_pid_list external;
struct trace_pid_internal internal;
};
Then use the internal version within the C file that modifies it, and just
return a pointer to the external part.
So this has proved to be a PITA.
That should follow the "C standard".
Really, thinking about abstraction, I don't believe there's anything wrong
with returning a pointer of one type, and then typecasting it to a pointer
of another type. Is there? As long as whoever uses the returned type does
nothing with it.
That is, if I simply do:
In the header file:
struct trace_pid_list {
void *ignore;
};
struct trace_pid_list *trace_pid_list_alloc(void);
void trace_pid_list_free(struct trace_pid_list *pid_list);
And then in the C file:
struct pid_list {
[...]
};
struct trace_pid_list *trace_pid_list_alloc(void)
{
struct pid_list *pid_list;
pid_list = kmalloc(sizeof(*pid_list), GFP_KERNEL);
[..]
return (struct trace_pid_list *)pid_list;
}
void trace_pid_list_free(struct trace_pid_list *list)
{
struct pid_list *pid_list = (struct pid_list *)list;
[..]
kfree(pid_list);
}
That should be perfectly fine for standard C. Right?
-- Steve
On Tue, Oct 5, 2021 at 1:38 PM Steven Rostedt [off-list ref] wrote:
Really, thinking about abstraction, I don't believe there's anything wrong
with returning a pointer of one type, and then typecasting it to a pointer
of another type. Is there? As long as whoever uses the returned type does
nothing with it.
Just stop doing this.,
Dammit, just include the header file that defines the type in the
places that you use the thing.
Because, yes, there is a LOT wrong with just randomly casting pointers
that you think have the "wrong type". You're basically taking it on
yourself to lie to the compiler, and intentionally breaking the type
system, because you have some completely bogus reason to hide a type.
We don't hide types in the kernel for no good reason.
You are literally talking about making things worse, for a reason that
hasn't even been explained, and isn't valid in the first place.
Nothing else in the kernel has had a problem just declaring the damn
type,.
If there was some clean and simple solution to the compiler warning
problem, that would be one thing. But when you think you need to
change core RCU macros, or lie to the compiler about the type system,
at that point it's not some clean and simple fix any more. At that
point you're literally making things worse than just exposing the
type.
Linus
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-10-05 21:05:25
On Tue, 5 Oct 2021 13:45:52 -0700
Linus Torvalds [off-list ref] wrote:
If there was some clean and simple solution to the compiler warning
problem, that would be one thing. But when you think you need to
change core RCU macros, or lie to the compiler about the type system,
at that point it's not some clean and simple fix any more. At that
point you're literally making things worse than just exposing the
type.
Fine, I'll just create a separate header file with all that is needed and
add it to the include. At least that way, it doesn't muck up the rest of
the header file.
-- Steve
From: Jan Engelhardt <hidden> Date: 2021-10-05 21:09:13
On Tuesday 2021-10-05 22:37, Steven Rostedt wrote:
Really, thinking about abstraction, I don't believe there's anything wrong
with returning a pointer of one type, and then typecasting it to a pointer
of another type. Is there? As long as whoever uses the returned type does
nothing with it.
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-10-05 21:24:41
On Tue, 5 Oct 2021 23:09:08 +0200 (CEST)
Jan Engelhardt [off-list ref] wrote:
On Tuesday 2021-10-05 22:37, Steven Rostedt wrote:
quoted
Really, thinking about abstraction, I don't believe there's anything wrong
with returning a pointer of one type, and then typecasting it to a pointer
of another type. Is there? As long as whoever uses the returned type does
nothing with it.
Basically (one alternative I was looking at) was simply passing around a
void pointer. Not sure how the RCU macros would handle that. But to
completely abstract it out, I was thinking of just returning void * and
accepting void *, but I didn't want to do that because now we just lost any
kind of type checking done by the compiler. The tricks I was playing was to
keep some kind of type checking.
struct trace_pid_list { void *pid_list; };
struct trace_pid_list trace_pid_list_alloc(void)
{
struct trace_pid_list t;
t.pid_list = kmalloc(sizeof(t.orig), GFP_KERNEL);
return t;
}
void freethat(struct strace_pid_list x)
{
kfree(x.pid_list);
}
Might run afoul of -Waggregate-return in C.
The above isn't exactly what I was suggesting.
And really, not that I'm going to do this, I could have followed the rest
of the kernel with:
struct trace_pid_list {
int max;
[..]
};
int *trace_pid_list_alloc(void)
{
struct trace_pid_list *pid_list;
pid_list = kmalloc(sizeof(*pid_list), GFP_KERNEL);
[..]
return &pid_list->max;
}
void trace_pid_list_free(int *p)
{
struct trace_pid_list *pid_list = container_of(p, struct pid_list, max);
[..]
free(pid_list);
}
Because we do this all over the kernel. Talk about lying to the compiler ;-)
-- Steve
Well, we happily end up casting pointers to 'unsigned long' and back,
and doing bit games on the low bits of a pointer value.
So it's not like the kernel deeply cares about theoretical portability.
But I do discourage casting when not required, just because as much
static type checking we can possibly have is good when we can do it.
Linus
So it's not like the kernel deeply cares about theoretical portability.
All things considered, that's good enough.
Needless to say, how many times has the compiler changed and then
someone complaind and someone else replied "standard told you so". ;-)
From: David Laight <hidden> Date: 2021-10-11 08:34:19
From: Steven Rostedt
Sent: 05 October 2021 22:25
...
Basically (one alternative I was looking at) was simply passing around a
void pointer. Not sure how the RCU macros would handle that. But to
completely abstract it out, I was thinking of just returning void * and
accepting void *, but I didn't want to do that because now we just lost any
kind of type checking done by the compiler. The tricks I was playing was to
keep some kind of type checking.
Yes, don't use 'void *'.
Sun used it for all the pointers in their DDI/DKI - made it impossible
to ensure you were passing in the right type of 'token'.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: David Laight <hidden> Date: 2021-10-11 08:39:43
From: Mathieu Desnoyers
Sent: 05 October 2021 16:15
----- On Oct 5, 2021, at 9:47 AM, rostedt rostedt@goodmis.org wrote:
[...]
quoted
#define rcu_dereference_raw(p) \
({ \
/* Dependency order vs. p above. */ \
typeof(p) ________p1 = READ_ONCE(p); \
- ((typeof(*p) __force __kernel *)(________p1)); \
+ ((typeof(p) __force __kernel)(________p1)); \
})
AFAIU doing so removes validation that @p is indeed a pointer, so a user might mistakenly
try to use rcu_dereference() on an integer, and get away with it. I'm not sure we want to
loosen this check. I wonder if there might be another way to achieve the same check without
requiring the structure to be declared, e.g. with __builtin_types_compatible_p ?
Could you pass the pointer to something like:
static __always_inline void foo(void *arg) {};
That would fail for integers.
Not sure whether CFI bleats about function pointers though.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
----- On Oct 11, 2021, at 4:39 AM, David Laight David.Laight@ACULAB.COM wrote:
From: Mathieu Desnoyers
quoted
Sent: 05 October 2021 16:15
----- On Oct 5, 2021, at 9:47 AM, rostedt rostedt@goodmis.org wrote:
[...]
quoted
#define rcu_dereference_raw(p) \
({ \
/* Dependency order vs. p above. */ \
typeof(p) ________p1 = READ_ONCE(p); \
- ((typeof(*p) __force __kernel *)(________p1)); \
+ ((typeof(p) __force __kernel)(________p1)); \
})
AFAIU doing so removes validation that @p is indeed a pointer, so a user might
mistakenly
try to use rcu_dereference() on an integer, and get away with it. I'm not sure
we want to
loosen this check. I wonder if there might be another way to achieve the same
check without
requiring the structure to be declared, e.g. with __builtin_types_compatible_p ?
Could you pass the pointer to something like:
static __always_inline void foo(void *arg) {};
That would fail for integers.
Not sure whether CFI bleats about function pointers though.
That would indeed validate that a pointer is being passed to rcu_dereference()
and RCU_INITIALIZER().
However it would not solve this other issue: in Steven's patch, rcu_dereference_raw
is changed like so:
#define rcu_dereference_raw(p) \
({ \
/* Dependency order vs. p above. */ \
typeof(p) ________p1 = READ_ONCE(p); \
- ((typeof(*p) __force __kernel *)(________p1)); \
+ ((typeof(p) __force __kernel)(________p1)); \
})
and AFAIU the __force __kernel attributes end up applying to the pointer rather than the
object pointed to, which changes the semantic.
So checking the pointer argument is not the only issue here.
As Linus pointed out, it might indeed be simpler to just keep declaring the structure in
public headers.
Thanks,
Mathieu
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)