linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/9] Harden list_for_each_entry_rcu() and family
@ 2019-07-15 14:36 Joel Fernandes (Google)
  2019-07-15 14:36 ` [PATCH 1/9] rcu/update: Remove useless check for debug_locks (v1) Joel Fernandes (Google)
                   ` (9 more replies)
  0 siblings, 10 replies; 26+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-15 14:36 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
	keescook, kernel-hardening, kernel-team, Lai Jiangshan,
	Len Brown, linux-acpi, linux-doc, linux-pci, linux-pm,
	Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

Hi,
This series aims to provide lockdep checking to RCU list macros for additional
kernel hardening.

RCU has a number of primitives for "consumption" of an RCU protected pointer.
Most of the time, these consumers make sure that such accesses are under a RCU
reader-section (such as rcu_dereference{,sched,bh} or under a lock, such as
with rcu_dereference_protected()).

However, there are other ways to consume RCU pointers, such as by
list_for_each_entry_rcu or hlist_for_each_enry_rcu. Unlike the rcu_dereference
family, these consumers do no lockdep checking at all. And with the growing
number of RCU list uses (1000+), it is possible for bugs to creep in and go
unnoticed which lockdep checks can catch.

Since RCU consolidation efforts last year, the different traditional RCU
flavors (preempt, bh, sched) are all consolidated. In other words, any of these
flavors can cause a reader section to occur and all of them must cease before
the reader section is considered to be unlocked. Thanks to this, we can
generically check if we are in an RCU reader. This is what patch 1 does. Note
that the list_for_each_entry_rcu and family are different from the
rcu_dereference family in that, there is no _bh or _sched version of this
macro. They are used under many different RCU reader flavors, and also SRCU.
Patch 1 adds a new internal function rcu_read_lock_any_held() which checks
if any reader section is active at all, when these macros are called. If no
reader section exists, then the optional fourth argument to
list_for_each_entry_rcu() can be a lockdep expression which is evaluated
(similar to how rcu_dereference_check() works). If no lockdep expression is
passed, and we are not in a reader, then a splat occurs. Just take off the
lockdep expression after applying the patches, by using the following diff and
see what happens:

+++ b/arch/x86/pci/mmconfig-shared.c
@@ -55,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
        struct pci_mmcfg_region *cfg;

        /* keep list sorted by segment and starting bus number */
-       list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) {
+       list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {


The optional argument trick to list_for_each_entry_rcu() can also be used in
the future to possibly remove rcu_dereference_{,bh,sched}_protected() API and
we can pass an optional lockdep expression to rcu_dereference() itself. Thus
eliminating 3 more RCU APIs.

Note that some list macro wrappers already do their own lockdep checking in the
caller side. These can be eliminated in favor of the built-in lockdep checking
in the list macro that this series adds. For example, workqueue code has a
assert_rcu_or_wq_mutex() function which is called in for_each_wq().  This
series replaces that in favor of the built-in check.

Also in the future, we can extend these checks to list_entry_rcu() and other
list macros as well, if needed.

Please note that I have kept this option default-disabled under a new config:
CONFIG_PROVE_RCU_LIST. This is so that until all users are converted to pass
the optional argument, we should keep the check disabled. There are about a
1000 or so users and it is not possible to pass in the optional lockdep
expression in a single series since it is done on a case-by-case basis. I did
convert a few users in this series itself.

v2->v3: Simplified rcu-sync logic after rebase (Paul)
	Added check for bh_map (Paul)
	Refactored out more of the common code (Joel)
	Added Oleg ack to rcu-sync patch.

v1->v2: Have assert_rcu_or_wq_mutex deleted (Daniel Jordan)
	Simplify rcu_read_lock_any_held()   (Peter Zijlstra)
	Simplified rcu-sync logic	    (Oleg Nesterov)
	Updated documentation and rculist comments.
	Added GregKH ack.

RFC->v1: 
	Simplify list checking macro (Rasmus Villemoes)

Joel Fernandes (Google) (9):
rcu/update: Remove useless check for debug_locks (v1)
rcu: Add support for consolidated-RCU reader checking (v3)
rcu/sync: Remove custom check for reader-section (v2)
ipv4: add lockdep condition to fix for_each_entry (v1)
driver/core: Convert to use built-in RCU list checking (v1)
workqueue: Convert for_each_wq to use built-in list check (v2)
x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
acpi: Use built-in RCU list checking for acpi_ioremaps list (v1)
doc: Update documentation about list_for_each_entry_rcu (v1)

Documentation/RCU/lockdep.txt   | 15 ++++++++---
Documentation/RCU/whatisRCU.txt |  9 ++++++-
arch/x86/pci/mmconfig-shared.c  |  5 ++--
drivers/acpi/osl.c              |  6 +++--
drivers/base/base.h             |  1 +
drivers/base/core.c             | 10 +++++++
drivers/base/power/runtime.c    | 15 +++++++----
include/linux/rcu_sync.h        |  4 +--
include/linux/rculist.h         | 28 +++++++++++++++----
include/linux/rcupdate.h        |  7 +++++
kernel/rcu/Kconfig.debug        | 11 ++++++++
kernel/rcu/update.c             | 48 ++++++++++++++++++---------------
kernel/workqueue.c              | 10 ++-----
net/ipv4/fib_frontend.c         |  3 ++-
14 files changed, 119 insertions(+), 53 deletions(-)

--
2.22.0.510.g264f2c817a-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/9] rcu/update: Remove useless check for debug_locks (v1)
  2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
@ 2019-07-15 14:36 ` Joel Fernandes (Google)
  2019-07-15 14:36 ` [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3) Joel Fernandes (Google)
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 26+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-15 14:36 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
	keescook, kernel-hardening, kernel-team, Lai Jiangshan,
	Len Brown, linux-acpi, linux-doc, linux-pci, linux-pm,
	Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

In rcu_read_lock_sched_held(), debug_locks can never be true at the
point we check it because we already check debug_locks in
debug_lockdep_rcu_enabled() in the beginning. Remove the check.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/update.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 61df2bf08563..9dd5aeef6e70 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -93,17 +93,13 @@ module_param(rcu_normal_after_boot, int, 0);
  */
 int rcu_read_lock_sched_held(void)
 {
-	int lockdep_opinion = 0;
-
 	if (!debug_lockdep_rcu_enabled())
 		return 1;
 	if (!rcu_is_watching())
 		return 0;
 	if (!rcu_lockdep_current_cpu_online())
 		return 0;
-	if (debug_locks)
-		lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
-	return lockdep_opinion || !preemptible();
+	return lock_is_held(&rcu_sched_lock_map) || !preemptible();
 }
 EXPORT_SYMBOL(rcu_read_lock_sched_held);
 #endif
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
  2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
  2019-07-15 14:36 ` [PATCH 1/9] rcu/update: Remove useless check for debug_locks (v1) Joel Fernandes (Google)
@ 2019-07-15 14:36 ` Joel Fernandes (Google)
  2019-07-16 18:38   ` Paul E. McKenney
  2019-07-15 14:36 ` [PATCH 3/9] rcu/sync: Remove custom check for reader-section (v2) Joel Fernandes (Google)
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-15 14:36 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
	keescook, kernel-hardening, kernel-team, Lai Jiangshan,
	Len Brown, linux-acpi, linux-doc, linux-pci, linux-pm,
	Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

This patch adds support for checking RCU reader sections in list
traversal macros. Optionally, if the list macro is called under SRCU or
other lock/mutex protection, then appropriate lockdep expressions can be
passed to make the checks pass.

Existing list_for_each_entry_rcu() invocations don't need to pass the
optional fourth argument (cond) unless they are under some non-RCU
protection and needs to make lockdep check pass.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 include/linux/rculist.h  | 28 ++++++++++++++++++++-----
 include/linux/rcupdate.h |  7 +++++++
 kernel/rcu/Kconfig.debug | 11 ++++++++++
 kernel/rcu/update.c      | 44 ++++++++++++++++++++++++----------------
 4 files changed, 67 insertions(+), 23 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index e91ec9ddcd30..1048160625bb 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
  */
 #define list_next_rcu(list)	(*((struct list_head __rcu **)(&(list)->next)))
 
+/*
+ * Check during list traversal that we are within an RCU reader
+ */
+
+#ifdef CONFIG_PROVE_RCU_LIST
+#define __list_check_rcu(dummy, cond, ...)				\
+	({								\
+	RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),		\
+			 "RCU-list traversed in non-reader section!");	\
+	 })
+#else
+#define __list_check_rcu(dummy, cond, ...) ({})
+#endif
+
 /*
  * Insert a new entry between two known consecutive entries.
  *
@@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * @pos:	the type * to use as a loop cursor.
  * @head:	the head for your list.
  * @member:	the name of the list_head within the struct.
+ * @cond:	optional lockdep expression if called from non-RCU protection.
  *
  * This list-traversal primitive may safely run concurrently with
  * the _rcu list-mutation primitives such as list_add_rcu()
  * as long as the traversal is guarded by rcu_read_lock().
  */
-#define list_for_each_entry_rcu(pos, head, member) \
-	for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
-		&pos->member != (head); \
+#define list_for_each_entry_rcu(pos, head, member, cond...)		\
+	for (__list_check_rcu(dummy, ## cond, 0),			\
+	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
+		&pos->member != (head);					\
 		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
 
 /**
@@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
  * @pos:	the type * to use as a loop cursor.
  * @head:	the head for your list.
  * @member:	the name of the hlist_node within the struct.
+ * @cond:	optional lockdep expression if called from non-RCU protection.
  *
  * This list-traversal primitive may safely run concurrently with
  * the _rcu list-mutation primitives such as hlist_add_head_rcu()
  * as long as the traversal is guarded by rcu_read_lock().
  */
-#define hlist_for_each_entry_rcu(pos, head, member)			\
-	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
+#define hlist_for_each_entry_rcu(pos, head, member, cond...)		\
+	for (__list_check_rcu(dummy, ## cond, 0),			\
+	     pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
 			typeof(*(pos)), member);			\
 		pos;							\
 		pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 8f7167478c1d..f3c29efdf19a 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -221,6 +221,7 @@ int debug_lockdep_rcu_enabled(void);
 int rcu_read_lock_held(void);
 int rcu_read_lock_bh_held(void);
 int rcu_read_lock_sched_held(void);
+int rcu_read_lock_any_held(void);
 
 #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
@@ -241,6 +242,12 @@ static inline int rcu_read_lock_sched_held(void)
 {
 	return !preemptible();
 }
+
+static inline int rcu_read_lock_any_held(void)
+{
+	return !preemptible();
+}
+
 #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
 #ifdef CONFIG_PROVE_RCU
diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
index 5ec3ea4028e2..7fbd21dbfcd0 100644
--- a/kernel/rcu/Kconfig.debug
+++ b/kernel/rcu/Kconfig.debug
@@ -8,6 +8,17 @@ menu "RCU Debugging"
 config PROVE_RCU
 	def_bool PROVE_LOCKING
 
+config PROVE_RCU_LIST
+	bool "RCU list lockdep debugging"
+	depends on PROVE_RCU
+	default n
+	help
+	  Enable RCU lockdep checking for list usages. By default it is
+	  turned off since there are several list RCU users that still
+	  need to be converted to pass a lockdep expression. To prevent
+	  false-positive splats, we keep it default disabled but once all
+	  users are converted, we can remove this config option.
+
 config TORTURE_TEST
 	tristate
 	default n
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 9dd5aeef6e70..b7a4e3b5fa98 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -91,14 +91,18 @@ module_param(rcu_normal_after_boot, int, 0);
  * Similarly, we avoid claiming an SRCU read lock held if the current
  * CPU is offline.
  */
+#define rcu_read_lock_held_common()		\
+	if (!debug_lockdep_rcu_enabled())	\
+		return 1;			\
+	if (!rcu_is_watching())			\
+		return 0;			\
+	if (!rcu_lockdep_current_cpu_online())	\
+		return 0;
+
 int rcu_read_lock_sched_held(void)
 {
-	if (!debug_lockdep_rcu_enabled())
-		return 1;
-	if (!rcu_is_watching())
-		return 0;
-	if (!rcu_lockdep_current_cpu_online())
-		return 0;
+	rcu_read_lock_held_common();
+
 	return lock_is_held(&rcu_sched_lock_map) || !preemptible();
 }
 EXPORT_SYMBOL(rcu_read_lock_sched_held);
@@ -257,12 +261,8 @@ NOKPROBE_SYMBOL(debug_lockdep_rcu_enabled);
  */
 int rcu_read_lock_held(void)
 {
-	if (!debug_lockdep_rcu_enabled())
-		return 1;
-	if (!rcu_is_watching())
-		return 0;
-	if (!rcu_lockdep_current_cpu_online())
-		return 0;
+	rcu_read_lock_held_common();
+
 	return lock_is_held(&rcu_lock_map);
 }
 EXPORT_SYMBOL_GPL(rcu_read_lock_held);
@@ -284,16 +284,24 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_held);
  */
 int rcu_read_lock_bh_held(void)
 {
-	if (!debug_lockdep_rcu_enabled())
-		return 1;
-	if (!rcu_is_watching())
-		return 0;
-	if (!rcu_lockdep_current_cpu_online())
-		return 0;
+	rcu_read_lock_held_common();
+
 	return in_softirq() || irqs_disabled();
 }
 EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
 
+int rcu_read_lock_any_held(void)
+{
+	rcu_read_lock_held_common();
+
+	if (lock_is_held(&rcu_lock_map) ||
+	    lock_is_held(&rcu_bh_lock_map) ||
+	    lock_is_held(&rcu_sched_lock_map))
+		return 1;
+	return !preemptible();
+}
+EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
+
 #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
 /**
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/9] rcu/sync: Remove custom check for reader-section (v2)
  2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
  2019-07-15 14:36 ` [PATCH 1/9] rcu/update: Remove useless check for debug_locks (v1) Joel Fernandes (Google)
  2019-07-15 14:36 ` [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3) Joel Fernandes (Google)
@ 2019-07-15 14:36 ` Joel Fernandes (Google)
  2019-07-16 18:39   ` Paul E. McKenney
  2019-07-15 14:37 ` [PATCH 4/9] ipv4: add lockdep condition to fix for_each_entry (v1) Joel Fernandes (Google)
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-15 14:36 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Oleg Nesterov, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Paul E. McKenney,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

The rcu/sync code was doing its own check whether we are in a reader
section. With RCU consolidating flavors and the generic helper added in
this series, this is no longer need. We can just use the generic helper
and it results in a nice cleanup.

Cc: Oleg Nesterov <oleg@redhat.com>
Acked-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 include/linux/rcu_sync.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
index 9b83865d24f9..0027d4c8087c 100644
--- a/include/linux/rcu_sync.h
+++ b/include/linux/rcu_sync.h
@@ -31,9 +31,7 @@ struct rcu_sync {
  */
 static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
 {
-	RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
-			 !rcu_read_lock_bh_held() &&
-			 !rcu_read_lock_sched_held(),
+	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
 			 "suspicious rcu_sync_is_idle() usage");
 	return !READ_ONCE(rsp->gp_state); /* GP_IDLE */
 }
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 4/9] ipv4: add lockdep condition to fix for_each_entry (v1)
  2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
                   ` (2 preceding siblings ...)
  2019-07-15 14:36 ` [PATCH 3/9] rcu/sync: Remove custom check for reader-section (v2) Joel Fernandes (Google)
@ 2019-07-15 14:37 ` Joel Fernandes (Google)
  2019-07-16 18:39   ` Paul E. McKenney
  2019-07-15 14:37 ` [PATCH 5/9] driver/core: Convert to use built-in RCU list checking (v1) Joel Fernandes (Google)
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-15 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
	keescook, kernel-hardening, kernel-team, Lai Jiangshan,
	Len Brown, linux-acpi, linux-doc, linux-pci, linux-pm,
	Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

Using the previous support added, use it for adding lockdep conditions
to list usage here.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 net/ipv4/fib_frontend.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 317339cd7f03..26b0fb24e2c2 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -124,7 +124,8 @@ struct fib_table *fib_get_table(struct net *net, u32 id)
 	h = id & (FIB_TABLE_HASHSZ - 1);
 
 	head = &net->ipv4.fib_table_hash[h];
-	hlist_for_each_entry_rcu(tb, head, tb_hlist) {
+	hlist_for_each_entry_rcu(tb, head, tb_hlist,
+				 lockdep_rtnl_is_held()) {
 		if (tb->tb_id == id)
 			return tb;
 	}
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 5/9] driver/core: Convert to use built-in RCU list checking (v1)
  2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
                   ` (3 preceding siblings ...)
  2019-07-15 14:37 ` [PATCH 4/9] ipv4: add lockdep condition to fix for_each_entry (v1) Joel Fernandes (Google)
@ 2019-07-15 14:37 ` Joel Fernandes (Google)
  2019-07-16 18:40   ` Paul E. McKenney
  2019-07-15 14:37 ` [PATCH 6/9] workqueue: Convert for_each_wq to use built-in list check (v2) Joel Fernandes (Google)
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-15 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Greg Kroah-Hartman, Alexey Kuznetsov, Bjorn Helgaas,
	Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
it in driver core.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 drivers/base/base.h          |  1 +
 drivers/base/core.c          | 10 ++++++++++
 drivers/base/power/runtime.c | 15 ++++++++++-----
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index b405436ee28e..0d32544b6f91 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -165,6 +165,7 @@ static inline int devtmpfs_init(void) { return 0; }
 /* Device links support */
 extern int device_links_read_lock(void);
 extern void device_links_read_unlock(int idx);
+extern int device_links_read_lock_held(void);
 extern int device_links_check_suppliers(struct device *dev);
 extern void device_links_driver_bound(struct device *dev);
 extern void device_links_driver_cleanup(struct device *dev);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index da84a73f2ba6..85e82f38717f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -68,6 +68,11 @@ void device_links_read_unlock(int idx)
 {
 	srcu_read_unlock(&device_links_srcu, idx);
 }
+
+int device_links_read_lock_held(void)
+{
+	return srcu_read_lock_held(&device_links_srcu);
+}
 #else /* !CONFIG_SRCU */
 static DECLARE_RWSEM(device_links_lock);
 
@@ -91,6 +96,11 @@ void device_links_read_unlock(int not_used)
 {
 	up_read(&device_links_lock);
 }
+
+int device_links_read_lock_held(void)
+{
+	return lock_is_held(&device_links_lock);
+}
 #endif /* !CONFIG_SRCU */
 
 /**
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 952a1e7057c7..7a10e8379a70 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -287,7 +287,8 @@ static int rpm_get_suppliers(struct device *dev)
 {
 	struct device_link *link;
 
-	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
+	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+				device_links_read_lock_held()) {
 		int retval;
 
 		if (!(link->flags & DL_FLAG_PM_RUNTIME) ||
@@ -309,7 +310,8 @@ static void rpm_put_suppliers(struct device *dev)
 {
 	struct device_link *link;
 
-	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
+	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+				device_links_read_lock_held()) {
 		if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND)
 			continue;
 
@@ -1640,7 +1642,8 @@ void pm_runtime_clean_up_links(struct device *dev)
 
 	idx = device_links_read_lock();
 
-	list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
+	list_for_each_entry_rcu(link, &dev->links.consumers, s_node,
+				device_links_read_lock_held()) {
 		if (link->flags & DL_FLAG_STATELESS)
 			continue;
 
@@ -1662,7 +1665,8 @@ void pm_runtime_get_suppliers(struct device *dev)
 
 	idx = device_links_read_lock();
 
-	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
+	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+				device_links_read_lock_held())
 		if (link->flags & DL_FLAG_PM_RUNTIME) {
 			link->supplier_preactivated = true;
 			refcount_inc(&link->rpm_active);
@@ -1683,7 +1687,8 @@ void pm_runtime_put_suppliers(struct device *dev)
 
 	idx = device_links_read_lock();
 
-	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
+	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
+				device_links_read_lock_held())
 		if (link->supplier_preactivated) {
 			link->supplier_preactivated = false;
 			if (refcount_dec_not_one(&link->rpm_active))
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 6/9] workqueue: Convert for_each_wq to use built-in list check (v2)
  2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
                   ` (4 preceding siblings ...)
  2019-07-15 14:37 ` [PATCH 5/9] driver/core: Convert to use built-in RCU list checking (v1) Joel Fernandes (Google)
@ 2019-07-15 14:37 ` Joel Fernandes (Google)
  2019-07-16 18:41   ` Paul E. McKenney
  2019-07-15 14:37 ` [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1) Joel Fernandes (Google)
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-15 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
	keescook, kernel-hardening, kernel-team, Lai Jiangshan,
	Len Brown, linux-acpi, linux-doc, linux-pci, linux-pm,
	Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

list_for_each_entry_rcu now has support to check for RCU reader sections
as well as lock. Just use the support in it, instead of explictly
checking in the caller.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/workqueue.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 601d61150b65..e882477ebf6e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -364,11 +364,6 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
 			 !lockdep_is_held(&wq_pool_mutex),		\
 			 "RCU or wq_pool_mutex should be held")
 
-#define assert_rcu_or_wq_mutex(wq)					\
-	RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&			\
-			 !lockdep_is_held(&wq->mutex),			\
-			 "RCU or wq->mutex should be held")
-
 #define assert_rcu_or_wq_mutex_or_pool_mutex(wq)			\
 	RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&			\
 			 !lockdep_is_held(&wq->mutex) &&		\
@@ -425,9 +420,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
  * ignored.
  */
 #define for_each_pwq(pwq, wq)						\
-	list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node)		\
-		if (({ assert_rcu_or_wq_mutex(wq); false; })) { }	\
-		else
+	list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node,		\
+				 lock_is_held(&(wq->mutex).dep_map))
 
 #ifdef CONFIG_DEBUG_OBJECTS_WORK
 
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
  2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
                   ` (5 preceding siblings ...)
  2019-07-15 14:37 ` [PATCH 6/9] workqueue: Convert for_each_wq to use built-in list check (v2) Joel Fernandes (Google)
@ 2019-07-15 14:37 ` Joel Fernandes (Google)
  2019-07-15 20:02   ` Bjorn Helgaas
  2019-07-15 14:37 ` [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1) Joel Fernandes (Google)
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-15 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
	keescook, kernel-hardening, kernel-team, Lai Jiangshan,
	Len Brown, linux-acpi, linux-doc, linux-pci, linux-pm,
	Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
reader-lock held, because the pci_mmcfg_lock is already held. Make this
known to the list macro so that it fixes new lockdep warnings that
trigger due to lockdep checks added to list_for_each_entry_rcu().

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 arch/x86/pci/mmconfig-shared.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 7389db538c30..6fa42e9c4e6f 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -29,6 +29,7 @@
 static bool pci_mmcfg_running_state;
 static bool pci_mmcfg_arch_init_failed;
 static DEFINE_MUTEX(pci_mmcfg_lock);
+#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map)
 
 LIST_HEAD(pci_mmcfg_list);
 
@@ -54,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
 	struct pci_mmcfg_region *cfg;
 
 	/* keep list sorted by segment and starting bus number */
-	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
+	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) {
 		if (cfg->segment > new->segment ||
 		    (cfg->segment == new->segment &&
 		     cfg->start_bus >= new->start_bus)) {
@@ -118,7 +119,7 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
 {
 	struct pci_mmcfg_region *cfg;
 
-	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
+	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held())
 		if (cfg->segment == segment &&
 		    cfg->start_bus <= bus && bus <= cfg->end_bus)
 			return cfg;
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1)
  2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
                   ` (6 preceding siblings ...)
  2019-07-15 14:37 ` [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1) Joel Fernandes (Google)
@ 2019-07-15 14:37 ` Joel Fernandes (Google)
  2019-07-15 21:44   ` Rafael J. Wysocki
  2019-07-16 18:43   ` Paul E. McKenney
  2019-07-15 14:37 ` [PATCH 9/9] doc: Update documentation about list_for_each_entry_rcu (v1) Joel Fernandes (Google)
  2019-07-16 18:46 ` [PATCH 0/9] Harden list_for_each_entry_rcu() and family Paul E. McKenney
  9 siblings, 2 replies; 26+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-15 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
	keescook, kernel-hardening, kernel-team, Lai Jiangshan,
	Len Brown, linux-acpi, linux-doc, linux-pci, linux-pm,
	Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
it for acpi_ioremaps list traversal.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 drivers/acpi/osl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 9c0edf2fc0dd..2f9d0d20b836 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -14,6 +14,7 @@
 #include <linux/slab.h>
 #include <linux/mm.h>
 #include <linux/highmem.h>
+#include <linux/lockdep.h>
 #include <linux/pci.h>
 #include <linux/interrupt.h>
 #include <linux/kmod.h>
@@ -80,6 +81,7 @@ struct acpi_ioremap {
 
 static LIST_HEAD(acpi_ioremaps);
 static DEFINE_MUTEX(acpi_ioremap_lock);
+#define acpi_ioremap_lock_held() lock_is_held(&acpi_ioremap_lock.dep_map)
 
 static void __init acpi_request_region (struct acpi_generic_address *gas,
 	unsigned int length, char *desc)
@@ -206,7 +208,7 @@ acpi_map_lookup(acpi_physical_address phys, acpi_size size)
 {
 	struct acpi_ioremap *map;
 
-	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
+	list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
 		if (map->phys <= phys &&
 		    phys + size <= map->phys + map->size)
 			return map;
@@ -249,7 +251,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
 {
 	struct acpi_ioremap *map;
 
-	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
+	list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
 		if (map->virt <= virt &&
 		    virt + size <= map->virt + map->size)
 			return map;
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 9/9] doc: Update documentation about list_for_each_entry_rcu (v1)
  2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
                   ` (7 preceding siblings ...)
  2019-07-15 14:37 ` [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1) Joel Fernandes (Google)
@ 2019-07-15 14:37 ` Joel Fernandes (Google)
  2019-07-16 18:46 ` [PATCH 0/9] Harden list_for_each_entry_rcu() and family Paul E. McKenney
  9 siblings, 0 replies; 26+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-15 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
	keescook, kernel-hardening, kernel-team, Lai Jiangshan,
	Len Brown, linux-acpi, linux-doc, linux-pci, linux-pm,
	Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

This patch updates the documentation with information about
usage of lockdep with list_for_each_entry_rcu().

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 Documentation/RCU/lockdep.txt   | 15 +++++++++++----
 Documentation/RCU/whatisRCU.txt |  9 ++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/Documentation/RCU/lockdep.txt b/Documentation/RCU/lockdep.txt
index da51d3068850..3d967df3a801 100644
--- a/Documentation/RCU/lockdep.txt
+++ b/Documentation/RCU/lockdep.txt
@@ -96,7 +96,14 @@ other flavors of rcu_dereference().  On the other hand, it is illegal
 to use rcu_dereference_protected() if either the RCU-protected pointer
 or the RCU-protected data that it points to can change concurrently.
 
-There are currently only "universal" versions of the rcu_assign_pointer()
-and RCU list-/tree-traversal primitives, which do not (yet) check for
-being in an RCU read-side critical section.  In the future, separate
-versions of these primitives might be created.
+Similar to rcu_dereference_protected, The RCU list and hlist traversal
+primitives also check for whether there are called from within a reader
+section. However, an optional lockdep expression can be passed to them as
+the last argument in case they are called under other non-RCU protection.
+
+For example, the workqueue for_each_pwq() macro is implemented as follows.
+It is safe to call for_each_pwq() outside a reader section but under protection
+of wq->mutex:
+#define for_each_pwq(pwq, wq)
+	list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node,
+				lock_is_held(&(wq->mutex).dep_map))
diff --git a/Documentation/RCU/whatisRCU.txt b/Documentation/RCU/whatisRCU.txt
index 7e1a8721637a..00fe77ede1e2 100644
--- a/Documentation/RCU/whatisRCU.txt
+++ b/Documentation/RCU/whatisRCU.txt
@@ -290,7 +290,7 @@ rcu_dereference()
 	at any time, including immediately after the rcu_dereference().
 	And, again like rcu_assign_pointer(), rcu_dereference() is
 	typically used indirectly, via the _rcu list-manipulation
-	primitives, such as list_for_each_entry_rcu().
+	primitives, such as list_for_each_entry_rcu() [2].
 
 	[1] The variant rcu_dereference_protected() can be used outside
 	of an RCU read-side critical section as long as the usage is
@@ -305,6 +305,13 @@ rcu_dereference()
 	a lockdep splat is emitted.  See RCU/Design/Requirements/Requirements.html
 	and the API's code comments for more details and example usage.
 
+	[2] In case the list_for_each_entry_rcu() primitive is intended
+	to be used outside of an RCU reader section such as when
+	protected by a lock, then an additional lockdep expression can be
+	passed as the last argument to it so that RCU lockdep checking code
+	knows that the dereference of the list pointers are safe. If the
+	indicated protection is not provided, a lockdep splat is emitted.
+
 The following diagram shows how each API communicates among the
 reader, updater, and reclaimer.
 
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
  2019-07-15 14:37 ` [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1) Joel Fernandes (Google)
@ 2019-07-15 20:02   ` Bjorn Helgaas
  2019-07-16  4:03     ` Joel Fernandes
  0 siblings, 1 reply; 26+ messages in thread
From: Bjorn Helgaas @ 2019-07-15 20:02 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Alexey Kuznetsov, Borislav Petkov, c0d1n61at3,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
	keescook, kernel-hardening, kernel-team, Lai Jiangshan,
	Len Brown, linux-acpi, linux-doc, linux-pci, linux-pm,
	Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jul 15, 2019 at 10:37:03AM -0400, Joel Fernandes (Google) wrote:
> The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
> reader-lock held, because the pci_mmcfg_lock is already held. Make this
> known to the list macro so that it fixes new lockdep warnings that
> trigger due to lockdep checks added to list_for_each_entry_rcu().
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Ingo takes care of most patches to this file, but FWIW,

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I would personally prefer if you capitalized the subject to match the
"x86/PCI:" convention that's used fairly consistently in
arch/x86/pci/.

Also, I didn't apply this to be sure, but it looks like this might
make a line or two wider than 80 columns, which I would rewrap if I
were applying this.

> ---
>  arch/x86/pci/mmconfig-shared.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 7389db538c30..6fa42e9c4e6f 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -29,6 +29,7 @@
>  static bool pci_mmcfg_running_state;
>  static bool pci_mmcfg_arch_init_failed;
>  static DEFINE_MUTEX(pci_mmcfg_lock);
> +#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map)
>  
>  LIST_HEAD(pci_mmcfg_list);
>  
> @@ -54,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
>  	struct pci_mmcfg_region *cfg;
>  
>  	/* keep list sorted by segment and starting bus number */
> -	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
> +	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) {
>  		if (cfg->segment > new->segment ||
>  		    (cfg->segment == new->segment &&
>  		     cfg->start_bus >= new->start_bus)) {
> @@ -118,7 +119,7 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
>  {
>  	struct pci_mmcfg_region *cfg;
>  
> -	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
> +	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held())
>  		if (cfg->segment == segment &&
>  		    cfg->start_bus <= bus && bus <= cfg->end_bus)
>  			return cfg;
> -- 
> 2.22.0.510.g264f2c817a-goog
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1)
  2019-07-15 14:37 ` [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1) Joel Fernandes (Google)
@ 2019-07-15 21:44   ` Rafael J. Wysocki
  2019-07-16 18:43   ` Paul E. McKenney
  1 sibling, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2019-07-15 21:44 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: Linux Kernel Mailing List, Alexey Kuznetsov, Bjorn Helgaas,
	Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
	Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
	Ingo Molnar, Jonathan Corbet, Josh Triplett, Kees Cook,
	Kernel Hardening, Cc: Android Kernel, Lai Jiangshan, Len Brown,
	ACPI Devel Maling List, open list:DOCUMENTATION, Linux PCI,
	Linux PM, Mathieu Desnoyers, NeilBrown, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, Peter Zijlstra,
	Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
	Tejun Heo, Thomas Gleixner, Will Deacon,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jul 15, 2019 at 4:43 PM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
>
> list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
> it for acpi_ioremaps list traversal.
>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/osl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 9c0edf2fc0dd..2f9d0d20b836 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -14,6 +14,7 @@
>  #include <linux/slab.h>
>  #include <linux/mm.h>
>  #include <linux/highmem.h>
> +#include <linux/lockdep.h>
>  #include <linux/pci.h>
>  #include <linux/interrupt.h>
>  #include <linux/kmod.h>
> @@ -80,6 +81,7 @@ struct acpi_ioremap {
>
>  static LIST_HEAD(acpi_ioremaps);
>  static DEFINE_MUTEX(acpi_ioremap_lock);
> +#define acpi_ioremap_lock_held() lock_is_held(&acpi_ioremap_lock.dep_map)
>
>  static void __init acpi_request_region (struct acpi_generic_address *gas,
>         unsigned int length, char *desc)
> @@ -206,7 +208,7 @@ acpi_map_lookup(acpi_physical_address phys, acpi_size size)
>  {
>         struct acpi_ioremap *map;
>
> -       list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> +       list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
>                 if (map->phys <= phys &&
>                     phys + size <= map->phys + map->size)
>                         return map;
> @@ -249,7 +251,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
>  {
>         struct acpi_ioremap *map;
>
> -       list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> +       list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
>                 if (map->virt <= virt &&
>                     virt + size <= map->virt + map->size)
>                         return map;
> --
> 2.22.0.510.g264f2c817a-goog
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
  2019-07-15 20:02   ` Bjorn Helgaas
@ 2019-07-16  4:03     ` Joel Fernandes
  2019-07-16 18:42       ` Paul E. McKenney
  0 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes @ 2019-07-16  4:03 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: linux-kernel, Alexey Kuznetsov, Borislav Petkov, c0d1n61at3,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
	keescook, kernel-hardening, kernel-team, Lai Jiangshan,
	Len Brown, linux-acpi, linux-doc, linux-pci, linux-pm,
	Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jul 15, 2019 at 03:02:35PM -0500, Bjorn Helgaas wrote:
> On Mon, Jul 15, 2019 at 10:37:03AM -0400, Joel Fernandes (Google) wrote:
> > The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
> > reader-lock held, because the pci_mmcfg_lock is already held. Make this
> > known to the list macro so that it fixes new lockdep warnings that
> > trigger due to lockdep checks added to list_for_each_entry_rcu().
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> Ingo takes care of most patches to this file, but FWIW,
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks.

> I would personally prefer if you capitalized the subject to match the
> "x86/PCI:" convention that's used fairly consistently in
> arch/x86/pci/.
> 
> Also, I didn't apply this to be sure, but it looks like this might
> make a line or two wider than 80 columns, which I would rewrap if I
> were applying this.

Updated below is the patch with the nits corrected:

---8<-----------------------

From 73fab09d7e33ca2110c24215f8ed428c12625dbe Mon Sep 17 00:00:00 2001
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Date: Sat, 1 Jun 2019 15:05:49 -0400
Subject: [PATCH] x86/PCI: Pass lockdep condition to pcm_mmcfg_list iterator
 (v1)

The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
reader-lock held, because the pci_mmcfg_lock is already held. Make this
known to the list macro so that it fixes new lockdep warnings that
trigger due to lockdep checks added to list_for_each_entry_rcu().

Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 arch/x86/pci/mmconfig-shared.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 7389db538c30..9e3250ec5a37 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -29,6 +29,7 @@
 static bool pci_mmcfg_running_state;
 static bool pci_mmcfg_arch_init_failed;
 static DEFINE_MUTEX(pci_mmcfg_lock);
+#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map)
 
 LIST_HEAD(pci_mmcfg_list);
 
@@ -54,7 +55,8 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
 	struct pci_mmcfg_region *cfg;
 
 	/* keep list sorted by segment and starting bus number */
-	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
+	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list,
+				pci_mmcfg_lock_held()) {
 		if (cfg->segment > new->segment ||
 		    (cfg->segment == new->segment &&
 		     cfg->start_bus >= new->start_bus)) {
@@ -118,7 +120,8 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
 {
 	struct pci_mmcfg_region *cfg;
 
-	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
+	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list
+				pci_mmcfg_lock_held())
 		if (cfg->segment == segment &&
 		    cfg->start_bus <= bus && bus <= cfg->end_bus)
 			return cfg;
-- 
2.22.0.510.g264f2c817a-goog


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
  2019-07-15 14:36 ` [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3) Joel Fernandes (Google)
@ 2019-07-16 18:38   ` Paul E. McKenney
  2019-07-16 18:46     ` Joel Fernandes
  0 siblings, 1 reply; 26+ messages in thread
From: Paul E. McKenney @ 2019-07-16 18:38 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jul 15, 2019 at 10:36:58AM -0400, Joel Fernandes (Google) wrote:
> This patch adds support for checking RCU reader sections in list
> traversal macros. Optionally, if the list macro is called under SRCU or
> other lock/mutex protection, then appropriate lockdep expressions can be
> passed to make the checks pass.
> 
> Existing list_for_each_entry_rcu() invocations don't need to pass the
> optional fourth argument (cond) unless they are under some non-RCU
> protection and needs to make lockdep check pass.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Now that I am on the correct version, again please fold in the checks
for the extra argument.  The ability to have an optional argument looks
quite helpful, especially when compared to growing the RCU API!

A few more things below.

> ---
>  include/linux/rculist.h  | 28 ++++++++++++++++++++-----
>  include/linux/rcupdate.h |  7 +++++++
>  kernel/rcu/Kconfig.debug | 11 ++++++++++
>  kernel/rcu/update.c      | 44 ++++++++++++++++++++++++----------------
>  4 files changed, 67 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index e91ec9ddcd30..1048160625bb 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
>   */
>  #define list_next_rcu(list)	(*((struct list_head __rcu **)(&(list)->next)))
>  
> +/*
> + * Check during list traversal that we are within an RCU reader
> + */
> +
> +#ifdef CONFIG_PROVE_RCU_LIST

This new Kconfig option is OK temporarily, but unless there is reason to
fear malfunction that a few weeks of rcutorture, 0day, and -next won't
find, it would be better to just use CONFIG_PROVE_RCU.  The overall goal
is to reduce the number of RCU knobs rather than grow them, must though
history might lead one to believe otherwise.  :-/

> +#define __list_check_rcu(dummy, cond, ...)				\
> +	({								\
> +	RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),		\
> +			 "RCU-list traversed in non-reader section!");	\
> +	 })
> +#else
> +#define __list_check_rcu(dummy, cond, ...) ({})
> +#endif
> +
>  /*
>   * Insert a new entry between two known consecutive entries.
>   *
> @@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
>   * @pos:	the type * to use as a loop cursor.
>   * @head:	the head for your list.
>   * @member:	the name of the list_head within the struct.
> + * @cond:	optional lockdep expression if called from non-RCU protection.
>   *
>   * This list-traversal primitive may safely run concurrently with
>   * the _rcu list-mutation primitives such as list_add_rcu()
>   * as long as the traversal is guarded by rcu_read_lock().
>   */
> -#define list_for_each_entry_rcu(pos, head, member) \
> -	for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> -		&pos->member != (head); \
> +#define list_for_each_entry_rcu(pos, head, member, cond...)		\
> +	for (__list_check_rcu(dummy, ## cond, 0),			\
> +	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> +		&pos->member != (head);					\
>  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
>  
>  /**
> @@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
>   * @pos:	the type * to use as a loop cursor.
>   * @head:	the head for your list.
>   * @member:	the name of the hlist_node within the struct.
> + * @cond:	optional lockdep expression if called from non-RCU protection.
>   *
>   * This list-traversal primitive may safely run concurrently with
>   * the _rcu list-mutation primitives such as hlist_add_head_rcu()
>   * as long as the traversal is guarded by rcu_read_lock().
>   */
> -#define hlist_for_each_entry_rcu(pos, head, member)			\
> -	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> +#define hlist_for_each_entry_rcu(pos, head, member, cond...)		\
> +	for (__list_check_rcu(dummy, ## cond, 0),			\
> +	     pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
>  			typeof(*(pos)), member);			\
>  		pos;							\
>  		pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 8f7167478c1d..f3c29efdf19a 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -221,6 +221,7 @@ int debug_lockdep_rcu_enabled(void);
>  int rcu_read_lock_held(void);
>  int rcu_read_lock_bh_held(void);
>  int rcu_read_lock_sched_held(void);
> +int rcu_read_lock_any_held(void);
>  
>  #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>  
> @@ -241,6 +242,12 @@ static inline int rcu_read_lock_sched_held(void)
>  {
>  	return !preemptible();
>  }
> +
> +static inline int rcu_read_lock_any_held(void)
> +{
> +	return !preemptible();
> +}
> +
>  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>  
>  #ifdef CONFIG_PROVE_RCU
> diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> index 5ec3ea4028e2..7fbd21dbfcd0 100644
> --- a/kernel/rcu/Kconfig.debug
> +++ b/kernel/rcu/Kconfig.debug
> @@ -8,6 +8,17 @@ menu "RCU Debugging"
>  config PROVE_RCU
>  	def_bool PROVE_LOCKING
>  
> +config PROVE_RCU_LIST
> +	bool "RCU list lockdep debugging"
> +	depends on PROVE_RCU

This must also depend on RCU_EXPERT.  

> +	default n
> +	help
> +	  Enable RCU lockdep checking for list usages. By default it is
> +	  turned off since there are several list RCU users that still
> +	  need to be converted to pass a lockdep expression. To prevent
> +	  false-positive splats, we keep it default disabled but once all
> +	  users are converted, we can remove this config option.
> +
>  config TORTURE_TEST
>  	tristate
>  	default n
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index 9dd5aeef6e70..b7a4e3b5fa98 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -91,14 +91,18 @@ module_param(rcu_normal_after_boot, int, 0);
>   * Similarly, we avoid claiming an SRCU read lock held if the current
>   * CPU is offline.
>   */
> +#define rcu_read_lock_held_common()		\
> +	if (!debug_lockdep_rcu_enabled())	\
> +		return 1;			\
> +	if (!rcu_is_watching())			\
> +		return 0;			\
> +	if (!rcu_lockdep_current_cpu_online())	\
> +		return 0;

Nice abstraction of common code!

							Thanx, Paul

> +
>  int rcu_read_lock_sched_held(void)
>  {
> -	if (!debug_lockdep_rcu_enabled())
> -		return 1;
> -	if (!rcu_is_watching())
> -		return 0;
> -	if (!rcu_lockdep_current_cpu_online())
> -		return 0;
> +	rcu_read_lock_held_common();
> +
>  	return lock_is_held(&rcu_sched_lock_map) || !preemptible();
>  }
>  EXPORT_SYMBOL(rcu_read_lock_sched_held);
> @@ -257,12 +261,8 @@ NOKPROBE_SYMBOL(debug_lockdep_rcu_enabled);
>   */
>  int rcu_read_lock_held(void)
>  {
> -	if (!debug_lockdep_rcu_enabled())
> -		return 1;
> -	if (!rcu_is_watching())
> -		return 0;
> -	if (!rcu_lockdep_current_cpu_online())
> -		return 0;
> +	rcu_read_lock_held_common();
> +
>  	return lock_is_held(&rcu_lock_map);
>  }
>  EXPORT_SYMBOL_GPL(rcu_read_lock_held);
> @@ -284,16 +284,24 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_held);
>   */
>  int rcu_read_lock_bh_held(void)
>  {
> -	if (!debug_lockdep_rcu_enabled())
> -		return 1;
> -	if (!rcu_is_watching())
> -		return 0;
> -	if (!rcu_lockdep_current_cpu_online())
> -		return 0;
> +	rcu_read_lock_held_common();
> +
>  	return in_softirq() || irqs_disabled();
>  }
>  EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
>  
> +int rcu_read_lock_any_held(void)
> +{
> +	rcu_read_lock_held_common();
> +
> +	if (lock_is_held(&rcu_lock_map) ||
> +	    lock_is_held(&rcu_bh_lock_map) ||
> +	    lock_is_held(&rcu_sched_lock_map))
> +		return 1;
> +	return !preemptible();
> +}
> +EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
> +
>  #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>  
>  /**
> -- 
> 2.22.0.510.g264f2c817a-goog
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/9] rcu/sync: Remove custom check for reader-section (v2)
  2019-07-15 14:36 ` [PATCH 3/9] rcu/sync: Remove custom check for reader-section (v2) Joel Fernandes (Google)
@ 2019-07-16 18:39   ` Paul E. McKenney
  0 siblings, 0 replies; 26+ messages in thread
From: Paul E. McKenney @ 2019-07-16 18:39 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Oleg Nesterov, Alexey Kuznetsov, Bjorn Helgaas,
	Borislav Petkov, c0d1n61at3, David S. Miller, edumazet,
	Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
	Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
	kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
	linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
	neilb, netdev, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jul 15, 2019 at 10:36:59AM -0400, Joel Fernandes (Google) wrote:
> The rcu/sync code was doing its own check whether we are in a reader
> section. With RCU consolidating flavors and the generic helper added in
> this series, this is no longer need. We can just use the generic helper
> and it results in a nice cleanup.
> 
> Cc: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

This one looks good!

							Thanx, Paul

> ---
>  include/linux/rcu_sync.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> index 9b83865d24f9..0027d4c8087c 100644
> --- a/include/linux/rcu_sync.h
> +++ b/include/linux/rcu_sync.h
> @@ -31,9 +31,7 @@ struct rcu_sync {
>   */
>  static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
>  {
> -	RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&
> -			 !rcu_read_lock_bh_held() &&
> -			 !rcu_read_lock_sched_held(),
> +	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),
>  			 "suspicious rcu_sync_is_idle() usage");
>  	return !READ_ONCE(rsp->gp_state); /* GP_IDLE */
>  }
> -- 
> 2.22.0.510.g264f2c817a-goog
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/9] ipv4: add lockdep condition to fix for_each_entry (v1)
  2019-07-15 14:37 ` [PATCH 4/9] ipv4: add lockdep condition to fix for_each_entry (v1) Joel Fernandes (Google)
@ 2019-07-16 18:39   ` Paul E. McKenney
  2019-07-16 21:12     ` David Miller
  0 siblings, 1 reply; 26+ messages in thread
From: Paul E. McKenney @ 2019-07-16 18:39 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jul 15, 2019 at 10:37:00AM -0400, Joel Fernandes (Google) wrote:
> Using the previous support added, use it for adding lockdep conditions
> to list usage here.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

We need an ack or better from the subsystem maintainer for this one.

						Thanx, Paul

> ---
>  net/ipv4/fib_frontend.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 317339cd7f03..26b0fb24e2c2 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -124,7 +124,8 @@ struct fib_table *fib_get_table(struct net *net, u32 id)
>  	h = id & (FIB_TABLE_HASHSZ - 1);
>  
>  	head = &net->ipv4.fib_table_hash[h];
> -	hlist_for_each_entry_rcu(tb, head, tb_hlist) {
> +	hlist_for_each_entry_rcu(tb, head, tb_hlist,
> +				 lockdep_rtnl_is_held()) {
>  		if (tb->tb_id == id)
>  			return tb;
>  	}
> -- 
> 2.22.0.510.g264f2c817a-goog
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 5/9] driver/core: Convert to use built-in RCU list checking (v1)
  2019-07-15 14:37 ` [PATCH 5/9] driver/core: Convert to use built-in RCU list checking (v1) Joel Fernandes (Google)
@ 2019-07-16 18:40   ` Paul E. McKenney
  0 siblings, 0 replies; 26+ messages in thread
From: Paul E. McKenney @ 2019-07-16 18:40 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Greg Kroah-Hartman, Alexey Kuznetsov,
	Bjorn Helgaas, Borislav Petkov, c0d1n61at3, David S. Miller,
	edumazet, Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar,
	Jonathan Corbet, Josh Triplett, keescook, kernel-hardening,
	kernel-team, Lai Jiangshan, Len Brown, linux-acpi, linux-doc,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev,
	Oleg Nesterov, Pavel Machek, peterz, Rafael J. Wysocki,
	Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jul 15, 2019 at 10:37:01AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
> it in driver core.
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

This one looks ready.

							Thanx, Paul

> ---
>  drivers/base/base.h          |  1 +
>  drivers/base/core.c          | 10 ++++++++++
>  drivers/base/power/runtime.c | 15 ++++++++++-----
>  3 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index b405436ee28e..0d32544b6f91 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -165,6 +165,7 @@ static inline int devtmpfs_init(void) { return 0; }
>  /* Device links support */
>  extern int device_links_read_lock(void);
>  extern void device_links_read_unlock(int idx);
> +extern int device_links_read_lock_held(void);
>  extern int device_links_check_suppliers(struct device *dev);
>  extern void device_links_driver_bound(struct device *dev);
>  extern void device_links_driver_cleanup(struct device *dev);
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index da84a73f2ba6..85e82f38717f 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -68,6 +68,11 @@ void device_links_read_unlock(int idx)
>  {
>  	srcu_read_unlock(&device_links_srcu, idx);
>  }
> +
> +int device_links_read_lock_held(void)
> +{
> +	return srcu_read_lock_held(&device_links_srcu);
> +}
>  #else /* !CONFIG_SRCU */
>  static DECLARE_RWSEM(device_links_lock);
>  
> @@ -91,6 +96,11 @@ void device_links_read_unlock(int not_used)
>  {
>  	up_read(&device_links_lock);
>  }
> +
> +int device_links_read_lock_held(void)
> +{
> +	return lock_is_held(&device_links_lock);
> +}
>  #endif /* !CONFIG_SRCU */
>  
>  /**
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 952a1e7057c7..7a10e8379a70 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -287,7 +287,8 @@ static int rpm_get_suppliers(struct device *dev)
>  {
>  	struct device_link *link;
>  
> -	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
> +	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
> +				device_links_read_lock_held()) {
>  		int retval;
>  
>  		if (!(link->flags & DL_FLAG_PM_RUNTIME) ||
> @@ -309,7 +310,8 @@ static void rpm_put_suppliers(struct device *dev)
>  {
>  	struct device_link *link;
>  
> -	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node) {
> +	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
> +				device_links_read_lock_held()) {
>  		if (READ_ONCE(link->status) == DL_STATE_SUPPLIER_UNBIND)
>  			continue;
>  
> @@ -1640,7 +1642,8 @@ void pm_runtime_clean_up_links(struct device *dev)
>  
>  	idx = device_links_read_lock();
>  
> -	list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
> +	list_for_each_entry_rcu(link, &dev->links.consumers, s_node,
> +				device_links_read_lock_held()) {
>  		if (link->flags & DL_FLAG_STATELESS)
>  			continue;
>  
> @@ -1662,7 +1665,8 @@ void pm_runtime_get_suppliers(struct device *dev)
>  
>  	idx = device_links_read_lock();
>  
> -	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
> +	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
> +				device_links_read_lock_held())
>  		if (link->flags & DL_FLAG_PM_RUNTIME) {
>  			link->supplier_preactivated = true;
>  			refcount_inc(&link->rpm_active);
> @@ -1683,7 +1687,8 @@ void pm_runtime_put_suppliers(struct device *dev)
>  
>  	idx = device_links_read_lock();
>  
> -	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
> +	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node,
> +				device_links_read_lock_held())
>  		if (link->supplier_preactivated) {
>  			link->supplier_preactivated = false;
>  			if (refcount_dec_not_one(&link->rpm_active))
> -- 
> 2.22.0.510.g264f2c817a-goog
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 6/9] workqueue: Convert for_each_wq to use built-in list check (v2)
  2019-07-15 14:37 ` [PATCH 6/9] workqueue: Convert for_each_wq to use built-in list check (v2) Joel Fernandes (Google)
@ 2019-07-16 18:41   ` Paul E. McKenney
  0 siblings, 0 replies; 26+ messages in thread
From: Paul E. McKenney @ 2019-07-16 18:41 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jul 15, 2019 at 10:37:02AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu now has support to check for RCU reader sections
> as well as lock. Just use the support in it, instead of explictly
> checking in the caller.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

We need an ack from one of the subsystem maintainers on this one.

							Thanx, Paul

> ---
>  kernel/workqueue.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 601d61150b65..e882477ebf6e 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -364,11 +364,6 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
>  			 !lockdep_is_held(&wq_pool_mutex),		\
>  			 "RCU or wq_pool_mutex should be held")
>  
> -#define assert_rcu_or_wq_mutex(wq)					\
> -	RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&			\
> -			 !lockdep_is_held(&wq->mutex),			\
> -			 "RCU or wq->mutex should be held")
> -
>  #define assert_rcu_or_wq_mutex_or_pool_mutex(wq)			\
>  	RCU_LOCKDEP_WARN(!rcu_read_lock_held() &&			\
>  			 !lockdep_is_held(&wq->mutex) &&		\
> @@ -425,9 +420,8 @@ static void workqueue_sysfs_unregister(struct workqueue_struct *wq);
>   * ignored.
>   */
>  #define for_each_pwq(pwq, wq)						\
> -	list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node)		\
> -		if (({ assert_rcu_or_wq_mutex(wq); false; })) { }	\
> -		else
> +	list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node,		\
> +				 lock_is_held(&(wq->mutex).dep_map))
>  
>  #ifdef CONFIG_DEBUG_OBJECTS_WORK
>  
> -- 
> 2.22.0.510.g264f2c817a-goog
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
  2019-07-16  4:03     ` Joel Fernandes
@ 2019-07-16 18:42       ` Paul E. McKenney
  0 siblings, 0 replies; 26+ messages in thread
From: Paul E. McKenney @ 2019-07-16 18:42 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Bjorn Helgaas, linux-kernel, Alexey Kuznetsov, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Tue, Jul 16, 2019 at 12:03:03AM -0400, Joel Fernandes wrote:
> On Mon, Jul 15, 2019 at 03:02:35PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jul 15, 2019 at 10:37:03AM -0400, Joel Fernandes (Google) wrote:
> > > The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
> > > reader-lock held, because the pci_mmcfg_lock is already held. Make this
> > > known to the list macro so that it fixes new lockdep warnings that
> > > trigger due to lockdep checks added to list_for_each_entry_rcu().
> > > 
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > 
> > Ingo takes care of most patches to this file, but FWIW,
> > 
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> Thanks.
> 
> > I would personally prefer if you capitalized the subject to match the
> > "x86/PCI:" convention that's used fairly consistently in
> > arch/x86/pci/.
> > 
> > Also, I didn't apply this to be sure, but it looks like this might
> > make a line or two wider than 80 columns, which I would rewrap if I
> > were applying this.
> 
> Updated below is the patch with the nits corrected:

I am OK with this going either way, but it does depend on an earlier
patch.

							Thanx, Paul

> ---8<-----------------------
> 
> >From 73fab09d7e33ca2110c24215f8ed428c12625dbe Mon Sep 17 00:00:00 2001
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Date: Sat, 1 Jun 2019 15:05:49 -0400
> Subject: [PATCH] x86/PCI: Pass lockdep condition to pcm_mmcfg_list iterator
>  (v1)
> 
> The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
> reader-lock held, because the pci_mmcfg_lock is already held. Make this
> known to the list macro so that it fixes new lockdep warnings that
> trigger due to lockdep checks added to list_for_each_entry_rcu().
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  arch/x86/pci/mmconfig-shared.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 7389db538c30..9e3250ec5a37 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -29,6 +29,7 @@
>  static bool pci_mmcfg_running_state;
>  static bool pci_mmcfg_arch_init_failed;
>  static DEFINE_MUTEX(pci_mmcfg_lock);
> +#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map)
>  
>  LIST_HEAD(pci_mmcfg_list);
>  
> @@ -54,7 +55,8 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
>  	struct pci_mmcfg_region *cfg;
>  
>  	/* keep list sorted by segment and starting bus number */
> -	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
> +	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list,
> +				pci_mmcfg_lock_held()) {
>  		if (cfg->segment > new->segment ||
>  		    (cfg->segment == new->segment &&
>  		     cfg->start_bus >= new->start_bus)) {
> @@ -118,7 +120,8 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
>  {
>  	struct pci_mmcfg_region *cfg;
>  
> -	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
> +	list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list
> +				pci_mmcfg_lock_held())
>  		if (cfg->segment == segment &&
>  		    cfg->start_bus <= bus && bus <= cfg->end_bus)
>  			return cfg;
> -- 
> 2.22.0.510.g264f2c817a-goog
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1)
  2019-07-15 14:37 ` [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1) Joel Fernandes (Google)
  2019-07-15 21:44   ` Rafael J. Wysocki
@ 2019-07-16 18:43   ` Paul E. McKenney
  1 sibling, 0 replies; 26+ messages in thread
From: Paul E. McKenney @ 2019-07-16 18:43 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jul 15, 2019 at 10:37:04AM -0400, Joel Fernandes (Google) wrote:
> list_for_each_entry_rcu has built-in RCU and lock checking. Make use of
> it for acpi_ioremaps list traversal.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Given that Rafael acked it, this one looks ready.

							Thanx, Paul

> ---
>  drivers/acpi/osl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 9c0edf2fc0dd..2f9d0d20b836 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -14,6 +14,7 @@
>  #include <linux/slab.h>
>  #include <linux/mm.h>
>  #include <linux/highmem.h>
> +#include <linux/lockdep.h>
>  #include <linux/pci.h>
>  #include <linux/interrupt.h>
>  #include <linux/kmod.h>
> @@ -80,6 +81,7 @@ struct acpi_ioremap {
>  
>  static LIST_HEAD(acpi_ioremaps);
>  static DEFINE_MUTEX(acpi_ioremap_lock);
> +#define acpi_ioremap_lock_held() lock_is_held(&acpi_ioremap_lock.dep_map)
>  
>  static void __init acpi_request_region (struct acpi_generic_address *gas,
>  	unsigned int length, char *desc)
> @@ -206,7 +208,7 @@ acpi_map_lookup(acpi_physical_address phys, acpi_size size)
>  {
>  	struct acpi_ioremap *map;
>  
> -	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> +	list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
>  		if (map->phys <= phys &&
>  		    phys + size <= map->phys + map->size)
>  			return map;
> @@ -249,7 +251,7 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size size)
>  {
>  	struct acpi_ioremap *map;
>  
> -	list_for_each_entry_rcu(map, &acpi_ioremaps, list)
> +	list_for_each_entry_rcu(map, &acpi_ioremaps, list, acpi_ioremap_lock_held())
>  		if (map->virt <= virt &&
>  		    virt + size <= map->virt + map->size)
>  			return map;
> -- 
> 2.22.0.510.g264f2c817a-goog
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
  2019-07-16 18:38   ` Paul E. McKenney
@ 2019-07-16 18:46     ` Joel Fernandes
  2019-07-16 18:53       ` Paul E. McKenney
  0 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes @ 2019-07-16 18:46 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Tue, Jul 16, 2019 at 11:38:33AM -0700, Paul E. McKenney wrote:
> On Mon, Jul 15, 2019 at 10:36:58AM -0400, Joel Fernandes (Google) wrote:
> > This patch adds support for checking RCU reader sections in list
> > traversal macros. Optionally, if the list macro is called under SRCU or
> > other lock/mutex protection, then appropriate lockdep expressions can be
> > passed to make the checks pass.
> > 
> > Existing list_for_each_entry_rcu() invocations don't need to pass the
> > optional fourth argument (cond) unless they are under some non-RCU
> > protection and needs to make lockdep check pass.
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> Now that I am on the correct version, again please fold in the checks
> for the extra argument.  The ability to have an optional argument looks
> quite helpful, especially when compared to growing the RCU API!

I did fold this and replied with a pull request URL based on /dev branch. But
we can hold off on the pull requests until we decide on the below comments:

> A few more things below.
> > ---
> >  include/linux/rculist.h  | 28 ++++++++++++++++++++-----
> >  include/linux/rcupdate.h |  7 +++++++
> >  kernel/rcu/Kconfig.debug | 11 ++++++++++
> >  kernel/rcu/update.c      | 44 ++++++++++++++++++++++++----------------
> >  4 files changed, 67 insertions(+), 23 deletions(-)
> > 
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index e91ec9ddcd30..1048160625bb 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> >   */
> >  #define list_next_rcu(list)	(*((struct list_head __rcu **)(&(list)->next)))
> >  
> > +/*
> > + * Check during list traversal that we are within an RCU reader
> > + */
> > +
> > +#ifdef CONFIG_PROVE_RCU_LIST
> 
> This new Kconfig option is OK temporarily, but unless there is reason to
> fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> find, it would be better to just use CONFIG_PROVE_RCU.  The overall goal
> is to reduce the number of RCU knobs rather than grow them, must though
> history might lead one to believe otherwise.  :-/

If you want, we can try to drop this option and just use PROVE_RCU however I
must say there may be several warnings that need to be fixed in a short
period of time (even a few weeks may be too short) considering the 1000+
uses of RCU lists.

But I don't mind dropping it and it may just accelerate the fixing up of all
callers.

> > +#define __list_check_rcu(dummy, cond, ...)				\
> > +	({								\
> > +	RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),		\
> > +			 "RCU-list traversed in non-reader section!");	\
> > +	 })
> > +#else
> > +#define __list_check_rcu(dummy, cond, ...) ({})
> > +#endif
> > +
> >  /*
> >   * Insert a new entry between two known consecutive entries.
> >   *
> > @@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> >   * @pos:	the type * to use as a loop cursor.
> >   * @head:	the head for your list.
> >   * @member:	the name of the list_head within the struct.
> > + * @cond:	optional lockdep expression if called from non-RCU protection.
> >   *
> >   * This list-traversal primitive may safely run concurrently with
> >   * the _rcu list-mutation primitives such as list_add_rcu()
> >   * as long as the traversal is guarded by rcu_read_lock().
> >   */
> > -#define list_for_each_entry_rcu(pos, head, member) \
> > -	for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> > -		&pos->member != (head); \
> > +#define list_for_each_entry_rcu(pos, head, member, cond...)		\
> > +	for (__list_check_rcu(dummy, ## cond, 0),			\
> > +	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> > +		&pos->member != (head);					\
> >  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> >  
> >  /**
> > @@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> >   * @pos:	the type * to use as a loop cursor.
> >   * @head:	the head for your list.
> >   * @member:	the name of the hlist_node within the struct.
> > + * @cond:	optional lockdep expression if called from non-RCU protection.
> >   *
> >   * This list-traversal primitive may safely run concurrently with
> >   * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> >   * as long as the traversal is guarded by rcu_read_lock().
> >   */
> > -#define hlist_for_each_entry_rcu(pos, head, member)			\
> > -	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > +#define hlist_for_each_entry_rcu(pos, head, member, cond...)		\
> > +	for (__list_check_rcu(dummy, ## cond, 0),			\
> > +	     pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> >  			typeof(*(pos)), member);			\
> >  		pos;							\
> >  		pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 8f7167478c1d..f3c29efdf19a 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -221,6 +221,7 @@ int debug_lockdep_rcu_enabled(void);
> >  int rcu_read_lock_held(void);
> >  int rcu_read_lock_bh_held(void);
> >  int rcu_read_lock_sched_held(void);
> > +int rcu_read_lock_any_held(void);
> >  
> >  #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> >  
> > @@ -241,6 +242,12 @@ static inline int rcu_read_lock_sched_held(void)
> >  {
> >  	return !preemptible();
> >  }
> > +
> > +static inline int rcu_read_lock_any_held(void)
> > +{
> > +	return !preemptible();
> > +}
> > +
> >  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> >  
> >  #ifdef CONFIG_PROVE_RCU
> > diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> > index 5ec3ea4028e2..7fbd21dbfcd0 100644
> > --- a/kernel/rcu/Kconfig.debug
> > +++ b/kernel/rcu/Kconfig.debug
> > @@ -8,6 +8,17 @@ menu "RCU Debugging"
> >  config PROVE_RCU
> >  	def_bool PROVE_LOCKING
> >  
> > +config PROVE_RCU_LIST
> > +	bool "RCU list lockdep debugging"
> > +	depends on PROVE_RCU
> 
> This must also depend on RCU_EXPERT.  

Sure.

> > +	default n
> > +	help
> > +	  Enable RCU lockdep checking for list usages. By default it is
> > +	  turned off since there are several list RCU users that still
> > +	  need to be converted to pass a lockdep expression. To prevent
> > +	  false-positive splats, we keep it default disabled but once all
> > +	  users are converted, we can remove this config option.
> > +
> >  config TORTURE_TEST
> >  	tristate
> >  	default n
> > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > index 9dd5aeef6e70..b7a4e3b5fa98 100644
> > --- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -91,14 +91,18 @@ module_param(rcu_normal_after_boot, int, 0);
> >   * Similarly, we avoid claiming an SRCU read lock held if the current
> >   * CPU is offline.
> >   */
> > +#define rcu_read_lock_held_common()		\
> > +	if (!debug_lockdep_rcu_enabled())	\
> > +		return 1;			\
> > +	if (!rcu_is_watching())			\
> > +		return 0;			\
> > +	if (!rcu_lockdep_current_cpu_online())	\
> > +		return 0;
> 
> Nice abstraction of common code!

Thanks!


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/9] Harden list_for_each_entry_rcu() and family
  2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
                   ` (8 preceding siblings ...)
  2019-07-15 14:37 ` [PATCH 9/9] doc: Update documentation about list_for_each_entry_rcu (v1) Joel Fernandes (Google)
@ 2019-07-16 18:46 ` Paul E. McKenney
  9 siblings, 0 replies; 26+ messages in thread
From: Paul E. McKenney @ 2019-07-16 18:46 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jul 15, 2019 at 10:36:56AM -0400, Joel Fernandes (Google) wrote:
> Hi,
> This series aims to provide lockdep checking to RCU list macros for additional
> kernel hardening.
> 
> RCU has a number of primitives for "consumption" of an RCU protected pointer.
> Most of the time, these consumers make sure that such accesses are under a RCU
> reader-section (such as rcu_dereference{,sched,bh} or under a lock, such as
> with rcu_dereference_protected()).
> 
> However, there are other ways to consume RCU pointers, such as by
> list_for_each_entry_rcu or hlist_for_each_enry_rcu. Unlike the rcu_dereference
> family, these consumers do no lockdep checking at all. And with the growing
> number of RCU list uses (1000+), it is possible for bugs to creep in and go
> unnoticed which lockdep checks can catch.
> 
> Since RCU consolidation efforts last year, the different traditional RCU
> flavors (preempt, bh, sched) are all consolidated. In other words, any of these
> flavors can cause a reader section to occur and all of them must cease before
> the reader section is considered to be unlocked. Thanks to this, we can
> generically check if we are in an RCU reader. This is what patch 1 does. Note
> that the list_for_each_entry_rcu and family are different from the
> rcu_dereference family in that, there is no _bh or _sched version of this
> macro. They are used under many different RCU reader flavors, and also SRCU.
> Patch 1 adds a new internal function rcu_read_lock_any_held() which checks
> if any reader section is active at all, when these macros are called. If no
> reader section exists, then the optional fourth argument to
> list_for_each_entry_rcu() can be a lockdep expression which is evaluated
> (similar to how rcu_dereference_check() works). If no lockdep expression is
> passed, and we are not in a reader, then a splat occurs. Just take off the
> lockdep expression after applying the patches, by using the following diff and
> see what happens:
> 
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -55,7 +55,7 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
>         struct pci_mmcfg_region *cfg;
> 
>         /* keep list sorted by segment and starting bus number */
> -       list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list, pci_mmcfg_lock_held()) {
> +       list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
> 
> 
> The optional argument trick to list_for_each_entry_rcu() can also be used in
> the future to possibly remove rcu_dereference_{,bh,sched}_protected() API and
> we can pass an optional lockdep expression to rcu_dereference() itself. Thus
> eliminating 3 more RCU APIs.
> 
> Note that some list macro wrappers already do their own lockdep checking in the
> caller side. These can be eliminated in favor of the built-in lockdep checking
> in the list macro that this series adds. For example, workqueue code has a
> assert_rcu_or_wq_mutex() function which is called in for_each_wq().  This
> series replaces that in favor of the built-in check.
> 
> Also in the future, we can extend these checks to list_entry_rcu() and other
> list macros as well, if needed.
> 
> Please note that I have kept this option default-disabled under a new config:
> CONFIG_PROVE_RCU_LIST. This is so that until all users are converted to pass
> the optional argument, we should keep the check disabled. There are about a
> 1000 or so users and it is not possible to pass in the optional lockdep
> expression in a single series since it is done on a case-by-case basis. I did
> convert a few users in this series itself.

I do like the optional argument as opposed to the traditional practice
of expanding the RCU API!  Good stuff!!!

Please resend incorporating the acks and the changes from feedback.
I will hold off on any patches not yet having their maintainer's ack,
but it is OK to include them in v4.  (I will just avoid applying them.)

The documentation patch needs a bit of wordsmithing, but I can do that.
Feel free to take another pass on it if you wish, though.

							Thanx, Paul

> v2->v3: Simplified rcu-sync logic after rebase (Paul)
> 	Added check for bh_map (Paul)
> 	Refactored out more of the common code (Joel)
> 	Added Oleg ack to rcu-sync patch.
> 
> v1->v2: Have assert_rcu_or_wq_mutex deleted (Daniel Jordan)
> 	Simplify rcu_read_lock_any_held()   (Peter Zijlstra)
> 	Simplified rcu-sync logic	    (Oleg Nesterov)
> 	Updated documentation and rculist comments.
> 	Added GregKH ack.
> 
> RFC->v1: 
> 	Simplify list checking macro (Rasmus Villemoes)
> 
> Joel Fernandes (Google) (9):
> rcu/update: Remove useless check for debug_locks (v1)
> rcu: Add support for consolidated-RCU reader checking (v3)
> rcu/sync: Remove custom check for reader-section (v2)
> ipv4: add lockdep condition to fix for_each_entry (v1)
> driver/core: Convert to use built-in RCU list checking (v1)
> workqueue: Convert for_each_wq to use built-in list check (v2)
> x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
> acpi: Use built-in RCU list checking for acpi_ioremaps list (v1)
> doc: Update documentation about list_for_each_entry_rcu (v1)
> 
> Documentation/RCU/lockdep.txt   | 15 ++++++++---
> Documentation/RCU/whatisRCU.txt |  9 ++++++-
> arch/x86/pci/mmconfig-shared.c  |  5 ++--
> drivers/acpi/osl.c              |  6 +++--
> drivers/base/base.h             |  1 +
> drivers/base/core.c             | 10 +++++++
> drivers/base/power/runtime.c    | 15 +++++++----
> include/linux/rcu_sync.h        |  4 +--
> include/linux/rculist.h         | 28 +++++++++++++++----
> include/linux/rcupdate.h        |  7 +++++
> kernel/rcu/Kconfig.debug        | 11 ++++++++
> kernel/rcu/update.c             | 48 ++++++++++++++++++---------------
> kernel/workqueue.c              | 10 ++-----
> net/ipv4/fib_frontend.c         |  3 ++-
> 14 files changed, 119 insertions(+), 53 deletions(-)
> 
> --
> 2.22.0.510.g264f2c817a-goog
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
  2019-07-16 18:46     ` Joel Fernandes
@ 2019-07-16 18:53       ` Paul E. McKenney
  2019-07-16 22:02         ` Joel Fernandes
  0 siblings, 1 reply; 26+ messages in thread
From: Paul E. McKenney @ 2019-07-16 18:53 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Tue, Jul 16, 2019 at 02:46:49PM -0400, Joel Fernandes wrote:
> On Tue, Jul 16, 2019 at 11:38:33AM -0700, Paul E. McKenney wrote:
> > On Mon, Jul 15, 2019 at 10:36:58AM -0400, Joel Fernandes (Google) wrote:
> > > This patch adds support for checking RCU reader sections in list
> > > traversal macros. Optionally, if the list macro is called under SRCU or
> > > other lock/mutex protection, then appropriate lockdep expressions can be
> > > passed to make the checks pass.
> > > 
> > > Existing list_for_each_entry_rcu() invocations don't need to pass the
> > > optional fourth argument (cond) unless they are under some non-RCU
> > > protection and needs to make lockdep check pass.
> > > 
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > 
> > Now that I am on the correct version, again please fold in the checks
> > for the extra argument.  The ability to have an optional argument looks
> > quite helpful, especially when compared to growing the RCU API!
> 
> I did fold this and replied with a pull request URL based on /dev branch. But
> we can hold off on the pull requests until we decide on the below comments:
> 
> > A few more things below.
> > > ---
> > >  include/linux/rculist.h  | 28 ++++++++++++++++++++-----
> > >  include/linux/rcupdate.h |  7 +++++++
> > >  kernel/rcu/Kconfig.debug | 11 ++++++++++
> > >  kernel/rcu/update.c      | 44 ++++++++++++++++++++++++----------------
> > >  4 files changed, 67 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > index e91ec9ddcd30..1048160625bb 100644
> > > --- a/include/linux/rculist.h
> > > +++ b/include/linux/rculist.h
> > > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > >   */
> > >  #define list_next_rcu(list)	(*((struct list_head __rcu **)(&(list)->next)))
> > >  
> > > +/*
> > > + * Check during list traversal that we are within an RCU reader
> > > + */
> > > +
> > > +#ifdef CONFIG_PROVE_RCU_LIST
> > 
> > This new Kconfig option is OK temporarily, but unless there is reason to
> > fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> > find, it would be better to just use CONFIG_PROVE_RCU.  The overall goal
> > is to reduce the number of RCU knobs rather than grow them, must though
> > history might lead one to believe otherwise.  :-/
> 
> If you want, we can try to drop this option and just use PROVE_RCU however I
> must say there may be several warnings that need to be fixed in a short
> period of time (even a few weeks may be too short) considering the 1000+
> uses of RCU lists.

Do many people other than me build with CONFIG_PROVE_RCU?  If so, then
that would be a good reason for a temporary CONFIG_PROVE_RCU_LIST,
as in going away in a release or two once the warnings get fixed.

> But I don't mind dropping it and it may just accelerate the fixing up of all
> callers.

I will let you decide based on the above question.  But if you have
CONFIG_PROVE_RCU_LIST, as noted below, it needs to depend on RCU_EXPERT.

							Thanx, Paul

> > > +#define __list_check_rcu(dummy, cond, ...)				\
> > > +	({								\
> > > +	RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),		\
> > > +			 "RCU-list traversed in non-reader section!");	\
> > > +	 })
> > > +#else
> > > +#define __list_check_rcu(dummy, cond, ...) ({})
> > > +#endif
> > > +
> > >  /*
> > >   * Insert a new entry between two known consecutive entries.
> > >   *
> > > @@ -343,14 +357,16 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> > >   * @pos:	the type * to use as a loop cursor.
> > >   * @head:	the head for your list.
> > >   * @member:	the name of the list_head within the struct.
> > > + * @cond:	optional lockdep expression if called from non-RCU protection.
> > >   *
> > >   * This list-traversal primitive may safely run concurrently with
> > >   * the _rcu list-mutation primitives such as list_add_rcu()
> > >   * as long as the traversal is guarded by rcu_read_lock().
> > >   */
> > > -#define list_for_each_entry_rcu(pos, head, member) \
> > > -	for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> > > -		&pos->member != (head); \
> > > +#define list_for_each_entry_rcu(pos, head, member, cond...)		\
> > > +	for (__list_check_rcu(dummy, ## cond, 0),			\
> > > +	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> > > +		&pos->member != (head);					\
> > >  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> > >  
> > >  /**
> > > @@ -616,13 +632,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> > >   * @pos:	the type * to use as a loop cursor.
> > >   * @head:	the head for your list.
> > >   * @member:	the name of the hlist_node within the struct.
> > > + * @cond:	optional lockdep expression if called from non-RCU protection.
> > >   *
> > >   * This list-traversal primitive may safely run concurrently with
> > >   * the _rcu list-mutation primitives such as hlist_add_head_rcu()
> > >   * as long as the traversal is guarded by rcu_read_lock().
> > >   */
> > > -#define hlist_for_each_entry_rcu(pos, head, member)			\
> > > -	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > > +#define hlist_for_each_entry_rcu(pos, head, member, cond...)		\
> > > +	for (__list_check_rcu(dummy, ## cond, 0),			\
> > > +	     pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > >  			typeof(*(pos)), member);			\
> > >  		pos;							\
> > >  		pos = hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(\
> > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > > index 8f7167478c1d..f3c29efdf19a 100644
> > > --- a/include/linux/rcupdate.h
> > > +++ b/include/linux/rcupdate.h
> > > @@ -221,6 +221,7 @@ int debug_lockdep_rcu_enabled(void);
> > >  int rcu_read_lock_held(void);
> > >  int rcu_read_lock_bh_held(void);
> > >  int rcu_read_lock_sched_held(void);
> > > +int rcu_read_lock_any_held(void);
> > >  
> > >  #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> > >  
> > > @@ -241,6 +242,12 @@ static inline int rcu_read_lock_sched_held(void)
> > >  {
> > >  	return !preemptible();
> > >  }
> > > +
> > > +static inline int rcu_read_lock_any_held(void)
> > > +{
> > > +	return !preemptible();
> > > +}
> > > +
> > >  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> > >  
> > >  #ifdef CONFIG_PROVE_RCU
> > > diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> > > index 5ec3ea4028e2..7fbd21dbfcd0 100644
> > > --- a/kernel/rcu/Kconfig.debug
> > > +++ b/kernel/rcu/Kconfig.debug
> > > @@ -8,6 +8,17 @@ menu "RCU Debugging"
> > >  config PROVE_RCU
> > >  	def_bool PROVE_LOCKING
> > >  
> > > +config PROVE_RCU_LIST
> > > +	bool "RCU list lockdep debugging"
> > > +	depends on PROVE_RCU
> > 
> > This must also depend on RCU_EXPERT.  
> 
> Sure.
> 
> > > +	default n
> > > +	help
> > > +	  Enable RCU lockdep checking for list usages. By default it is
> > > +	  turned off since there are several list RCU users that still
> > > +	  need to be converted to pass a lockdep expression. To prevent
> > > +	  false-positive splats, we keep it default disabled but once all
> > > +	  users are converted, we can remove this config option.
> > > +
> > >  config TORTURE_TEST
> > >  	tristate
> > >  	default n
> > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > index 9dd5aeef6e70..b7a4e3b5fa98 100644
> > > --- a/kernel/rcu/update.c
> > > +++ b/kernel/rcu/update.c
> > > @@ -91,14 +91,18 @@ module_param(rcu_normal_after_boot, int, 0);
> > >   * Similarly, we avoid claiming an SRCU read lock held if the current
> > >   * CPU is offline.
> > >   */
> > > +#define rcu_read_lock_held_common()		\
> > > +	if (!debug_lockdep_rcu_enabled())	\
> > > +		return 1;			\
> > > +	if (!rcu_is_watching())			\
> > > +		return 0;			\
> > > +	if (!rcu_lockdep_current_cpu_online())	\
> > > +		return 0;
> > 
> > Nice abstraction of common code!
> 
> Thanks!
> 


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/9] ipv4: add lockdep condition to fix for_each_entry (v1)
  2019-07-16 18:39   ` Paul E. McKenney
@ 2019-07-16 21:12     ` David Miller
  0 siblings, 0 replies; 26+ messages in thread
From: David Miller @ 2019-07-16 21:12 UTC (permalink / raw)
  To: paulmck
  Cc: joel, linux-kernel, kuznet, bhelgaas, bp, c0d1n61at3, edumazet,
	gregkh, yoshfuji, hpa, mingo, corbet, josh, keescook,
	kernel-hardening, kernel-team, jiangshanlai, lenb, linux-acpi,
	linux-doc, linux-pci, linux-pm, mathieu.desnoyers, neilb, netdev,
	oleg, pavel, peterz, rjw, rasmus.villemoes, rcu, rostedt, tj,
	tglx, will, x86

From: "Paul E. McKenney" <paulmck@linux.ibm.com>
Date: Tue, 16 Jul 2019 11:39:55 -0700

> On Mon, Jul 15, 2019 at 10:37:00AM -0400, Joel Fernandes (Google) wrote:
>> Using the previous support added, use it for adding lockdep conditions
>> to list usage here.
>> 
>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> We need an ack or better from the subsystem maintainer for this one.

Acked-by: David S. Miller <davem@davemloft.net>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
  2019-07-16 18:53       ` Paul E. McKenney
@ 2019-07-16 22:02         ` Joel Fernandes
  2019-07-17  0:07           ` Paul E. McKenney
  0 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes @ 2019-07-16 22:02 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Tue, Jul 16, 2019 at 11:53:03AM -0700, Paul E. McKenney wrote:
[snip]
> > > A few more things below.
> > > > ---
> > > >  include/linux/rculist.h  | 28 ++++++++++++++++++++-----
> > > >  include/linux/rcupdate.h |  7 +++++++
> > > >  kernel/rcu/Kconfig.debug | 11 ++++++++++
> > > >  kernel/rcu/update.c      | 44 ++++++++++++++++++++++++----------------
> > > >  4 files changed, 67 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > > index e91ec9ddcd30..1048160625bb 100644
> > > > --- a/include/linux/rculist.h
> > > > +++ b/include/linux/rculist.h
> > > > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > > >   */
> > > >  #define list_next_rcu(list)	(*((struct list_head __rcu **)(&(list)->next)))
> > > >  
> > > > +/*
> > > > + * Check during list traversal that we are within an RCU reader
> > > > + */
> > > > +
> > > > +#ifdef CONFIG_PROVE_RCU_LIST
> > > 
> > > This new Kconfig option is OK temporarily, but unless there is reason to
> > > fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> > > find, it would be better to just use CONFIG_PROVE_RCU.  The overall goal
> > > is to reduce the number of RCU knobs rather than grow them, must though
> > > history might lead one to believe otherwise.  :-/
> > 
> > If you want, we can try to drop this option and just use PROVE_RCU however I
> > must say there may be several warnings that need to be fixed in a short
> > period of time (even a few weeks may be too short) considering the 1000+
> > uses of RCU lists.
> Do many people other than me build with CONFIG_PROVE_RCU?  If so, then
> that would be a good reason for a temporary CONFIG_PROVE_RCU_LIST,
> as in going away in a release or two once the warnings get fixed.

PROVE_RCU is enabled by default with PROVE_LOCKING, so it is used quite
heavilty.

> > But I don't mind dropping it and it may just accelerate the fixing up of all
> > callers.
> 
> I will let you decide based on the above question.  But if you have
> CONFIG_PROVE_RCU_LIST, as noted below, it needs to depend on RCU_EXPERT.

Ok, will make it depend. But yes for temporary purpose, I will leave it as a
config and remove it later.

thanks,

 - Joel
 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3)
  2019-07-16 22:02         ` Joel Fernandes
@ 2019-07-17  0:07           ` Paul E. McKenney
  0 siblings, 0 replies; 26+ messages in thread
From: Paul E. McKenney @ 2019-07-17  0:07 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	c0d1n61at3, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Jonathan Corbet,
	Josh Triplett, keescook, kernel-hardening, kernel-team,
	Lai Jiangshan, Len Brown, linux-acpi, linux-doc, linux-pci,
	linux-pm, Mathieu Desnoyers, neilb, netdev, Oleg Nesterov,
	Pavel Machek, peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, will,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Tue, Jul 16, 2019 at 06:02:05PM -0400, Joel Fernandes wrote:
> On Tue, Jul 16, 2019 at 11:53:03AM -0700, Paul E. McKenney wrote:
> [snip]
> > > > A few more things below.
> > > > > ---
> > > > >  include/linux/rculist.h  | 28 ++++++++++++++++++++-----
> > > > >  include/linux/rcupdate.h |  7 +++++++
> > > > >  kernel/rcu/Kconfig.debug | 11 ++++++++++
> > > > >  kernel/rcu/update.c      | 44 ++++++++++++++++++++++++----------------
> > > > >  4 files changed, 67 insertions(+), 23 deletions(-)
> > > > > 
> > > > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > > > > index e91ec9ddcd30..1048160625bb 100644
> > > > > --- a/include/linux/rculist.h
> > > > > +++ b/include/linux/rculist.h
> > > > > @@ -40,6 +40,20 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
> > > > >   */
> > > > >  #define list_next_rcu(list)	(*((struct list_head __rcu **)(&(list)->next)))
> > > > >  
> > > > > +/*
> > > > > + * Check during list traversal that we are within an RCU reader
> > > > > + */
> > > > > +
> > > > > +#ifdef CONFIG_PROVE_RCU_LIST
> > > > 
> > > > This new Kconfig option is OK temporarily, but unless there is reason to
> > > > fear malfunction that a few weeks of rcutorture, 0day, and -next won't
> > > > find, it would be better to just use CONFIG_PROVE_RCU.  The overall goal
> > > > is to reduce the number of RCU knobs rather than grow them, must though
> > > > history might lead one to believe otherwise.  :-/
> > > 
> > > If you want, we can try to drop this option and just use PROVE_RCU however I
> > > must say there may be several warnings that need to be fixed in a short
> > > period of time (even a few weeks may be too short) considering the 1000+
> > > uses of RCU lists.
> > Do many people other than me build with CONFIG_PROVE_RCU?  If so, then
> > that would be a good reason for a temporary CONFIG_PROVE_RCU_LIST,
> > as in going away in a release or two once the warnings get fixed.
> 
> PROVE_RCU is enabled by default with PROVE_LOCKING, so it is used quite
> heavilty.
> 
> > > But I don't mind dropping it and it may just accelerate the fixing up of all
> > > callers.
> > 
> > I will let you decide based on the above question.  But if you have
> > CONFIG_PROVE_RCU_LIST, as noted below, it needs to depend on RCU_EXPERT.
> 
> Ok, will make it depend. But yes for temporary purpose, I will leave it as a
> config and remove it later.

Very good, thank you!  Plus you got another ack.  ;-)

							Thanx, Paul

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2019-07-17  0:10 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-15 14:36 [PATCH 0/9] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
2019-07-15 14:36 ` [PATCH 1/9] rcu/update: Remove useless check for debug_locks (v1) Joel Fernandes (Google)
2019-07-15 14:36 ` [PATCH 2/9] rcu: Add support for consolidated-RCU reader checking (v3) Joel Fernandes (Google)
2019-07-16 18:38   ` Paul E. McKenney
2019-07-16 18:46     ` Joel Fernandes
2019-07-16 18:53       ` Paul E. McKenney
2019-07-16 22:02         ` Joel Fernandes
2019-07-17  0:07           ` Paul E. McKenney
2019-07-15 14:36 ` [PATCH 3/9] rcu/sync: Remove custom check for reader-section (v2) Joel Fernandes (Google)
2019-07-16 18:39   ` Paul E. McKenney
2019-07-15 14:37 ` [PATCH 4/9] ipv4: add lockdep condition to fix for_each_entry (v1) Joel Fernandes (Google)
2019-07-16 18:39   ` Paul E. McKenney
2019-07-16 21:12     ` David Miller
2019-07-15 14:37 ` [PATCH 5/9] driver/core: Convert to use built-in RCU list checking (v1) Joel Fernandes (Google)
2019-07-16 18:40   ` Paul E. McKenney
2019-07-15 14:37 ` [PATCH 6/9] workqueue: Convert for_each_wq to use built-in list check (v2) Joel Fernandes (Google)
2019-07-16 18:41   ` Paul E. McKenney
2019-07-15 14:37 ` [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1) Joel Fernandes (Google)
2019-07-15 20:02   ` Bjorn Helgaas
2019-07-16  4:03     ` Joel Fernandes
2019-07-16 18:42       ` Paul E. McKenney
2019-07-15 14:37 ` [PATCH 8/9] acpi: Use built-in RCU list checking for acpi_ioremaps list (v1) Joel Fernandes (Google)
2019-07-15 21:44   ` Rafael J. Wysocki
2019-07-16 18:43   ` Paul E. McKenney
2019-07-15 14:37 ` [PATCH 9/9] doc: Update documentation about list_for_each_entry_rcu (v1) Joel Fernandes (Google)
2019-07-16 18:46 ` [PATCH 0/9] Harden list_for_each_entry_rcu() and family Paul E. McKenney

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).