[srcu] Can we suppress sparse warning?

[srcu] Can we suppress sparse warning?

[Tetsuo Handa]

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.

Re: [srcu] Can we suppress sparse warning?

[Paul E. McKenney]

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

Re: [srcu] Can we suppress sparse warning?

[Tetsuo Handa]

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.

Re: [srcu] Can we suppress sparse warning?

[Paul E. McKenney]

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

Re: [srcu] Can we suppress sparse warning?

[Tetsuo Handa]

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