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

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

[Steven Rostedt]

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

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:
-- 
2.31.1

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

[Mathieu Desnoyers]

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

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

[Steven Rostedt]

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

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

[Mathieu Desnoyers]

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


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

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

[Linus Torvalds]

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

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

[Paul E. McKenney]

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

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

[Steven Rostedt]

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

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

[Steven Rostedt]

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

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

[Steven Rostedt]

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

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

[Linus Torvalds]

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

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

[Rasmus Villemoes]

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

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

[Mathieu Desnoyers]

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

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

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

[Jan Engelhardt]

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*

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

[Steven Rostedt]

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

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

[Rasmus Villemoes]

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

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

[Jan Engelhardt]

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

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

[Steven Rostedt]

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

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

[Linus Torvalds]

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

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

[Mathieu Desnoyers]

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

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

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

[Steven Rostedt]

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

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

[Steven Rostedt]

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

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

[Steven Rostedt]

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

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

[Linus Torvalds]

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

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

[Steven Rostedt]

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

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

[Jan Engelhardt]

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.

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

[Steven Rostedt]

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

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

[Linus Torvalds]

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

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

[Jan Engelhardt]

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

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

[David Laight]

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)

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

[David Laight]

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)

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

[Mathieu Desnoyers]

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

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

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

[Steven Rostedt]

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