* [srcu] Can we suppress sparse warning?
@ 2016-11-09 11:36 Tetsuo Handa
2016-11-09 18:58 ` Paul E. McKenney
0 siblings, 1 reply; 5+ messages in thread
From: Tetsuo Handa @ 2016-11-09 11:36 UTC (permalink / raw)
To: paulmck, josh; +Cc: rostedt, mathieu.desnoyers, jiangshanlai, linux-kernel
Hello.
When I build
---------- test/test.c ----------
#include <linux/module.h>
static int __init test_init(void)
{
DEFINE_SRCU(srcu);
int idx = srcu_read_lock(&srcu);
void *ptr = srcu_dereference(ptr, &srcu);
srcu_read_unlock(&srcu, idx);
return -EINVAL;
}
module_init(test_init);
MODULE_LICENSE("GPL");
---------- test/test.c ----------
with C=1 option, I get
test/test.c:7:21: error: incompatible types in comparison expression (different address spaces)
warning from
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -605,7 +605,7 @@ static inline void rcu_preempt_sleep_check(void)
/* Dependency order vs. p above. */ \
typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
- rcu_dereference_sparse(p, space); \
+ rcu_dereference_sparse(p, space); /** this line **/ \
((typeof(*p) __force __kernel *)(________p1)); \
})
#define __rcu_dereference_protected(p, c, space) \
.
I want to use srcu_dereference() at
https://lists.01.org/pipermail/kbuild-all/2016-October/026587.html .
Can we have srcu version of 995f1405610bd844 ("rcu: Suppress sparse
warnings for rcu_dereference_raw()") ?
Regards.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [srcu] Can we suppress sparse warning?
2016-11-09 11:36 [srcu] Can we suppress sparse warning? Tetsuo Handa
@ 2016-11-09 18:58 ` Paul E. McKenney
2016-11-12 14:26 ` Tetsuo Handa
0 siblings, 1 reply; 5+ messages in thread
From: Paul E. McKenney @ 2016-11-09 18:58 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, linux-kernel
On Wed, Nov 09, 2016 at 08:36:31PM +0900, Tetsuo Handa wrote:
> Hello.
>
> When I build
>
> ---------- test/test.c ----------
> #include <linux/module.h>
>
> static int __init test_init(void)
> {
> DEFINE_SRCU(srcu);
> int idx = srcu_read_lock(&srcu);
> void *ptr = srcu_dereference(ptr, &srcu);
I am really confused by the above line of code...
> srcu_read_unlock(&srcu, idx);
> return -EINVAL;
> }
>
> module_init(test_init);
> MODULE_LICENSE("GPL");
> ---------- test/test.c ----------
>
> with C=1 option, I get
>
> test/test.c:7:21: error: incompatible types in comparison expression (different address spaces)
>
> warning from
>
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -605,7 +605,7 @@ static inline void rcu_preempt_sleep_check(void)
> /* Dependency order vs. p above. */ \
> typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
> RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
> - rcu_dereference_sparse(p, space); \
> + rcu_dereference_sparse(p, space); /** this line **/ \
> ((typeof(*p) __force __kernel *)(________p1)); \
> })
> #define __rcu_dereference_protected(p, c, space) \
>
> .
>
> I want to use srcu_dereference() at
> https://lists.01.org/pipermail/kbuild-all/2016-October/026587.html .
> Can we have srcu version of 995f1405610bd844 ("rcu: Suppress sparse
> warnings for rcu_dereference_raw()") ?
Notwithstanding my confusion about what your self-referential
srcu_dereference() is intended to achieve, what happens if you change the
"void *ptr = srcu_dereference(ptr, &srcu);" to add __rcu?
void __rcu *ptr = srcu_dereference(ptr, &srcu);
Or, more to the point:
void __rcu *gptr;
void *lptr = srcu_dereference(ptr, &srcu);
Thanx, Paul
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [srcu] Can we suppress sparse warning?
2016-11-09 18:58 ` Paul E. McKenney
@ 2016-11-12 14:26 ` Tetsuo Handa
2016-11-12 19:17 ` Paul E. McKenney
0 siblings, 1 reply; 5+ messages in thread
From: Tetsuo Handa @ 2016-11-12 14:26 UTC (permalink / raw)
To: paulmck; +Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, linux-kernel
Paul E. McKenney wrote:
> Notwithstanding my confusion about what your self-referential
> srcu_dereference() is intended to achieve, what happens if you change the
> "void *ptr = srcu_dereference(ptr, &srcu);" to add __rcu?
Sorry, I wrote this code for only showing warning message.
This self-referential has no intention.
Well, you can reproduce this warning with current linux.git by running
make C=1 security/tomoyo/ with CONFIG_SECURITY_TOMOYO=y .
security/tomoyo/common.c:896:9: error: incompatible types in comparison expression (different address spaces)
security/tomoyo/common.c:896:9: error: incompatible types in comparison expression (different address spaces)
---------- security/tomoyo/common.c ----------
896: list_for_each_cookie(head->r.acl, &tomoyo_kernel_namespace.
897: policy_list[TOMOYO_ID_MANAGER]) {
---------- security/tomoyo/common.h ----------
1320: /**
1321: * list_for_each_cookie - iterate over a list with cookie.
1322: * @pos: the &struct list_head to use as a loop cursor.
1323: * @head: the head for your list.
1324: */
1325: #define list_for_each_cookie(pos, head) \
1326: if (!pos) \
1327: pos = srcu_dereference((head)->next, &tomoyo_ss); \
1328: for ( ; pos != (head); pos = srcu_dereference(pos->next, &tomoyo_ss))
Both head->r.acl and &tomoyo_kernel_namespace.policy_list[TOMOYO_ID_MANAGER]->next
refer normal kernel address space. Thus, I think that this warning is a false positive.
This warning goes away if I disable rcu_dereference_sparse() call in
__rcu_dereference_check() from srcu_dereference_check() from srcu_dereference().
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -605,7 +605,7 @@ static inline void rcu_preempt_sleep_check(void)
/* Dependency order vs. p above. */ \
typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
- rcu_dereference_sparse(p, space); \
+ /*rcu_dereference_sparse(p, space); */ \
((typeof(*p) __force __kernel *)(________p1)); \
})
#define __rcu_dereference_protected(p, c, space) \
Regards.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [srcu] Can we suppress sparse warning?
2016-11-12 14:26 ` Tetsuo Handa
@ 2016-11-12 19:17 ` Paul E. McKenney
2016-11-13 12:55 ` Tetsuo Handa
0 siblings, 1 reply; 5+ messages in thread
From: Paul E. McKenney @ 2016-11-12 19:17 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, linux-kernel
On Sat, Nov 12, 2016 at 11:26:15PM +0900, Tetsuo Handa wrote:
> Paul E. McKenney wrote:
> > Notwithstanding my confusion about what your self-referential
> > srcu_dereference() is intended to achieve, what happens if you change the
> > "void *ptr = srcu_dereference(ptr, &srcu);" to add __rcu?
>
> Sorry, I wrote this code for only showing warning message.
> This self-referential has no intention.
>
> Well, you can reproduce this warning with current linux.git by running
> make C=1 security/tomoyo/ with CONFIG_SECURITY_TOMOYO=y .
>
> security/tomoyo/common.c:896:9: error: incompatible types in comparison expression (different address spaces)
> security/tomoyo/common.c:896:9: error: incompatible types in comparison expression (different address spaces)
>
> ---------- security/tomoyo/common.c ----------
> 896: list_for_each_cookie(head->r.acl, &tomoyo_kernel_namespace.
> 897: policy_list[TOMOYO_ID_MANAGER]) {
>
> ---------- security/tomoyo/common.h ----------
> 1320: /**
> 1321: * list_for_each_cookie - iterate over a list with cookie.
> 1322: * @pos: the &struct list_head to use as a loop cursor.
> 1323: * @head: the head for your list.
> 1324: */
> 1325: #define list_for_each_cookie(pos, head) \
> 1326: if (!pos) \
> 1327: pos = srcu_dereference((head)->next, &tomoyo_ss); \
> 1328: for ( ; pos != (head); pos = srcu_dereference(pos->next, &tomoyo_ss))
This definition will be fun if used in an "if" statement, but I will leave
that in your capable hands.
>
> Both head->r.acl and &tomoyo_kernel_namespace.policy_list[TOMOYO_ID_MANAGER]->next
> refer normal kernel address space. Thus, I think that this warning is a false positive.
>
> This warning goes away if I disable rcu_dereference_sparse() call in
> __rcu_dereference_check() from srcu_dereference_check() from srcu_dereference().
>
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -605,7 +605,7 @@ static inline void rcu_preempt_sleep_check(void)
> /* Dependency order vs. p above. */ \
> typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
> RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
> - rcu_dereference_sparse(p, space); \
> + /*rcu_dereference_sparse(p, space); */ \
> ((typeof(*p) __force __kernel *)(________p1)); \
> })
> #define __rcu_dereference_protected(p, c, space) \
OK.
One approach would be for you to replace your:
pos = srcu_dereference((head)->next, &tomoyo_ss);
with:
pos = ecu_dereference_raw((head)->next);
This will suppress the sparse complaint, but would also suppress the
lockdep complaint about using list_for_each_cookie() unprotected by an
SRCU read-side critical section. But this can be handled by placing an
appropriate RCU_LOCKDEP_WARN() in list_for_each_cookie().
Does that work for you?
Thanx, Paul
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [srcu] Can we suppress sparse warning?
2016-11-12 19:17 ` Paul E. McKenney
@ 2016-11-13 12:55 ` Tetsuo Handa
0 siblings, 0 replies; 5+ messages in thread
From: Tetsuo Handa @ 2016-11-13 12:55 UTC (permalink / raw)
To: paulmck; +Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, linux-kernel
Paul E. McKenney wrote:
> On Sat, Nov 12, 2016 at 11:26:15PM +0900, Tetsuo Handa wrote:
> > Both head->r.acl and &tomoyo_kernel_namespace.policy_list[TOMOYO_ID_MANAGER]->next
> > refer normal kernel address space. Thus, I think that this warning is a false positive.
> >
> > This warning goes away if I disable rcu_dereference_sparse() call in
> > __rcu_dereference_check() from srcu_dereference_check() from srcu_dereference().
> >
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -605,7 +605,7 @@ static inline void rcu_preempt_sleep_check(void)
> > /* Dependency order vs. p above. */ \
> > typeof(*p) *________p1 = (typeof(*p) *__force)lockless_dereference(p); \
> > RCU_LOCKDEP_WARN(!(c), "suspicious rcu_dereference_check() usage"); \
> > - rcu_dereference_sparse(p, space); \
> > + /*rcu_dereference_sparse(p, space); */ \
> > ((typeof(*p) __force __kernel *)(________p1)); \
> > })
> > #define __rcu_dereference_protected(p, c, space) \
>
> OK.
>
> One approach would be for you to replace your:
>
> pos = srcu_dereference((head)->next, &tomoyo_ss);
>
> with:
>
> pos = ecu_dereference_raw((head)->next);
>
> This will suppress the sparse complaint, but would also suppress the
> lockdep complaint about using list_for_each_cookie() unprotected by an
> SRCU read-side critical section. But this can be handled by placing an
> appropriate RCU_LOCKDEP_WARN() in list_for_each_cookie().
>
> Does that work for you?
Yes, I can use below approach. Thank you.
--- a/security/tomoyo/common.h
+++ b/security/tomoyo/common.h
@@ -1317,14 +1317,20 @@ static inline int tomoyo_round2(size_t size)
#endif
+static inline void tomoyo_read_lock_check(void)
+{
+ RCU_LOCKDEP_WARN(!srcu_read_lock_held(&tomoyo_ss),
+ "tomoyo_ss lock is not held.");
+}
+
/**
* list_for_each_cookie - iterate over a list with cookie.
* @pos: the &struct list_head to use as a loop cursor.
* @head: the head for your list.
*/
#define list_for_each_cookie(pos, head) \
- if (!pos) \
- pos = srcu_dereference((head)->next, &tomoyo_ss); \
- for ( ; pos != (head); pos = srcu_dereference(pos->next, &tomoyo_ss))
+ for (pos = pos ? pos : rcu_dereference_raw((head)->next); \
+ tomoyo_read_lock_check(), pos != (head); \
+ pos = rcu_dereference_raw(pos->next))
#endif /* !defined(_SECURITY_TOMOYO_COMMON_H) */
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-13 12:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-09 11:36 [srcu] Can we suppress sparse warning? Tetsuo Handa
2016-11-09 18:58 ` Paul E. McKenney
2016-11-12 14:26 ` Tetsuo Handa
2016-11-12 19:17 ` Paul E. McKenney
2016-11-13 12:55 ` Tetsuo Handa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).