LinuxLists.cc - [RFC][PATCH] rcu: Use typeof(p) instead of typeof(*p) *

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

----- On Oct 5, 2021, at 9:47 AM, rostedt [email protected] 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

2021-10-05 16:01:16

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 11:15:12 -0400 (EDT)
Mathieu Desnoyers <[email protected]> wrote:

> ----- On Oct 5, 2021, at 9:47 AM, rostedt [email protected] 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

2021-10-05 16:17:40

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

----- On Oct 5, 2021, at 11:58 AM, rostedt [email protected] wrote:

> On Tue, 5 Oct 2021 11:15:12 -0400 (EDT)
> Mathieu Desnoyers <[email protected]> wrote:
>
>> ----- On Oct 5, 2021, at 9:47 AM, rostedt [email protected] 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

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

2021-10-05 16:22:49

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, Oct 5, 2021 at 6:47 AM Steven Rostedt <[email protected]> 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

2021-10-05 16:31:55

by Paul E. McKenney

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, Oct 05, 2021 at 12:15:04PM -0400, Mathieu Desnoyers wrote:
> ----- On Oct 5, 2021, at 11:58 AM, rostedt [email protected] wrote:
>
> > On Tue, 5 Oct 2021 11:15:12 -0400 (EDT)
> > Mathieu Desnoyers <[email protected]> wrote:
> >
> >> ----- On Oct 5, 2021, at 9:47 AM, rostedt [email protected] 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

2021-10-05 16:40:17

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 09:18:04 -0700
Linus Torvalds <[email protected]> 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

2021-10-05 16:42:48

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 12:15:04 -0400 (EDT)
Mathieu Desnoyers <[email protected]> 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

2021-10-05 16:43:24

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 09:18:04 -0700
Linus Torvalds <[email protected]> wrote:

> On Tue, Oct 5, 2021 at 6:47 AM Steven Rostedt <[email protected]> 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

2021-10-05 16:50:09

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, Oct 5, 2021 at 9:37 AM Steven Rostedt <[email protected]> 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

2021-10-05 18:04:22

by Rasmus Villemoes

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

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

2021-10-05 18:08:54

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

----- On Oct 5, 2021, at 2:01 PM, Rasmus Villemoes [email protected] 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

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

2021-10-05 18:30:10

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

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*

2021-10-05 18:42:12

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 20:28:54 +0200 (CEST)
Jan Engelhardt <[email protected]> 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

2021-10-05 18:49:48

by Rasmus Villemoes

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On 05/10/2021 20.06, Mathieu Desnoyers wrote:
> ----- On Oct 5, 2021, at 2:01 PM, Rasmus Villemoes [email protected] 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

2021-10-05 19:07:54

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

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);

2021-10-05 19:41:36

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 21:06:36 +0200 (CEST)
Jan Engelhardt <[email protected]> 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

2021-10-05 19:50:25

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, Oct 5, 2021 at 12:40 PM Steven Rostedt <[email protected]> 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

2021-10-05 19:53:32

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

----- On Oct 5, 2021, at 3:40 PM, rostedt [email protected] wrote:

> On Tue, 5 Oct 2021 21:06:36 +0200 (CEST)
> Jan Engelhardt <[email protected]> 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

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

2021-10-05 20:07:02

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 12:46:43 -0700
Linus Torvalds <[email protected]> wrote:

> On Tue, Oct 5, 2021 at 12:40 PM Steven Rostedt <[email protected]> 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

2021-10-05 20:09:12

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 15:49:58 -0400 (EDT)
Mathieu Desnoyers <[email protected]> wrote:

> ----- On Oct 5, 2021, at 3:40 PM, rostedt [email protected] wrote:
>
> > On Tue, 5 Oct 2021 21:06:36 +0200 (CEST)
> > Jan Engelhardt <[email protected]> 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

2021-10-05 20:40:18

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 15:40:29 -0400
Steven Rostedt <[email protected]> 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

2021-10-05 20:48:04

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, Oct 5, 2021 at 1:38 PM Steven Rostedt <[email protected]> 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

2021-10-05 21:09:18

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 13:45:52 -0700
Linus Torvalds <[email protected]> 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

2021-10-05 21:12:43

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

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.

2021-10-05 21:25:39

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, 5 Oct 2021 23:09:08 +0200 (CEST)
Jan Engelhardt <[email protected]> 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

2021-10-05 21:36:16

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tue, Oct 5, 2021 at 2:09 PM Jan Engelhardt <[email protected]> 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

2021-10-05 22:30:49

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

On Tuesday 2021-10-05 23:27, Linus Torvalds wrote:
>On Tue, Oct 5, 2021 at 2:09 PM Jan Engelhardt <[email protected]> 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". ;-)

2021-10-11 12:16:03

by David Laight

[permalink] [raw]

Subject: RE: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

From: Mathieu Desnoyers
> Sent: 05 October 2021 16:15
>
> ----- On Oct 5, 2021, at 9:47 AM, rostedt [email protected] 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)

2021-10-11 12:40:09

by David Laight

[permalink] [raw]

Subject: RE: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

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)

2021-10-12 14:20:13

[permalink] [raw]

Subject: Re: [RFC][PATCH] rcu: Use typeof(p) instead of typeof(p)

----- On Oct 11, 2021, at 4:39 AM, David Laight [email protected] wrote:

> From: Mathieu Desnoyers
>> Sent: 05 October 2021 16:15
>>
>> ----- On Oct 5, 2021, at 9:47 AM, rostedt [email protected] 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)

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

2021-10-12 15:39:46