Skip to contentSkip to conversation

drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists

PATCH Superseded Amol Grover 3 participants 9 messages Jan 10, 2020 – Jan 16, 2020 IPMI

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Amol Grover

author
#0 lore raw

PATCH v2 1/1 drivers: char: ipmi: ipmi_msghandler: Pass lockdep expression to RCU lists

intf->cmd_rcvrs is traversed with list_for_each_entry_rcu outside an RCU read-side critical section but under the protection of intf->cmd_rcvrs_mutex.

ipmi_interfaces is traversed using list_for_each_entry_rcu outside an RCU read-side critical section but under the protection of ipmi_interfaces_mutex.

Hence, add the corresponding lockdep expression to the list traversal primitive to silence false-positive lockdep warnings, and harden RCU lists.

Add macro for the corresponding lockdep expression to make the code clean and concise.

Signed-off-by Amol Grover <[email protected]>

---
v2:
- Fix sparse error
CHECK: Alignment should match open parenthesis

 drivers/char/ipmi/ipmi_msghandler.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/char/ipmi/ipmi_msghandler.c +12 -4
index cad9563f8f48..1cbeacb1e8b9 100644
@@ -35,6 +35,8 @@
 #include <linux/nospec.h>
 
 #define IPMI_DRIVER_VERSION "39.2"
+#define cmd_rcvrs_mutex_held() \
+	lockdep_is_held(&intf->cmd_rcvrs_mutex)
 
 static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void);
 static int ipmi_init_msghandler(void);

@@ -618,6 +620,8 @@ static DEFINE_MUTEX(ipmidriver_mutex);
 
 static LIST_HEAD(ipmi_interfaces);
 static DEFINE_MUTEX(ipmi_interfaces_mutex);
+#define ipmi_interfaces_mutex_held() \
+	lockdep_is_held(&ipmi_interfaces_mutex)
 static struct srcu_struct ipmi_interfaces_srcu;
 
 /*

@@ -1321,7 +1325,8 @@ static void _ipmi_destroy_user(struct ipmi_user *user)
 	 * synchronize_srcu()) then free everything in that list.
 	 */
 	mutex_lock(&intf->cmd_rcvrs_mutex);
-	list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link) {
+	list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
+				cmd_rcvrs_mutex_held()) {
 		if (rcvr->user == user) {
 			list_del_rcu(&rcvr->link);
 			rcvr->next = rcvrs;

@@ -1599,7 +1604,8 @@ static struct cmd_rcvr *find_cmd_rcvr(struct ipmi_smi *intf,
 {
 	struct cmd_rcvr *rcvr;
 
-	list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link) {
+	list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
+				rcu_read_lock_held() || cmd_rcvrs_mutex_held()) {
 		if ((rcvr->netfn == netfn) && (rcvr->cmd == cmd)
 					&& (rcvr->chans & (1 << chan)))
 			return rcvr;

@@ -1614,7 +1620,8 @@ static int is_cmd_rcvr_exclusive(struct ipmi_smi *intf,
 {
 	struct cmd_rcvr *rcvr;
 
-	list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link) {
+	list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
+				cmd_rcvrs_mutex_held()) {
 		if ((rcvr->netfn == netfn) && (rcvr->cmd == cmd)
 					&& (rcvr->chans & chans))
 			return 0;

@@ -3450,7 +3457,8 @@ int ipmi_add_smi(struct module         *owner,
 	/* Look for a hole in the numbers. */
 	i = 0;
 	link = &ipmi_interfaces;
-	list_for_each_entry_rcu(tintf, &ipmi_interfaces, link) {
+	list_for_each_entry_rcu(tintf, &ipmi_interfaces, link,
+				ipmi_interfaces_mutex_held()) {
 		if (tintf->intf_num != i) {
 			link = &tintf->link;
 			break;
Signature · 1 line
2.24.1
build bot
#1 lore raw
Thank you for the patch! Perhaps something to improve:

Hi Amol,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf
config: x86_64-randconfig-a003-20200109 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from include/linux/export.h:43:0,
from include/linux/linkage.h:7,
from include/linux/kernel.h:8,
from include/linux/list.h:9,
from include/linux/module.h:12,
from drivers/char/ipmi/ipmi_msghandler.c:17:
drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr':
include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^
>> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'
      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
>> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^
>> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'
      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
>> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^
>> include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'
      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
>> include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^

vim +/if +263 include/linux/rcupdate.h

632ee200130899 Paul E. McKenney 2010-02-22  254  
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  255  /**
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  256   * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  257   * @c: condition to check
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  258   * @s: informative message
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  259   */
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  260  #define RCU_LOCKDEP_WARN(c, s)						\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  261  	do {								\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  262  		static bool __section(.data.unlikely) __warned;		\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263  		if (debug_lockdep_rcu_enabled() && !__warned && (c)) {	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  264  			__warned = true;				\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  265  			lockdep_rcu_suspicious(__FILE__, __LINE__, s);	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  266  		}							\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  267  	} while (0)

f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268

:::::: The code at line 263 was first introduced by commit
:::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN()

:::::: TO: Paul E. McKenney <[email protected]> :::::: CC: Paul E. McKenney <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

author
#2 lore raw
Quoted 23 lines · kbuild test robot

Hi Amol,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf
config: x86_64-randconfig-a003-20200109 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 10 lines
   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/char/ipmi/ipmi_msghandler.c:17:
   drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr':
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^

As mentioned above, RCU_LOCKDEP_WARN macro is called from __list_check_rcu with 2 parameters

1. !cond && !rcu_read_lock_any_held()
2. The message to display incase there is a lockdep warning.

However, if I pass the lockdep checking condition as:

list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held())

this trickles down to __list_check_rcu and then finally to
RCU_LOCKDEP_WARN as (here cond is lockdep_is_held(&some_lock) || rcu_read_lock_held()):

RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held())

which according to operator precedence (I hopefully got them right) would always evaluate to true if we are in an RCU read-side critical section (without a lock), and hence, result in a false-positive lockdep warning.

This could be easily solved by putting cond inside brackets as it is correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu macro. Is that so, or did I miss something?

Secondly, since there is already a condition that checks for RCU read-side critical section, the extra rcu_read_lock_held() we supply is sort of redundant and can be skipped right?

Thanks
Amol

Quoted 82 lines
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^

vim +/if +263 include/linux/rcupdate.h

632ee200130899 Paul E. McKenney 2010-02-22  254  
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  255  /**
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  256   * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  257   * @c: condition to check
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  258   * @s: informative message
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  259   */
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  260  #define RCU_LOCKDEP_WARN(c, s)						\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  261  	do {								\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  262  		static bool __section(.data.unlikely) __warned;		\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263  		if (debug_lockdep_rcu_enabled() && !__warned && (c)) {	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  264  			__warned = true;				\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  265  			lockdep_rcu_suspicious(__FILE__, __LINE__, s);	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  266  		}							\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  267  	} while (0)

f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268

:::::: The code at line 263 was first introduced by commit
:::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN()

:::::: TO: Paul E. McKenney <[email protected]> :::::: CC: Paul E. McKenney <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation

#3 lore raw
Quoted 38 lines · Amol Grover
Quoted 23 lines · kbuild test robot

Hi Amol,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf
config: x86_64-randconfig-a003-20200109 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 10 lines
   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/char/ipmi/ipmi_msghandler.c:17:
   drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr':
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^

As mentioned above, RCU_LOCKDEP_WARN macro is called from __list_check_rcu with 2 parameters

Quoted 7 lines

1. !cond && !rcu_read_lock_any_held()
2. The message to display incase there is a lockdep warning.

However, if I pass the lockdep checking condition as:

list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held())

Right, given the _rcu() suffix on the command, the rcu_read_lock_held() is implied.

Quoted 9 lines

this trickles down to __list_check_rcu and then finally to
RCU_LOCKDEP_WARN as (here cond is lockdep_is_held(&some_lock) || rcu_read_lock_held()):

RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held())

which according to operator precedence (I hopefully got them right) would always evaluate to true if we are in an RCU read-side critical section (without a lock), and hence, result in a false-positive lockdep warning.

It looks that way to me. But why not actually try it out? After all, only the running system knows for sure. And there might be some trick that we are both missing.

Quoted 3 lines

This could be easily solved by putting cond inside brackets as it is correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu macro. Is that so, or did I miss something?

Again, that looks correct to me, but please check.

Quoted 3 lines

Secondly, since there is already a condition that checks for RCU read-side critical section, the extra rcu_read_lock_held() we supply is sort of redundant and can be skipped right?

Yes, the general rule is that if the primitives ends with _rcu(), any lockdep condition will be in addition to rcu_read_lock_any_held(). So you should not need to pass RCU read-side lockdep expressions to primitives whose names end in _rcu..

Thanx, Paul

Quoted 87 lines

Thanks
Amol

Quoted 82 lines
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^

vim +/if +263 include/linux/rcupdate.h

632ee200130899 Paul E. McKenney 2010-02-22  254  
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  255  /**
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  256   * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  257   * @c: condition to check
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  258   * @s: informative message
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  259   */
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  260  #define RCU_LOCKDEP_WARN(c, s)						\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  261  	do {								\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  262  		static bool __section(.data.unlikely) __warned;		\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263  		if (debug_lockdep_rcu_enabled() && !__warned && (c)) {	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  264  			__warned = true;				\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  265  			lockdep_rcu_suspicious(__FILE__, __LINE__, s);	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  266  		}							\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  267  	} while (0)

f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268

:::::: The code at line 263 was first introduced by commit
:::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN()

:::::: TO: Paul E. McKenney <[email protected]> :::::: CC: Paul E. McKenney <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

author
#4 lore raw
Quoted 55 lines · Paul E. McKenney
Quoted 38 lines · Amol Grover
Quoted 23 lines · kbuild test robot

Hi Amol,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf
config: x86_64-randconfig-a003-20200109 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 10 lines
   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/char/ipmi/ipmi_msghandler.c:17:
   drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr':
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^

As mentioned above, RCU_LOCKDEP_WARN macro is called from __list_check_rcu with 2 parameters

Quoted 7 lines

1. !cond && !rcu_read_lock_any_held()
2. The message to display incase there is a lockdep warning.

However, if I pass the lockdep checking condition as:

list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held())

Right, given the _rcu() suffix on the command, the rcu_read_lock_held() is implied.

Quoted 5 lines

this trickles down to __list_check_rcu and then finally to
RCU_LOCKDEP_WARN as (here cond is lockdep_is_held(&some_lock) || rcu_read_lock_held()):

RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held())

Quoted 9 lines
Quoted 4 lines

which according to operator precedence (I hopefully got them right) would always evaluate to true if we are in an RCU read-side critical section (without a lock), and hence, result in a false-positive lockdep warning.

It looks that way to me. But why not actually try it out? After all, only the running system knows for sure. And there might be some trick that we are both missing.

I just tested this, here are the results:

Case 1: Usinglockdep_is_held() || rcu_read_lock_held()

lock RCU RSCS Splat? Actual
Y Y N N
Y N N N
N Y Y N <=
N N Y Y

Similar for
Case 2: Using rcu_read_lock_held() || lockdep_is_held()

Case 3: Consider 2 locks (outside rcu_read_lock()) lockdep_is_held(lock1) || lockdep_is_held(lock2)

lock1 lock2 Splat? Actual
Y Y N* N
Y N N N
N Y Y N <=
N N Y Y

This too proves the hypothesis (I'd like to call that).

*However, this shows an interesting result. When both lock1 and lock2 are held, according to the hypothesis, a splat should've occured, since the check condition (albeit faulty atm) would be:

!lockdep_is_held(lock1) || lockdep_is_held(lock2) && !rcu_read_lock_any_held()

=> `!T || T && !F`
=> `F || T && T`
=> `F || T`

=> T
However, there was no splat. Which led me to investigate further and I found out:
1. rcu_read_lock_any_held() always returns 1 even if it is outside RCU read-side CS.
2. rcu_read_lock_held() seems OK, returns 1 when inside and 0 when outside

The kernel is compiled with
PROVE_RCU=y
PROVE_RCU_LIST=y

Any thoughts on this? Is this intended? And should I send-in the patch for the first problem?

Thanks
Amol

Quoted 104 lines
Quoted 3 lines

This could be easily solved by putting cond inside brackets as it is correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu macro. Is that so, or did I miss something?

Again, that looks correct to me, but please check.

Quoted 3 lines

Secondly, since there is already a condition that checks for RCU read-side critical section, the extra rcu_read_lock_held() we supply is sort of redundant and can be skipped right?

Yes, the general rule is that if the primitives ends with _rcu(), any lockdep condition will be in addition to rcu_read_lock_any_held(). So you should not need to pass RCU read-side lockdep expressions to primitives whose names end in _rcu..

Thanx, Paul

Quoted 87 lines

Thanks
Amol

Quoted 82 lines
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^

vim +/if +263 include/linux/rcupdate.h

632ee200130899 Paul E. McKenney 2010-02-22  254  
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  255  /**
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  256   * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  257   * @c: condition to check
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  258   * @s: informative message
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  259   */
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  260  #define RCU_LOCKDEP_WARN(c, s)						\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  261  	do {								\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  262  		static bool __section(.data.unlikely) __warned;		\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263  		if (debug_lockdep_rcu_enabled() && !__warned && (c)) {	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  264  			__warned = true;				\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  265  			lockdep_rcu_suspicious(__FILE__, __LINE__, s);	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  266  		}							\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  267  	} while (0)

f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268

:::::: The code at line 263 was first introduced by commit
:::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN()

:::::: TO: Paul E. McKenney <[email protected]> :::::: CC: Paul E. McKenney <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation

#5 lore raw
Quoted 82 lines · Amol Grover
Quoted 55 lines · Paul E. McKenney
Quoted 38 lines · Amol Grover
Quoted 23 lines · kbuild test robot

Hi Amol,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf
config: x86_64-randconfig-a003-20200109 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 10 lines
   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/char/ipmi/ipmi_msghandler.c:17:
   drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr':
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^

As mentioned above, RCU_LOCKDEP_WARN macro is called from __list_check_rcu with 2 parameters

Quoted 7 lines

1. !cond && !rcu_read_lock_any_held()
2. The message to display incase there is a lockdep warning.

However, if I pass the lockdep checking condition as:

list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held())

Right, given the _rcu() suffix on the command, the rcu_read_lock_held() is implied.

Quoted 5 lines

this trickles down to __list_check_rcu and then finally to
RCU_LOCKDEP_WARN as (here cond is lockdep_is_held(&some_lock) || rcu_read_lock_held()):

RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held())

Quoted 9 lines
Quoted 4 lines

which according to operator precedence (I hopefully got them right) would always evaluate to true if we are in an RCU read-side critical section (without a lock), and hence, result in a false-positive lockdep warning.

It looks that way to me. But why not actually try it out? After all, only the running system knows for sure. And there might be some trick that we are both missing.

I just tested this, here are the results:

Case 1: Usinglockdep_is_held() || rcu_read_lock_held()

lock RCU RSCS Splat? Actual
Y Y N N
Y N N N
N Y Y N <=
N N Y Y

Similar for
Case 2: Using rcu_read_lock_held() || lockdep_is_held()

Case 3: Consider 2 locks (outside rcu_read_lock()) lockdep_is_held(lock1) || lockdep_is_held(lock2)

Quoted 7 lines

lock1 lock2 Splat? Actual
Y Y N* N
Y N N N
N Y Y N <=
N N Y Y

This too proves the hypothesis (I'd like to call that).

Very good!

Quoted 16 lines

*However, this shows an interesting result. When both lock1 and lock2 are held, according to the hypothesis, a splat should've occured, since the check condition (albeit faulty atm) would be:

!lockdep_is_held(lock1) || lockdep_is_held(lock2) && !rcu_read_lock_any_held()

=> `!T || T && !F`
=> `F || T && T`
=> `F || T`

=> T
However, there was no splat. Which led me to investigate further and I found out:
1. rcu_read_lock_any_held() always returns 1 even if it is outside RCU read-side CS.
2. rcu_read_lock_held() seems OK, returns 1 when inside and 0 when outside

Quoted 3 lines

The kernel is compiled with
PROVE_RCU=y
PROVE_RCU_LIST=y

Were you within a preempt-disable region, for example, was some other spinlock held? (A lockdep splat should give you a list of locks held.) Both of these act as generalized RCU read-side critical sections in recent kernels.

Quoted 2 lines

Any thoughts on this? Is this intended? And should I send-in the patch for the first problem?

Separate patches for the initial problem and fixing the macro argument, please, if that is what you are asking.

Thanx, Paul

Quoted 107 lines

Thanks
Amol

Quoted 104 lines
Quoted 3 lines

This could be easily solved by putting cond inside brackets as it is correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu macro. Is that so, or did I miss something?

Again, that looks correct to me, but please check.

Quoted 3 lines

Secondly, since there is already a condition that checks for RCU read-side critical section, the extra rcu_read_lock_held() we supply is sort of redundant and can be skipped right?

Yes, the general rule is that if the primitives ends with _rcu(), any lockdep condition will be in addition to rcu_read_lock_any_held(). So you should not need to pass RCU read-side lockdep expressions to primitives whose names end in _rcu..

Thanx, Paul

Quoted 87 lines

Thanks
Amol

Quoted 82 lines
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^

vim +/if +263 include/linux/rcupdate.h

632ee200130899 Paul E. McKenney 2010-02-22  254  
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  255  /**
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  256   * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  257   * @c: condition to check
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  258   * @s: informative message
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  259   */
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  260  #define RCU_LOCKDEP_WARN(c, s)						\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  261  	do {								\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  262  		static bool __section(.data.unlikely) __warned;		\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263  		if (debug_lockdep_rcu_enabled() && !__warned && (c)) {	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  264  			__warned = true;				\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  265  			lockdep_rcu_suspicious(__FILE__, __LINE__, s);	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  266  		}							\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  267  	} while (0)

f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268

:::::: The code at line 263 was first introduced by commit
:::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN()

:::::: TO: Paul E. McKenney <[email protected]> :::::: CC: Paul E. McKenney <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

author
#6 lore raw
Quoted 109 lines · Paul E. McKenney
Quoted 82 lines · Amol Grover
Quoted 55 lines · Paul E. McKenney
Quoted 38 lines · Amol Grover
Quoted 23 lines · kbuild test robot

Hi Amol,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf
config: x86_64-randconfig-a003-20200109 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 10 lines
   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/char/ipmi/ipmi_msghandler.c:17:
   drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr':
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^

As mentioned above, RCU_LOCKDEP_WARN macro is called from __list_check_rcu with 2 parameters

Quoted 7 lines

1. !cond && !rcu_read_lock_any_held()
2. The message to display incase there is a lockdep warning.

However, if I pass the lockdep checking condition as:

list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held())

Right, given the _rcu() suffix on the command, the rcu_read_lock_held() is implied.

Quoted 5 lines

this trickles down to __list_check_rcu and then finally to
RCU_LOCKDEP_WARN as (here cond is lockdep_is_held(&some_lock) || rcu_read_lock_held()):

RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held())

Quoted 9 lines
Quoted 4 lines

which according to operator precedence (I hopefully got them right) would always evaluate to true if we are in an RCU read-side critical section (without a lock), and hence, result in a false-positive lockdep warning.

It looks that way to me. But why not actually try it out? After all, only the running system knows for sure. And there might be some trick that we are both missing.

I just tested this, here are the results:

Case 1: Usinglockdep_is_held() || rcu_read_lock_held()

lock RCU RSCS Splat? Actual
Y Y N N
Y N N N
N Y Y N <=
N N Y Y

Similar for
Case 2: Using rcu_read_lock_held() || lockdep_is_held()

Case 3: Consider 2 locks (outside rcu_read_lock()) lockdep_is_held(lock1) || lockdep_is_held(lock2)

Quoted 7 lines

lock1 lock2 Splat? Actual
Y Y N* N
Y N N N
N Y Y N <=
N N Y Y

This too proves the hypothesis (I'd like to call that).

Very good!

Quoted 16 lines

*However, this shows an interesting result. When both lock1 and lock2 are held, according to the hypothesis, a splat should've occured, since the check condition (albeit faulty atm) would be:

!lockdep_is_held(lock1) || lockdep_is_held(lock2) && !rcu_read_lock_any_held()

=> `!T || T && !F`
=> `F || T && T`
=> `F || T`

=> T
However, there was no splat. Which led me to investigate further and I found out:
1. rcu_read_lock_any_held() always returns 1 even if it is outside RCU read-side CS.
2. rcu_read_lock_held() seems OK, returns 1 when inside and 0 when outside

Quoted 9 lines
Quoted 3 lines

The kernel is compiled with
PROVE_RCU=y
PROVE_RCU_LIST=y

Were you within a preempt-disable region, for example, was some other spinlock held? (A lockdep splat should give you a list of locks held.) Both of these act as generalized RCU read-side critical sections in recent kernels.

I'm not sure if I was inside a preempt-disable region. I just created a thread via kthread_run and ran rcu_read_lock_any_held() inside the thread, I'll investigate further and get back to you. And no-other locks were held. Here is the splat:

[ 2724.551419] =============================
[ 2724.551420] WARNING: suspicious RCU usage
[ 2724.551421] 5.5.0-rc5-next #15 Tainted: G           OE    
[ 2724.551422] -----------------------------
[ 2724.551423] /home/amolg/git/lockdep_check/main.c:28 RCU-list
traversed in non-reader section!!
[ 2724.551424] 
               other info that might help us debug this:
[ 2724.551425] 
               rcu_scheduler_active = 2, debug_locks = 1
[ 2724.551426] no locks held by my_thread/4275.
[ 2724.551426] 
               stack backtrace:
[ 2724.551428] CPU: 2 PID: 4275 Comm: my_thread Tainted: G           OE
5.5.0-rc5-next #15
[ 2724.551429] Hardware name: Gigabyte Technology Co., Ltd.
Z170-D3H/Z170-D3H-CF, BIOS F21 03/06/2017
[ 2724.551430] Call Trace:
[ 2724.551437]  dump_stack+0x8f/0xd0
[ 2724.551440]  threadfn+0x100/0x105 [main]
[ 2724.551443]  ? __kthread_parkme+0x48/0x60
[ 2724.551445]  kthread+0xf9/0x130
[ 2724.551447]  ? 0xffffffffc00d7000
[ 2724.551448]  ? kthread_park+0x90/0x90
[ 2724.551451]  ret_from_fork+0x3a/0x50
Quoted 6 lines
Quoted 2 lines

Any thoughts on this? Is this intended? And should I send-in the patch for the first problem?

Separate patches for the initial problem and fixing the macro argument, please, if that is what you are asking.

Sorry if I didn't make myself clear earlier. By first problem I was referring to the macro argument problem itself.

I think by initial problem you mean this patch (drivers/char/ipmi), right? I'll make sure to send in a separate patch for the macro argument problem.

Thanks
Amol

Quoted 109 lines

Thanx, Paul

Quoted 107 lines

Thanks
Amol

Quoted 104 lines
Quoted 3 lines

This could be easily solved by putting cond inside brackets as it is correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu macro. Is that so, or did I miss something?

Again, that looks correct to me, but please check.

Quoted 3 lines

Secondly, since there is already a condition that checks for RCU read-side critical section, the extra rcu_read_lock_held() we supply is sort of redundant and can be skipped right?

Yes, the general rule is that if the primitives ends with _rcu(), any lockdep condition will be in addition to rcu_read_lock_any_held(). So you should not need to pass RCU read-side lockdep expressions to primitives whose names end in _rcu..

Thanx, Paul

Quoted 87 lines

Thanks
Amol

Quoted 82 lines
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^

vim +/if +263 include/linux/rcupdate.h

632ee200130899 Paul E. McKenney 2010-02-22  254  
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  255  /**
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  256   * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  257   * @c: condition to check
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  258   * @s: informative message
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  259   */
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  260  #define RCU_LOCKDEP_WARN(c, s)						\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  261  	do {								\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  262  		static bool __section(.data.unlikely) __warned;		\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263  		if (debug_lockdep_rcu_enabled() && !__warned && (c)) {	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  264  			__warned = true;				\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  265  			lockdep_rcu_suspicious(__FILE__, __LINE__, s);	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  266  		}							\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  267  	} while (0)

f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268

:::::: The code at line 263 was first introduced by commit
:::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN()

:::::: TO: Paul E. McKenney <[email protected]> :::::: CC: Paul E. McKenney <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation

#7 lore raw
Quoted 142 lines · Amol Grover
Quoted 109 lines · Paul E. McKenney
Quoted 82 lines · Amol Grover
Quoted 55 lines · Paul E. McKenney
Quoted 38 lines · Amol Grover
Quoted 23 lines · kbuild test robot

Hi Amol,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf
config: x86_64-randconfig-a003-20200109 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 10 lines
   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/char/ipmi/ipmi_msghandler.c:17:
   drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr':
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^

As mentioned above, RCU_LOCKDEP_WARN macro is called from __list_check_rcu with 2 parameters

Quoted 7 lines

1. !cond && !rcu_read_lock_any_held()
2. The message to display incase there is a lockdep warning.

However, if I pass the lockdep checking condition as:

list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held())

Right, given the _rcu() suffix on the command, the rcu_read_lock_held() is implied.

Quoted 5 lines

this trickles down to __list_check_rcu and then finally to
RCU_LOCKDEP_WARN as (here cond is lockdep_is_held(&some_lock) || rcu_read_lock_held()):

RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held())

Quoted 9 lines
Quoted 4 lines

which according to operator precedence (I hopefully got them right) would always evaluate to true if we are in an RCU read-side critical section (without a lock), and hence, result in a false-positive lockdep warning.

It looks that way to me. But why not actually try it out? After all, only the running system knows for sure. And there might be some trick that we are both missing.

I just tested this, here are the results:

Case 1: Usinglockdep_is_held() || rcu_read_lock_held()

lock RCU RSCS Splat? Actual
Y Y N N
Y N N N
N Y Y N <=
N N Y Y

Similar for
Case 2: Using rcu_read_lock_held() || lockdep_is_held()

Case 3: Consider 2 locks (outside rcu_read_lock()) lockdep_is_held(lock1) || lockdep_is_held(lock2)

Quoted 7 lines

lock1 lock2 Splat? Actual
Y Y N* N
Y N N N
N Y Y N <=
N N Y Y

This too proves the hypothesis (I'd like to call that).

Very good!

Quoted 16 lines

*However, this shows an interesting result. When both lock1 and lock2 are held, according to the hypothesis, a splat should've occured, since the check condition (albeit faulty atm) would be:

!lockdep_is_held(lock1) || lockdep_is_held(lock2) && !rcu_read_lock_any_held()

=> `!T || T && !F`
=> `F || T && T`
=> `F || T`

=> T
However, there was no splat. Which led me to investigate further and I found out:
1. rcu_read_lock_any_held() always returns 1 even if it is outside RCU read-side CS.
2. rcu_read_lock_held() seems OK, returns 1 when inside and 0 when outside

Quoted 8 lines
Quoted 3 lines

The kernel is compiled with
PROVE_RCU=y
PROVE_RCU_LIST=y

Were you within a preempt-disable region, for example, was some other spinlock held? (A lockdep splat should give you a list of locks held.) Both of these act as generalized RCU read-side critical sections in recent kernels.

I'm not sure if I was inside a preempt-disable region. I just created a thread via kthread_run and ran rcu_read_lock_any_held() inside the thread, I'll investigate further and get back to you. And no-other locks were held. Here is the splat:

[ 2724.551419] =============================
[ 2724.551420] WARNING: suspicious RCU usage
[ 2724.551421] 5.5.0-rc5-next #15 Tainted: G           OE    
[ 2724.551422] -----------------------------
[ 2724.551423] /home/amolg/git/lockdep_check/main.c:28 RCU-list
traversed in non-reader section!!
[ 2724.551424] 
               other info that might help us debug this:
[ 2724.551425] 
               rcu_scheduler_active = 2, debug_locks = 1
[ 2724.551426] no locks held by my_thread/4275.
[ 2724.551426] 
               stack backtrace:
[ 2724.551428] CPU: 2 PID: 4275 Comm: my_thread Tainted: G           OE
5.5.0-rc5-next #15
[ 2724.551429] Hardware name: Gigabyte Technology Co., Ltd.

Z170-D3H/Z170-D3H-CF, BIOS F21 03/06/2017

Quoted 8 lines
[ 2724.551430] Call Trace:
[ 2724.551437]  dump_stack+0x8f/0xd0
[ 2724.551440]  threadfn+0x100/0x105 [main]
[ 2724.551443]  ? __kthread_parkme+0x48/0x60
[ 2724.551445]  kthread+0xf9/0x130
[ 2724.551447]  ? 0xffffffffc00d7000
[ 2724.551448]  ? kthread_park+0x90/0x90
[ 2724.551451]  ret_from_fork+0x3a/0x50

Does your .config file by chance contain "CONFIG_PREEMPT=n"?

In that case, pretty much any code not containing a call to schedule() or a return to userspace is an implicit RCU read-side critical section, which would mean that this is expected behavior.

Quoted 6 lines
Quoted 5 lines
Quoted 2 lines

Any thoughts on this? Is this intended? And should I send-in the patch for the first problem?

Separate patches for the initial problem and fixing the macro argument, please, if that is what you are asking.

Quoted 6 lines

Sorry if I didn't make myself clear earlier. By first problem I was referring to the macro argument problem itself.

I think by initial problem you mean this patch (drivers/char/ipmi), right? I'll make sure to send in a separate patch for the macro argument problem.

I did indeed mean that, thank you!

Thanx, Paul

Quoted 112 lines

Thanks
Amol

Quoted 109 lines

Thanx, Paul

Quoted 107 lines

Thanks
Amol

Quoted 104 lines
Quoted 3 lines

This could be easily solved by putting cond inside brackets as it is correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu macro. Is that so, or did I miss something?

Again, that looks correct to me, but please check.

Quoted 3 lines

Secondly, since there is already a condition that checks for RCU read-side critical section, the extra rcu_read_lock_held() we supply is sort of redundant and can be skipped right?

Yes, the general rule is that if the primitives ends with _rcu(), any lockdep condition will be in addition to rcu_read_lock_any_held(). So you should not need to pass RCU read-side lockdep expressions to primitives whose names end in _rcu..

Thanx, Paul

Quoted 87 lines

Thanks
Amol

Quoted 82 lines
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^

vim +/if +263 include/linux/rcupdate.h

632ee200130899 Paul E. McKenney 2010-02-22  254  
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  255  /**
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  256   * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  257   * @c: condition to check
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  258   * @s: informative message
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  259   */
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  260  #define RCU_LOCKDEP_WARN(c, s)						\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  261  	do {								\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  262  		static bool __section(.data.unlikely) __warned;		\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263  		if (debug_lockdep_rcu_enabled() && !__warned && (c)) {	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  264  			__warned = true;				\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  265  			lockdep_rcu_suspicious(__FILE__, __LINE__, s);	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  266  		}							\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  267  	} while (0)

f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268

:::::: The code at line 263 was first introduced by commit
:::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN()

:::::: TO: Paul E. McKenney <[email protected]> :::::: CC: Paul E. McKenney <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

author
#8 lore raw
Quoted 152 lines · Paul E. McKenney
Quoted 142 lines · Amol Grover
Quoted 109 lines · Paul E. McKenney
Quoted 82 lines · Amol Grover
Quoted 55 lines · Paul E. McKenney
Quoted 38 lines · Amol Grover
Quoted 23 lines · kbuild test robot

Hi Amol,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on ipmi/for-next arm-soc/for-next v5.5-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Amol-Grover/drivers-char-ipmi-ipmi_msghandler-Pass-lockdep-expression-to-RCU-lists/20200111-081002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 16bb7abc4a6b9defffa294e4dc28383e62a1dbcf
config: x86_64-randconfig-a003-20200109 (attached as .config)
compiler: gcc-5 (Ubuntu 5.5.0-12ubuntu1) 5.5.0 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

Quoted 10 lines
   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from include/linux/list.h:9,
                    from include/linux/module.h:12,
                    from drivers/char/ipmi/ipmi_msghandler.c:17:
   drivers/char/ipmi/ipmi_msghandler.c: In function 'find_cmd_rcvr':
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^

As mentioned above, RCU_LOCKDEP_WARN macro is called from __list_check_rcu with 2 parameters

Quoted 7 lines

1. !cond && !rcu_read_lock_any_held()
2. The message to display incase there is a lockdep warning.

However, if I pass the lockdep checking condition as:

list_for_each_entry_rcu(ptr, list, head, lockdep_is_held(&some_lock) || rcu_read_lock_held())

Right, given the _rcu() suffix on the command, the rcu_read_lock_held() is implied.

Quoted 5 lines

this trickles down to __list_check_rcu and then finally to
RCU_LOCKDEP_WARN as (here cond is lockdep_is_held(&some_lock) || rcu_read_lock_held()):

RCU_LOCKDEP_WARN(!lockdep_is_held(&some_lock) || rcu_read_lock_held() && !rcu_read_lock_any_held())

Quoted 9 lines
Quoted 4 lines

which according to operator precedence (I hopefully got them right) would always evaluate to true if we are in an RCU read-side critical section (without a lock), and hence, result in a false-positive lockdep warning.

It looks that way to me. But why not actually try it out? After all, only the running system knows for sure. And there might be some trick that we are both missing.

I just tested this, here are the results:

Case 1: Usinglockdep_is_held() || rcu_read_lock_held()

lock RCU RSCS Splat? Actual
Y Y N N
Y N N N
N Y Y N <=
N N Y Y

Similar for
Case 2: Using rcu_read_lock_held() || lockdep_is_held()

Case 3: Consider 2 locks (outside rcu_read_lock()) lockdep_is_held(lock1) || lockdep_is_held(lock2)

Quoted 7 lines

lock1 lock2 Splat? Actual
Y Y N* N
Y N N N
N Y Y N <=
N N Y Y

This too proves the hypothesis (I'd like to call that).

Very good!

Quoted 16 lines

*However, this shows an interesting result. When both lock1 and lock2 are held, according to the hypothesis, a splat should've occured, since the check condition (albeit faulty atm) would be:

!lockdep_is_held(lock1) || lockdep_is_held(lock2) && !rcu_read_lock_any_held()

=> `!T || T && !F`
=> `F || T && T`
=> `F || T`

=> T
However, there was no splat. Which led me to investigate further and I found out:
1. rcu_read_lock_any_held() always returns 1 even if it is outside RCU read-side CS.
2. rcu_read_lock_held() seems OK, returns 1 when inside and 0 when outside

Quoted 8 lines
Quoted 3 lines

The kernel is compiled with
PROVE_RCU=y
PROVE_RCU_LIST=y

Were you within a preempt-disable region, for example, was some other spinlock held? (A lockdep splat should give you a list of locks held.) Both of these act as generalized RCU read-side critical sections in recent kernels.

I'm not sure if I was inside a preempt-disable region. I just created a thread via kthread_run and ran rcu_read_lock_any_held() inside the thread, I'll investigate further and get back to you. And no-other locks were held. Here is the splat:

[ 2724.551419] =============================
[ 2724.551420] WARNING: suspicious RCU usage
[ 2724.551421] 5.5.0-rc5-next #15 Tainted: G           OE    
[ 2724.551422] -----------------------------
[ 2724.551423] /home/amolg/git/lockdep_check/main.c:28 RCU-list
traversed in non-reader section!!
[ 2724.551424] 
               other info that might help us debug this:
[ 2724.551425] 
               rcu_scheduler_active = 2, debug_locks = 1
[ 2724.551426] no locks held by my_thread/4275.
[ 2724.551426] 
               stack backtrace:
[ 2724.551428] CPU: 2 PID: 4275 Comm: my_thread Tainted: G           OE
5.5.0-rc5-next #15
[ 2724.551429] Hardware name: Gigabyte Technology Co., Ltd.

Z170-D3H/Z170-D3H-CF, BIOS F21 03/06/2017

Quoted 8 lines
[ 2724.551430] Call Trace:
[ 2724.551437]  dump_stack+0x8f/0xd0
[ 2724.551440]  threadfn+0x100/0x105 [main]
[ 2724.551443]  ? __kthread_parkme+0x48/0x60
[ 2724.551445]  kthread+0xf9/0x130
[ 2724.551447]  ? 0xffffffffc00d7000
[ 2724.551448]  ? kthread_park+0x90/0x90
[ 2724.551451]  ret_from_fork+0x3a/0x50
Quoted 2 lines

Does your .config file by chance contain "CONFIG_PREEMPT=n"?

Indeed, CONFIG_PREEMPT was not set in my config, hence, the different results.

Quoted 4 lines

In that case, pretty much any code not containing a call to schedule() or a return to userspace is an implicit RCU read-side critical section, which would mean that this is expected behavior.

Got it, thank you!

Thanks
Amol

Quoted 129 lines
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines

Any thoughts on this? Is this intended? And should I send-in the patch for the first problem?

Separate patches for the initial problem and fixing the macro argument, please, if that is what you are asking.

Quoted 6 lines

Sorry if I didn't make myself clear earlier. By first problem I was referring to the macro argument problem itself.

I think by initial problem you mean this patch (drivers/char/ipmi), right? I'll make sure to send in a separate patch for the macro argument problem.

I did indeed mean that, thank you!

Thanx, Paul

Quoted 112 lines

Thanks
Amol

Quoted 109 lines

Thanx, Paul

Quoted 107 lines

Thanks
Amol

Quoted 104 lines
Quoted 3 lines

This could be easily solved by putting cond inside brackets as it is correctly done in RCU_LOCKDEP_WARN macro but not in __list_check_rcu macro. Is that so, or did I miss something?

Again, that looks correct to me, but please check.

Quoted 3 lines

Secondly, since there is already a condition that checks for RCU read-side critical section, the extra rcu_read_lock_held() we supply is sort of redundant and can be skipped right?

Yes, the general rule is that if the primitives ends with _rcu(), any lockdep condition will be in addition to rcu_read_lock_any_held(). So you should not need to pass RCU read-side lockdep expressions to primitives whose names end in _rcu..

Thanx, Paul

Quoted 87 lines

Thanks
Amol

Quoted 82 lines
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^
   include/linux/rculist.h:53:25: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
                            ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^
Quoted 1 line
Quoted 1 line

include/linux/rcupdate.h:263:3: note: in expansion of macro 'if'

      if (debug_lockdep_rcu_enabled() && !__warned && (c)) { \
      ^
Quoted 1 line
Quoted 1 line

include/linux/rculist.h:53:2: note: in expansion of macro 'RCU_LOCKDEP_WARN'

     RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),  \
     ^
   include/linux/rculist.h:371:7: note: in expansion of macro '__list_check_rcu'
     for (__list_check_rcu(dummy, ## cond, 0),   \
          ^
   drivers/char/ipmi/ipmi_msghandler.c:1607:2: note: in expansion of macro 'list_for_each_entry_rcu'
     list_for_each_entry_rcu(rcvr, &intf->cmd_rcvrs, link,
     ^

vim +/if +263 include/linux/rcupdate.h

632ee200130899 Paul E. McKenney 2010-02-22  254  
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  255  /**
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  256   * RCU_LOCKDEP_WARN - emit lockdep splat if specified condition is met
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  257   * @c: condition to check
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  258   * @s: informative message
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  259   */
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  260  #define RCU_LOCKDEP_WARN(c, s)						\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  261  	do {								\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  262  		static bool __section(.data.unlikely) __warned;		\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 @263  		if (debug_lockdep_rcu_enabled() && !__warned && (c)) {	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  264  			__warned = true;				\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  265  			lockdep_rcu_suspicious(__FILE__, __LINE__, s);	\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  266  		}							\
f78f5b90c4ffa5 Paul E. McKenney 2015-06-18  267  	} while (0)

f78f5b90c4ffa5 Paul E. McKenney 2015-06-18 268

:::::: The code at line 263 was first introduced by commit
:::::: f78f5b90c4ffa559e400c3919a02236101f29f3f rcu: Rename rcu_lockdep_assert() to RCU_LOCKDEP_WARN()

:::::: TO: Paul E. McKenney <[email protected]> :::::: CC: Paul E. McKenney <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation