| Message ID | 20211005094728.203ecef2@gandalf.local.home (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] rcu: Use typeof(p) instead of typeof(*p) * | expand |
----- 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
On Tue, 5 Oct 2021 11:15:12 -0400 (EDT) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > ----- 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 ? 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 <mathieu.desnoyers@efficios.com> wrote: > >> ----- 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 ? > > 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
On Tue, Oct 5, 2021 at 6:47 AM Steven Rostedt <rostedt@goodmis.org> 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
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: > > > On Tue, 5 Oct 2021 11:15:12 -0400 (EDT) > > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > > >> ----- 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 ? > > > > 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
On Tue, 5 Oct 2021 09:18:04 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> So I think you should just expose your type to anybody who uses a pointer to it.
Oh well, it was a fun exercise. Too bad we failed due to inconsistencies in
compilers :-(
-- Steve
On Tue, 5 Oct 2021 12:15:04 -0400 (EDT) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> 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
On Tue, 5 Oct 2021 09:18:04 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Oct 5, 2021 at 6:47 AM Steven Rostedt <rostedt@goodmis.org> 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. > 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 <rostedt@goodmis.org> 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
On 05/10/2021 15.47, Steven Rostedt wrote: > 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: > >> 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
On Tuesday 2021-10-05 20:06, Mathieu Desnoyers wrote: >> 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. >>>> typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); #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*
On Tue, 5 Oct 2021 20:28:54 +0200 (CEST) Jan Engelhardt <jengelh@inai.de> wrote: > On Tuesday 2021-10-05 20:06, Mathieu Desnoyers wrote: > >> 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. > > >>>> typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); > > > #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 05/10/2021 20.06, Mathieu Desnoyers wrote: > ----- On Oct 5, 2021, at 2:01 PM, Rasmus Villemoes linux@rasmusvillemoes.dk wrote: > >> 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
On Tuesday 2021-10-05 20:40, Steven Rostedt wrote: >> >> >>>> typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); >> >> #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 ;-) “After all, why not?” typeof(p) p1 = (typeof(p) __force)READ_ONCE(p) + BUILD_BUG_ON_EXPR(__builtin_classify_type(p) != 5);
On Tue, 5 Oct 2021 21:06:36 +0200 (CEST) Jan Engelhardt <jengelh@inai.de> wrote: > On Tuesday 2021-10-05 20:40, Steven Rostedt wrote: > >> > >> >>>> typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); > >> > >> #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 ;-) > > “After all, why not?” > > typeof(p) p1 = (typeof(p) __force)READ_ONCE(p) + > BUILD_BUG_ON_EXPR(__builtin_classify_type(p) != 5); 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 <rostedt@goodmis.org> 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
----- On Oct 5, 2021, at 3:40 PM, rostedt rostedt@goodmis.org wrote: > On Tue, 5 Oct 2021 21:06:36 +0200 (CEST) > Jan Engelhardt <jengelh@inai.de> wrote: > >> On Tuesday 2021-10-05 20:40, Steven Rostedt wrote: >> >> >> >> >>>> typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); >> >> >> >> #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 ;-) >> >> “After all, why not?” >> >> typeof(p) p1 = (typeof(p) __force)READ_ONCE(p) + >> BUILD_BUG_ON_EXPR(__builtin_classify_type(p) != 5); > > 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
On Tue, 5 Oct 2021 12:46:43 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Oct 5, 2021 at 12:40 PM Steven Rostedt <rostedt@goodmis.org> 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". 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
On Tue, 5 Oct 2021 15:49:58 -0400 (EDT) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > ----- On Oct 5, 2021, at 3:40 PM, rostedt rostedt@goodmis.org wrote: > > > On Tue, 5 Oct 2021 21:06:36 +0200 (CEST) > > Jan Engelhardt <jengelh@inai.de> wrote: > > > >> On Tuesday 2021-10-05 20:40, Steven Rostedt wrote: > >> >> > >> >> >>>> typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); > >> >> > >> >> #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 ;-) > >> > >> “After all, why not?” > >> > >> typeof(p) p1 = (typeof(p) __force)READ_ONCE(p) + > >> BUILD_BUG_ON_EXPR(__builtin_classify_type(p) != 5); > > > > 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
On Tue, 5 Oct 2021 15:40:29 -0400 Steven Rostedt <rostedt@goodmis.org> 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 <rostedt@goodmis.org> 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
On Tue, 5 Oct 2021 13:45:52 -0700 Linus Torvalds <torvalds@linux-foundation.org> 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
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. Illegal. https://en.cppreference.com/w/c/language/conversion subsection "Pointer conversion" "No other guarantees are offered" >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; >} 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.
On Tue, 5 Oct 2021 23:09:08 +0200 (CEST) Jan Engelhardt <jengelh@inai.de> wrote: > 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. > > Illegal. > https://en.cppreference.com/w/c/language/conversion > subsection "Pointer conversion" > "No other guarantees are offered" 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 *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; > >} > > 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
On Tue, Oct 5, 2021 at 2:09 PM Jan Engelhardt <jengelh@inai.de> wrote: > > Illegal. > https://en.cppreference.com/w/c/language/conversion > subsection "Pointer conversion" > "No other guarantees are offered" 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
On Tuesday 2021-10-05 23:27, Linus Torvalds wrote: >On Tue, Oct 5, 2021 at 2:09 PM Jan Engelhardt <jengelh@inai.de> wrote: >> >> Illegal. >> https://en.cppreference.com/w/c/language/conversion >> subsection "Pointer conversion" >> "No other guarantees are offered" > >Well, we happily end up casting pointers to 'unsigned long' and back, Yes, that wiki had only a succinct summary of pointer-pointer conversions. http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf section 6.3.2.3 (pdfpage 59) has the details, including pointer-integer conversions. >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: 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: Mathieu Desnoyers > Sent: 05 October 2021 16:15 > > ----- 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 ? 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 >> Sent: 05 October 2021 16:15 >> >> ----- 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 ? > > 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)
On Tue, 12 Oct 2021 10:18:03 -0400 (EDT) Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > As Linus pointed out, it might indeed be simpler to just keep declaring the structure in > public headers. I solved this by creating a separate header for the nasty structure, but it's still public for all references. -- Steve
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index 434d12fe2d4f..3835ead65094 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -358,7 +358,7 @@ static inline void rcu_preempt_sleep_check(void) { } #ifdef __CHECKER__ #define rcu_check_sparse(p, space) \ - ((void)(((typeof(*p) space *)p) == p)) + ((void)(((typeof(p) space)p) == p)) #else /* #ifdef __CHECKER__ */ #define rcu_check_sparse(p, space) #endif /* #else #ifdef __CHECKER__ */ @@ -372,43 +372,43 @@ static inline void rcu_preempt_sleep_check(void) { } */ #define unrcu_pointer(p) \ ({ \ - typeof(*p) *_________p1 = (typeof(*p) *__force)(p); \ + typeof(p)_________p1 = (typeof(p) __force)(p); \ rcu_check_sparse(p, __rcu); \ - ((typeof(*p) __force __kernel *)(_________p1)); \ + ((typeof(p) __force __kernel)(_________p1)); \ }) #define __rcu_access_pointer(p, space) \ ({ \ - typeof(*p) *_________p1 = (typeof(*p) *__force)READ_ONCE(p); \ + typeof(p) _________p1 = (typeof(p) __force)READ_ONCE(p); \ rcu_check_sparse(p, space); \ - ((typeof(*p) __force __kernel *)(_________p1)); \ + ((typeof(p) __force __kernel)(_________p1)); \ }) #define __rcu_dereference_check(p, c, space) \ ({ \ /* Dependency order vs. p above. */ \ - typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); \ + typeof(p) ________p1 = (typeof(p) __force)READ_ONCE(p); \ RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \ rcu_check_sparse(p, space); \ - ((typeof(*p) __force __kernel *)(________p1)); \ + ((typeof(p) __force __kernel )(________p1)); \ }) #define __rcu_dereference_protected(p, c, space) \ ({ \ RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_protected() usage"); \ rcu_check_sparse(p, space); \ - ((typeof(*p) __force __kernel *)(p)); \ + ((typeof(p) __force __kernel)(p)); \ }) #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)); \ }) /** * RCU_INITIALIZER() - statically initialize an RCU-protected global variable * @v: The value to statically initialize with. */ -#define RCU_INITIALIZER(v) (typeof(*(v)) __force __rcu *)(v) +#define RCU_INITIALIZER(v) (typeof((v)) __force __rcu)(v) /** * rcu_assign_pointer() - assign to RCU-protected pointer diff --git a/net/ipv4/netfilter/nf_nat_h323.c b/net/ipv4/netfilter/nf_nat_h323.c index 3e2685c120c7..88c902141e33 100644 --- a/net/ipv4/netfilter/nf_nat_h323.c +++ b/net/ipv4/netfilter/nf_nat_h323.c @@ -592,15 +592,15 @@ static int __init init(void) BUG_ON(nat_callforwarding_hook != NULL); BUG_ON(nat_q931_hook != NULL); - RCU_INIT_POINTER(set_h245_addr_hook, set_h245_addr); - RCU_INIT_POINTER(set_h225_addr_hook, set_h225_addr); - RCU_INIT_POINTER(set_sig_addr_hook, set_sig_addr); - RCU_INIT_POINTER(set_ras_addr_hook, set_ras_addr); - RCU_INIT_POINTER(nat_rtp_rtcp_hook, nat_rtp_rtcp); - RCU_INIT_POINTER(nat_t120_hook, nat_t120); - RCU_INIT_POINTER(nat_h245_hook, nat_h245); - RCU_INIT_POINTER(nat_callforwarding_hook, nat_callforwarding); - RCU_INIT_POINTER(nat_q931_hook, nat_q931); + RCU_INIT_POINTER(set_h245_addr_hook, &set_h245_addr); + RCU_INIT_POINTER(set_h225_addr_hook, &set_h225_addr); + RCU_INIT_POINTER(set_sig_addr_hook, &set_sig_addr); + RCU_INIT_POINTER(set_ras_addr_hook, &set_ras_addr); + RCU_INIT_POINTER(nat_rtp_rtcp_hook, &nat_rtp_rtcp); + RCU_INIT_POINTER(nat_t120_hook, &nat_t120); + RCU_INIT_POINTER(nat_h245_hook, &nat_h245); + RCU_INIT_POINTER(nat_callforwarding_hook, &nat_callforwarding); + RCU_INIT_POINTER(nat_q931_hook, &nat_q931); nf_ct_helper_expectfn_register(&q931_nat); nf_ct_helper_expectfn_register(&callforwarding_nat); return 0; diff --git a/net/ipv4/netfilter/nf_nat_pptp.c b/net/ipv4/netfilter/nf_nat_pptp.c index 3f248a19faa3..6a22ffa5e4c7 100644 --- a/net/ipv4/netfilter/nf_nat_pptp.c +++ b/net/ipv4/netfilter/nf_nat_pptp.c @@ -298,16 +298,16 @@ pptp_inbound_pkt(struct sk_buff *skb, static int __init nf_nat_helper_pptp_init(void) { BUG_ON(nf_nat_pptp_hook_outbound != NULL); - RCU_INIT_POINTER(nf_nat_pptp_hook_outbound, pptp_outbound_pkt); + RCU_INIT_POINTER(nf_nat_pptp_hook_outbound, &pptp_outbound_pkt); BUG_ON(nf_nat_pptp_hook_inbound != NULL); - RCU_INIT_POINTER(nf_nat_pptp_hook_inbound, pptp_inbound_pkt); + RCU_INIT_POINTER(nf_nat_pptp_hook_inbound, &pptp_inbound_pkt); BUG_ON(nf_nat_pptp_hook_exp_gre != NULL); - RCU_INIT_POINTER(nf_nat_pptp_hook_exp_gre, pptp_exp_gre); + RCU_INIT_POINTER(nf_nat_pptp_hook_exp_gre, &pptp_exp_gre); BUG_ON(nf_nat_pptp_hook_expectfn != NULL); - RCU_INIT_POINTER(nf_nat_pptp_hook_expectfn, pptp_nat_expected); + RCU_INIT_POINTER(nf_nat_pptp_hook_expectfn, &pptp_nat_expected); return 0; } diff --git a/net/ipv4/netfilter/nf_nat_snmp_basic_main.c b/net/ipv4/netfilter/nf_nat_snmp_basic_main.c index 717b726504fe..2ac36fa86e73 100644 --- a/net/ipv4/netfilter/nf_nat_snmp_basic_main.c +++ b/net/ipv4/netfilter/nf_nat_snmp_basic_main.c @@ -215,7 +215,7 @@ static struct nf_conntrack_helper snmp_trap_helper __read_mostly = { static int __init nf_nat_snmp_basic_init(void) { BUG_ON(nf_nat_snmp_hook != NULL); - RCU_INIT_POINTER(nf_nat_snmp_hook, help); + RCU_INIT_POINTER(nf_nat_snmp_hook, &help); return nf_conntrack_helper_register(&snmp_trap_helper); } diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index 94e18fb9690d..e243e5695b63 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -2733,7 +2733,7 @@ static struct nf_ct_hook nf_conntrack_hook = { void nf_conntrack_init_end(void) { /* For use by REJECT target */ - RCU_INIT_POINTER(ip_ct_attach, nf_conntrack_attach); + RCU_INIT_POINTER(ip_ct_attach, &nf_conntrack_attach); RCU_INIT_POINTER(nf_ct_hook, &nf_conntrack_hook); } diff --git a/net/netfilter/nf_nat_amanda.c b/net/netfilter/nf_nat_amanda.c index 3bc7e0854efe..fe077c53e06a 100644 --- a/net/netfilter/nf_nat_amanda.c +++ b/net/netfilter/nf_nat_amanda.c @@ -84,7 +84,7 @@ static int __init nf_nat_amanda_init(void) { BUG_ON(nf_nat_amanda_hook != NULL); nf_nat_helper_register(&nat_helper_amanda); - RCU_INIT_POINTER(nf_nat_amanda_hook, help); + RCU_INIT_POINTER(nf_nat_amanda_hook, &help); return 0; } diff --git a/net/netfilter/nf_nat_ftp.c b/net/netfilter/nf_nat_ftp.c index aace6768a64e..d657606b3364 100644 --- a/net/netfilter/nf_nat_ftp.c +++ b/net/netfilter/nf_nat_ftp.c @@ -135,7 +135,7 @@ static int __init nf_nat_ftp_init(void) { BUG_ON(nf_nat_ftp_hook != NULL); nf_nat_helper_register(&nat_helper_ftp); - RCU_INIT_POINTER(nf_nat_ftp_hook, nf_nat_ftp); + RCU_INIT_POINTER(nf_nat_ftp_hook, &nf_nat_ftp); return 0; } diff --git a/net/netfilter/nf_nat_irc.c b/net/netfilter/nf_nat_irc.c index c691ab8d234c..4742ed478989 100644 --- a/net/netfilter/nf_nat_irc.c +++ b/net/netfilter/nf_nat_irc.c @@ -106,7 +106,7 @@ static int __init nf_nat_irc_init(void) { BUG_ON(nf_nat_irc_hook != NULL); nf_nat_helper_register(&nat_helper_irc); - RCU_INIT_POINTER(nf_nat_irc_hook, help); + RCU_INIT_POINTER(nf_nat_irc_hook, &help); return 0; } diff --git a/net/netfilter/nf_nat_tftp.c b/net/netfilter/nf_nat_tftp.c index 1a591132d6eb..591f4e559c55 100644 --- a/net/netfilter/nf_nat_tftp.c +++ b/net/netfilter/nf_nat_tftp.c @@ -48,7 +48,7 @@ static int __init nf_nat_tftp_init(void) { BUG_ON(nf_nat_tftp_hook != NULL); nf_nat_helper_register(&nat_helper_tftp); - RCU_INIT_POINTER(nf_nat_tftp_hook, help); + RCU_INIT_POINTER(nf_nat_tftp_hook, &help); return 0; } diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c index c57673d499be..0097e0a65ca2 100644 --- a/net/netfilter/nfnetlink_cttimeout.c +++ b/net/netfilter/nfnetlink_cttimeout.c @@ -619,8 +619,8 @@ static int __init cttimeout_init(void) "nfnetlink.\n"); goto err_out; } - RCU_INIT_POINTER(nf_ct_timeout_find_get_hook, ctnl_timeout_find_get); - RCU_INIT_POINTER(nf_ct_timeout_put_hook, ctnl_timeout_put); + RCU_INIT_POINTER(nf_ct_timeout_find_get_hook, &ctnl_timeout_find_get); + RCU_INIT_POINTER(nf_ct_timeout_put_hook, &ctnl_timeout_put); return 0; err_out: