Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [RFC 0/6] Harden list_for_each_entry_rcu() and family
@ 2019-06-01 22:27 Joel Fernandes (Google)
  2019-06-01 22:27 ` [RFC 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
                   ` (5 more replies)
  0 siblings, 6 replies; 21+ messages in thread
From: Joel Fernandes (Google) @ 2019-06-01 22:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, x86

Hi,
Please consider this as an RFC / proof-of-concept to gather some feedback. This
series aims to provide lockdep checking to RCU list macros.

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_enry_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, 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.

Now, 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.
This series 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).

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

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

Joel Fernandes (Google) (6):
rcu: Add support for consolidated-RCU reader checking
ipv4: add lockdep condition to fix for_each_entry
driver/core: Convert to use built-in RCU list checking
workqueue: Convert for_each_wq to use built-in list check
x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator
acpi: Use built-in RCU list checking for acpi_ioremaps list

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/rculist.h        | 40 ++++++++++++++++++++++++++++++----
include/linux/rcupdate.h       |  7 ++++++
kernel/rcu/update.c            | 26 ++++++++++++++++++++++
kernel/workqueue.c             |  5 ++---
net/ipv4/fib_frontend.c        |  3 ++-
10 files changed, 101 insertions(+), 17 deletions(-)

--
2.22.0.rc1.311.g5d7573a151-goog


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

* [RFC 1/6] rcu: Add support for consolidated-RCU reader checking
  2019-06-01 22:27 [RFC 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
@ 2019-06-01 22:27 ` Joel Fernandes (Google)
  2019-06-03  8:01   ` Peter Zijlstra
  2019-06-04 14:01   ` Rasmus Villemoes
  2019-06-01 22:27 ` [RFC 2/6] ipv4: add lockdep condition to fix for_each_entry Joel Fernandes (Google)
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 21+ messages in thread
From: Joel Fernandes (Google) @ 2019-06-01 22:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, x86

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  | 40 ++++++++++++++++++++++++++++++++++++----
 include/linux/rcupdate.h |  7 +++++++
 kernel/rcu/update.c      | 26 ++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index e91ec9ddcd30..b641fdd9f1a2 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -40,6 +40,25 @@ 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
+ */
+#define __list_check_rcu()						\
+	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),			\
+			 "RCU-list traversed in non-reader section!")
+
+static inline void __list_check_rcu_cond(int dummy, ...)
+{
+	va_list ap;
+	int cond;
+
+	va_start(ap, dummy);
+	cond = va_arg(ap, int);
+	va_end(ap);
+
+	RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),
+			 "RCU-list traversed in non-reader section!");
+}
 /*
  * Insert a new entry between two known consecutive entries.
  *
@@ -338,6 +357,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
 						  member) : NULL; \
 })
 
+#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
+#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
+
 /**
  * list_for_each_entry_rcu	-	iterate over rcu list of given type
  * @pos:	the type * to use as a loop cursor.
@@ -348,9 +370,14 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * 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...)		\
+	if (COUNT_VARGS(cond) != 0) {					\
+		__list_check_rcu_cond(0, ## cond);			\
+	} else {							\
+		__list_check_rcu();					\
+	}								\
+	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
+		&pos->member != (head);					\
 		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
 
 /**
@@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
  * 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)			\
+#define hlist_for_each_entry_rcu(pos, head, member, cond...)		\
+	if (COUNT_VARGS(cond) != 0) {					\
+		__list_check_rcu_cond(0, ## cond);			\
+	} else {							\
+		__list_check_rcu();					\
+	}								\
 	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
 			typeof(*(pos)), member);			\
 		pos;							\
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 922bb6848813..712b464ab960 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -223,6 +223,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 */
 
@@ -243,6 +244,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/update.c b/kernel/rcu/update.c
index c3bf44ba42e5..9cb30006a5e1 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -298,6 +298,32 @@ int rcu_read_lock_bh_held(void)
 }
 EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
 
+int rcu_read_lock_any_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;
+
+	/* Preemptible RCU flavor */
+	if (lock_is_held(&rcu_lock_map))
+		return 1;
+
+	/* BH flavor */
+	if (in_softirq() || irqs_disabled())
+		return 1;
+
+	/* Sched flavor */
+	if (debug_locks)
+		lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
+	return lockdep_opinion || !preemptible();
+}
+EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
+
 #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
 
 /**
-- 
2.22.0.rc1.311.g5d7573a151-goog


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

* [RFC 2/6] ipv4: add lockdep condition to fix for_each_entry
  2019-06-01 22:27 [RFC 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
  2019-06-01 22:27 ` [RFC 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
@ 2019-06-01 22:27 ` Joel Fernandes (Google)
  2019-06-02  7:00   ` Pavel Machek
  2019-06-01 22:27 ` [RFC 3/6] driver/core: Convert to use built-in RCU list checking Joel Fernandes (Google)
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 21+ messages in thread
From: Joel Fernandes (Google) @ 2019-06-01 22:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, x86

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 b298255f6fdb..ef7c9f8e8682 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -127,7 +127,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.rc1.311.g5d7573a151-goog


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

* [RFC 3/6] driver/core: Convert to use built-in RCU list checking
  2019-06-01 22:27 [RFC 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
  2019-06-01 22:27 ` [RFC 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
  2019-06-01 22:27 ` [RFC 2/6] ipv4: add lockdep condition to fix for_each_entry Joel Fernandes (Google)
@ 2019-06-01 22:27 ` Joel Fernandes (Google)
  2019-06-01 22:27 ` [RFC 4/6] workqueue: Convert for_each_wq to use built-in list check Joel Fernandes (Google)
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 21+ messages in thread
From: Joel Fernandes (Google) @ 2019-06-01 22:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, x86

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

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 fd7511e04e62..6c5ca9685647 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.rc1.311.g5d7573a151-goog


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

* [RFC 4/6] workqueue: Convert for_each_wq to use built-in list check
  2019-06-01 22:27 [RFC 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
                   ` (2 preceding siblings ...)
  2019-06-01 22:27 ` [RFC 3/6] driver/core: Convert to use built-in RCU list checking Joel Fernandes (Google)
@ 2019-06-01 22:27 ` Joel Fernandes (Google)
  2019-06-05  1:24   ` Daniel Jordan
  2019-06-01 22:27 ` [RFC 5/6] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator Joel Fernandes (Google)
  2019-06-01 22:27 ` [RFC 6/6] acpi: Use built-in RCU list checking for acpi_ioremaps list Joel Fernandes (Google)
  5 siblings, 1 reply; 21+ messages in thread
From: Joel Fernandes (Google) @ 2019-06-01 22:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, x86

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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 9657315405de..91ed7aca16e5 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -424,9 +424,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.rc1.311.g5d7573a151-goog


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

* [RFC 5/6] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator
  2019-06-01 22:27 [RFC 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
                   ` (3 preceding siblings ...)
  2019-06-01 22:27 ` [RFC 4/6] workqueue: Convert for_each_wq to use built-in list check Joel Fernandes (Google)
@ 2019-06-01 22:27 ` Joel Fernandes (Google)
  2019-06-01 22:27 ` [RFC 6/6] acpi: Use built-in RCU list checking for acpi_ioremaps list Joel Fernandes (Google)
  5 siblings, 0 replies; 21+ messages in thread
From: Joel Fernandes (Google) @ 2019-06-01 22:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, x86

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.rc1.311.g5d7573a151-goog


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

* [RFC 6/6] acpi: Use built-in RCU list checking for acpi_ioremaps list
  2019-06-01 22:27 [RFC 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
                   ` (4 preceding siblings ...)
  2019-06-01 22:27 ` [RFC 5/6] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator Joel Fernandes (Google)
@ 2019-06-01 22:27 ` Joel Fernandes (Google)
  5 siblings, 0 replies; 21+ messages in thread
From: Joel Fernandes (Google) @ 2019-06-01 22:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: Joel Fernandes (Google),
	Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner, x86

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 f29e427d0d1d..c8b5d712c7ae 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -28,6 +28,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>
@@ -94,6 +95,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)
@@ -220,7 +222,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;
@@ -263,7 +265,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.rc1.311.g5d7573a151-goog


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

* Re: [RFC 2/6] ipv4: add lockdep condition to fix for_each_entry
  2019-06-01 22:27 ` [RFC 2/6] ipv4: add lockdep condition to fix for_each_entry Joel Fernandes (Google)
@ 2019-06-02  7:00   ` Pavel Machek
  2019-06-02 12:20     ` Joel Fernandes
  0 siblings, 1 reply; 21+ messages in thread
From: Pavel Machek @ 2019-06-02  7:00 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, peterz, Rafael J. Wysocki, rcu, Steven Rostedt,
	Tejun Heo, Thomas Gleixner,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On Sat 2019-06-01 18:27:34, Joel Fernandes (Google) wrote:
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

This really needs to be merged to previous patch, you can't break
compilation in middle of series...

Or probably you need hlist_for_each_entry_rcu_lockdep() macro with
additional argument, and switch users to it.
								Pavel

>  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 b298255f6fdb..ef7c9f8e8682 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -127,7 +127,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;
>  	}

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

* Re: [RFC 2/6] ipv4: add lockdep condition to fix for_each_entry
  2019-06-02  7:00   ` Pavel Machek
@ 2019-06-02 12:20     ` Joel Fernandes
  2019-06-02 12:24       ` Joel Fernandes
  0 siblings, 1 reply; 21+ messages in thread
From: Joel Fernandes @ 2019-06-02 12:20 UTC (permalink / raw)
  To: Pavel Machek
  Cc: LKML, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, Eric Dumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Josh Triplett,
	Kees Cook, Kernel Hardening, Lai Jiangshan, Len Brown,
	linux-acpi, linux-pci, Linux PM, Mathieu Desnoyers, Neil Brown,
	netdev, Oleg Nesterov, Paul E. McKenney, Peter Zilstra,
	Rafael J. Wysocki, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Sun, Jun 2, 2019 at 3:00 AM Pavel Machek <pavel@denx.de> wrote:
>
> On Sat 2019-06-01 18:27:34, Joel Fernandes (Google) wrote:
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> This really needs to be merged to previous patch, you can't break
> compilation in middle of series...
>
> Or probably you need hlist_for_each_entry_rcu_lockdep() macro with
> additional argument, and switch users to it.

Good point. I can also just add a temporary transition macro, and then
remove it in the last patch. That way no new macro is needed.

Thanks!

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

* Re: [RFC 2/6] ipv4: add lockdep condition to fix for_each_entry
  2019-06-02 12:20     ` Joel Fernandes
@ 2019-06-02 12:24       ` Joel Fernandes
  2019-06-03  6:42         ` Pavel Machek
  0 siblings, 1 reply; 21+ messages in thread
From: Joel Fernandes @ 2019-06-02 12:24 UTC (permalink / raw)
  To: Pavel Machek
  Cc: LKML, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, Eric Dumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Josh Triplett,
	Kees Cook, Kernel Hardening, Lai Jiangshan, Len Brown,
	linux-acpi, linux-pci, Linux PM, Mathieu Desnoyers, Neil Brown,
	netdev, Oleg Nesterov, Paul E. McKenney, Peter Zilstra,
	Rafael J. Wysocki, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Sun, Jun 2, 2019 at 8:20 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Sun, Jun 2, 2019 at 3:00 AM Pavel Machek <pavel@denx.de> wrote:
> >
> > On Sat 2019-06-01 18:27:34, Joel Fernandes (Google) wrote:
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > This really needs to be merged to previous patch, you can't break
> > compilation in middle of series...
> >
> > Or probably you need hlist_for_each_entry_rcu_lockdep() macro with
> > additional argument, and switch users to it.
>
> Good point. I can also just add a temporary transition macro, and then
> remove it in the last patch. That way no new macro is needed.

Actually, no. There is no compilation break so I did not follow what
you mean. The fourth argument to the hlist_for_each_entry_rcu is
optional. The only thing that happens is new lockdep warnings will
arise which later parts of the series fix by passing in that fourth
argument.

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

* Re: [RFC 2/6] ipv4: add lockdep condition to fix for_each_entry
  2019-06-02 12:24       ` Joel Fernandes
@ 2019-06-03  6:42         ` Pavel Machek
  2019-06-03 12:28           ` Joel Fernandes
  0 siblings, 1 reply; 21+ messages in thread
From: Pavel Machek @ 2019-06-03  6:42 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Pavel Machek, LKML, Alexey Kuznetsov, Bjorn Helgaas,
	Borislav Petkov, David S. Miller, Eric Dumazet,
	Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
	Ingo Molnar, Josh Triplett, Kees Cook, Kernel Hardening,
	Lai Jiangshan, Len Brown, linux-acpi, linux-pci, Linux PM,
	Mathieu Desnoyers, Neil Brown, netdev, Oleg Nesterov,
	Paul E. McKenney, Peter Zilstra, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Sun 2019-06-02 08:24:35, Joel Fernandes wrote:
> On Sun, Jun 2, 2019 at 8:20 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > On Sun, Jun 2, 2019 at 3:00 AM Pavel Machek <pavel@denx.de> wrote:
> > >
> > > On Sat 2019-06-01 18:27:34, Joel Fernandes (Google) wrote:
> > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > >
> > > This really needs to be merged to previous patch, you can't break
> > > compilation in middle of series...
> > >
> > > Or probably you need hlist_for_each_entry_rcu_lockdep() macro with
> > > additional argument, and switch users to it.
> >
> > Good point. I can also just add a temporary transition macro, and then
> > remove it in the last patch. That way no new macro is needed.
> 
> Actually, no. There is no compilation break so I did not follow what
> you mean. The fourth argument to the hlist_for_each_entry_rcu is
> optional. The only thing that happens is new lockdep warnings will
> arise which later parts of the series fix by passing in that fourth
> argument.

Sorry, I missed that subtlety. Might be worth it enabling the lockdep
warning last in the series...

								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

* Re: [RFC 1/6] rcu: Add support for consolidated-RCU reader checking
  2019-06-01 22:27 ` [RFC 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
@ 2019-06-03  8:01   ` Peter Zijlstra
  2019-06-03 14:18     ` Joel Fernandes
  2019-06-04 14:01   ` Rasmus Villemoes
  1 sibling, 1 reply; 21+ messages in thread
From: Peter Zijlstra @ 2019-06-03  8:01 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Sat, Jun 01, 2019 at 06:27:33PM -0400, Joel Fernandes (Google) wrote:
> +#define list_for_each_entry_rcu(pos, head, member, cond...)		\
> +	if (COUNT_VARGS(cond) != 0) {					\
> +		__list_check_rcu_cond(0, ## cond);			\
> +	} else {							\
> +		__list_check_rcu();					\
> +	}								\
> +	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> +		&pos->member != (head);					\
>  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
>  
>  /**
> @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
>   * 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, cond...)		\
> +	if (COUNT_VARGS(cond) != 0) {					\
> +		__list_check_rcu_cond(0, ## cond);			\
> +	} else {							\
> +		__list_check_rcu();					\
> +	}								\
>  	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
>  			typeof(*(pos)), member);			\
>  		pos;							\


This breaks code like:

	if (...)
		list_for_each_entry_rcu(...);

as they are no longer a single statement. You'll have to frob it into
the initializer part of the for statement.

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

* Re: [RFC 2/6] ipv4: add lockdep condition to fix for_each_entry
  2019-06-03  6:42         ` Pavel Machek
@ 2019-06-03 12:28           ` Joel Fernandes
  0 siblings, 0 replies; 21+ messages in thread
From: Joel Fernandes @ 2019-06-03 12:28 UTC (permalink / raw)
  To: Pavel Machek
  Cc: LKML, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, Eric Dumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Josh Triplett,
	Kees Cook, Kernel Hardening, Lai Jiangshan, Len Brown,
	linux-acpi, linux-pci, Linux PM, Mathieu Desnoyers, Neil Brown,
	netdev, Oleg Nesterov, Paul E. McKenney, Peter Zilstra,
	Rafael J. Wysocki, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jun 3, 2019 at 2:42 AM Pavel Machek <pavel@denx.de> wrote:
>
> On Sun 2019-06-02 08:24:35, Joel Fernandes wrote:
> > On Sun, Jun 2, 2019 at 8:20 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> > >
> > > On Sun, Jun 2, 2019 at 3:00 AM Pavel Machek <pavel@denx.de> wrote:
> > > >
> > > > On Sat 2019-06-01 18:27:34, Joel Fernandes (Google) wrote:
> > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > >
> > > > This really needs to be merged to previous patch, you can't break
> > > > compilation in middle of series...
> > > >
> > > > Or probably you need hlist_for_each_entry_rcu_lockdep() macro with
> > > > additional argument, and switch users to it.
> > >
> > > Good point. I can also just add a temporary transition macro, and then
> > > remove it in the last patch. That way no new macro is needed.
> >
> > Actually, no. There is no compilation break so I did not follow what
> > you mean. The fourth argument to the hlist_for_each_entry_rcu is
> > optional. The only thing that happens is new lockdep warnings will
> > arise which later parts of the series fix by passing in that fourth
> > argument.
>
> Sorry, I missed that subtlety. Might be worth it enabling the lockdep
> warning last in the series...

Good idea, will do! Thanks.

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

* Re: [RFC 1/6] rcu: Add support for consolidated-RCU reader checking
  2019-06-03  8:01   ` Peter Zijlstra
@ 2019-06-03 14:18     ` Joel Fernandes
  2019-06-03 19:42       ` Joel Fernandes
  2019-06-04 10:53       ` Steven Rostedt
  0 siblings, 2 replies; 21+ messages in thread
From: Joel Fernandes @ 2019-06-03 14:18 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jun 03, 2019 at 10:01:28AM +0200, Peter Zijlstra wrote:
> On Sat, Jun 01, 2019 at 06:27:33PM -0400, Joel Fernandes (Google) wrote:
> > +#define list_for_each_entry_rcu(pos, head, member, cond...)		\
> > +	if (COUNT_VARGS(cond) != 0) {					\
> > +		__list_check_rcu_cond(0, ## cond);			\
> > +	} else {							\
> > +		__list_check_rcu();					\
> > +	}								\
> > +	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> > +		&pos->member != (head);					\
> >  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> >  
> >  /**
> > @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> >   * 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, cond...)		\
> > +	if (COUNT_VARGS(cond) != 0) {					\
> > +		__list_check_rcu_cond(0, ## cond);			\
> > +	} else {							\
> > +		__list_check_rcu();					\
> > +	}								\
> >  	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> >  			typeof(*(pos)), member);			\
> >  		pos;							\
> 
> 
> This breaks code like:
> 
> 	if (...)
> 		list_for_each_entry_rcu(...);
> 
> as they are no longer a single statement. You'll have to frob it into
> the initializer part of the for statement.

Thanks a lot for that. I fixed it as below (diff is on top of the patch):

If not for that '##' , I could have abstracted the whole if/else
expression into its own macro and called it from list_for_each_entry_rcu() to
keep it more clean.

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

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index b641fdd9f1a2..cc742d294bb0 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -371,12 +372,15 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * as long as the traversal is guarded by rcu_read_lock().
  */
 #define list_for_each_entry_rcu(pos, head, member, cond...)		\
-	if (COUNT_VARGS(cond) != 0) {					\
-		__list_check_rcu_cond(0, ## cond);			\
-	} else {							\
-		__list_check_rcu();					\
-	}								\
-	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
+	for (								\
+	     ({								\
+		if (COUNT_VARGS(cond) != 0) {				\
+			__list_check_rcu_cond(0, ## cond);		\
+		} else {						\
+			__list_check_rcu_nocond();			\
+		}							\
+	      }),							\
+	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
 		&pos->member != (head);					\
 		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
 
@@ -649,12 +653,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
  * as long as the traversal is guarded by rcu_read_lock().
  */
 #define hlist_for_each_entry_rcu(pos, head, member, cond...)		\
-	if (COUNT_VARGS(cond) != 0) {					\
-		__list_check_rcu_cond(0, ## cond);			\
-	} else {							\
-		__list_check_rcu();					\
-	}								\
-	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
+	for (								\
+	     ({								\
+		if (COUNT_VARGS(cond) != 0) {				\
+			__list_check_rcu_cond(0, ## cond);		\
+		} else {						\
+			__list_check_rcu_nocond();			\
+		}							\
+	     }),							\
+	     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(\
-- 
2.22.0.rc1.311.g5d7573a151-goog


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

* Re: [RFC 1/6] rcu: Add support for consolidated-RCU reader checking
  2019-06-03 14:18     ` Joel Fernandes
@ 2019-06-03 19:42       ` Joel Fernandes
  2019-06-04 10:53       ` Steven Rostedt
  1 sibling, 0 replies; 21+ messages in thread
From: Joel Fernandes @ 2019-06-03 19:42 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, Jun 03, 2019 at 10:18:47AM -0400, Joel Fernandes wrote:
> On Mon, Jun 03, 2019 at 10:01:28AM +0200, Peter Zijlstra wrote:
> > On Sat, Jun 01, 2019 at 06:27:33PM -0400, Joel Fernandes (Google) wrote:
> > > +#define list_for_each_entry_rcu(pos, head, member, cond...)		\
> > > +	if (COUNT_VARGS(cond) != 0) {					\
> > > +		__list_check_rcu_cond(0, ## cond);			\
> > > +	} else {							\
> > > +		__list_check_rcu();					\
> > > +	}								\
> > > +	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> > > +		&pos->member != (head);					\
> > >  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> > >  
> > >  /**
> > > @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> > >   * 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, cond...)		\
> > > +	if (COUNT_VARGS(cond) != 0) {					\
> > > +		__list_check_rcu_cond(0, ## cond);			\
> > > +	} else {							\
> > > +		__list_check_rcu();					\
> > > +	}								\
> > >  	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > >  			typeof(*(pos)), member);			\
> > >  		pos;							\
> > 
> > 
> > This breaks code like:
> > 
> > 	if (...)
> > 		list_for_each_entry_rcu(...);
> > 
> > as they are no longer a single statement. You'll have to frob it into
> > the initializer part of the for statement.
> 
> Thanks a lot for that. I fixed it as below (diff is on top of the patch):
> 
> If not for that '##' , I could have abstracted the whole if/else
> expression into its own macro and called it from list_for_each_entry_rcu() to
> keep it more clean.

Actually was able to roll the if/else into its own macro as well, thus
keeping it clean. thanks!

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

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index b641fdd9f1a2..cc9c382b080c 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -43,7 +43,11 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
 /*
  * Check during list traversal that we are within an RCU reader
  */
-#define __list_check_rcu()						\
+
+#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
+#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
+
+#define __list_check_rcu_nocond()					\
 	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),			\
 			 "RCU-list traversed in non-reader section!")
 
@@ -59,6 +63,16 @@ static inline void __list_check_rcu_cond(int dummy, ...)
 	RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),
 			 "RCU-list traversed in non-reader section!");
 }
+
+#define __list_check_rcu(cond...)				\
+     ({								\
+	if (COUNT_VARGS(cond) != 0) {				\
+		__list_check_rcu_cond(0, ## cond);		\
+	} else {						\
+		__list_check_rcu_nocond();			\
+	}							\
+      })
+
 /*
  * Insert a new entry between two known consecutive entries.
  *
@@ -357,9 +371,6 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
 						  member) : NULL; \
 })
 
-#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
-#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
-
 /**
  * list_for_each_entry_rcu	-	iterate over rcu list of given type
  * @pos:	the type * to use as a loop cursor.
@@ -371,12 +382,8 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
  * as long as the traversal is guarded by rcu_read_lock().
  */
 #define list_for_each_entry_rcu(pos, head, member, cond...)		\
-	if (COUNT_VARGS(cond) != 0) {					\
-		__list_check_rcu_cond(0, ## cond);			\
-	} else {							\
-		__list_check_rcu();					\
-	}								\
-	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
+	for (__list_check_rcu(cond),					\
+	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
 		&pos->member != (head);					\
 		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
 
@@ -649,12 +656,8 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
  * as long as the traversal is guarded by rcu_read_lock().
  */
 #define hlist_for_each_entry_rcu(pos, head, member, cond...)		\
-	if (COUNT_VARGS(cond) != 0) {					\
-		__list_check_rcu_cond(0, ## cond);			\
-	} else {							\
-		__list_check_rcu();					\
-	}								\
-	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
+	for (__list_check_rcu(cond),					\
+	     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(\
-- 
2.22.0.rc1.311.g5d7573a151-goog


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

* Re: [RFC 1/6] rcu: Add support for consolidated-RCU reader checking
  2019-06-03 14:18     ` Joel Fernandes
  2019-06-03 19:42       ` Joel Fernandes
@ 2019-06-04 10:53       ` Steven Rostedt
  2019-06-04 17:48         ` Joel Fernandes
  1 sibling, 1 reply; 21+ messages in thread
From: Steven Rostedt @ 2019-06-04 10:53 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Peter Zijlstra, linux-kernel, Alexey Kuznetsov, Bjorn Helgaas,
	Borislav Petkov, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Josh Triplett,
	keescook, kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, Rafael J. Wysocki, rcu,
	Tejun Heo, Thomas Gleixner,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Mon, 3 Jun 2019 10:18:47 -0400
Joel Fernandes <joel@joelfernandes.org> wrote:

> On Mon, Jun 03, 2019 at 10:01:28AM +0200, Peter Zijlstra wrote:
> > On Sat, Jun 01, 2019 at 06:27:33PM -0400, Joel Fernandes (Google) wrote:  
> > > +#define list_for_each_entry_rcu(pos, head, member, cond...)		\
> > > +	if (COUNT_VARGS(cond) != 0) {					\
> > > +		__list_check_rcu_cond(0, ## cond);			\
> > > +	} else {							\
> > > +		__list_check_rcu();					\
> > > +	}								\
> > > +	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> > > +		&pos->member != (head);					\
> > >  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> > >  
> > >  /**
> > > @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> > >   * 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, cond...)		\
> > > +	if (COUNT_VARGS(cond) != 0) {					\
> > > +		__list_check_rcu_cond(0, ## cond);			\
> > > +	} else {							\
> > > +		__list_check_rcu();					\
> > > +	}								\
> > >  	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > >  			typeof(*(pos)), member);			\
> > >  		pos;							\  
> > 
> > 
> > This breaks code like:
> > 
> > 	if (...)
> > 		list_for_each_entry_rcu(...);
> > 
> > as they are no longer a single statement. You'll have to frob it into
> > the initializer part of the for statement.  
> 
> Thanks a lot for that. I fixed it as below (diff is on top of the patch):
> 
> If not for that '##' , I could have abstracted the whole if/else
> expression into its own macro and called it from list_for_each_entry_rcu() to
> keep it more clean.
> 
> ---8<-----------------------
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index b641fdd9f1a2..cc742d294bb0 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -371,12 +372,15 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
>   * as long as the traversal is guarded by rcu_read_lock().
>   */
>  #define list_for_each_entry_rcu(pos, head, member, cond...)		\
> -	if (COUNT_VARGS(cond) != 0) {					\
> -		__list_check_rcu_cond(0, ## cond);			\
> -	} else {							\
> -		__list_check_rcu();					\
> -	}								\
> -	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> +	for (								\
> +	     ({								\
> +		if (COUNT_VARGS(cond) != 0) {				\
> +			__list_check_rcu_cond(0, ## cond);		\
> +		} else {						\
> +			__list_check_rcu_nocond();			\
> +		}							\
> +	      }),							\

For easier to read I would do something like this:

#define check_rcu_list(cond)						\
	({								\
		if (COUNT_VARGS(cond) != 0)				\
			__list_check_rcu_cond(0, ## cond);		\
		else							\
			__list_check_rcu_nocond();			\
	})

#define list_for_each_entry_rcu(pos, head, member, cond...)		\
	for (check_rcu_list(cond),					\


-- Steve

> +	     pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
>  		&pos->member != (head);					\
>  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
>  
> @@ -649,12 +653,15 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
>   * as long as the traversal is guarded by rcu_read_lock().
>   */
>  #define hlist_for_each_entry_rcu(pos, head, member, cond...)		\
> -	if (COUNT_VARGS(cond) != 0) {					\
> -		__list_check_rcu_cond(0, ## cond);			\
> -	} else {							\
> -		__list_check_rcu();					\
> -	}								\
> -	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> +	for (								\
> +	     ({								\
> +		if (COUNT_VARGS(cond) != 0) {				\
> +			__list_check_rcu_cond(0, ## cond);		\
> +		} else {						\
> +			__list_check_rcu_nocond();			\
> +		}							\
> +	     }),							\
> +	     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(\


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

* Re: [RFC 1/6] rcu: Add support for consolidated-RCU reader checking
  2019-06-01 22:27 ` [RFC 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
  2019-06-03  8:01   ` Peter Zijlstra
@ 2019-06-04 14:01   ` Rasmus Villemoes
  2019-06-04 23:57     ` Joel Fernandes
  1 sibling, 1 reply; 21+ messages in thread
From: Rasmus Villemoes @ 2019-06-04 14:01 UTC (permalink / raw)
  To: Joel Fernandes (Google), linux-kernel
  Cc: Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On 02/06/2019 00.27, 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>
> ---
>  include/linux/rculist.h  | 40 ++++++++++++++++++++++++++++++++++++----
>  include/linux/rcupdate.h |  7 +++++++
>  kernel/rcu/update.c      | 26 ++++++++++++++++++++++++++
>  3 files changed, 69 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index e91ec9ddcd30..b641fdd9f1a2 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -40,6 +40,25 @@ 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
> + */
> +#define __list_check_rcu()						\
> +	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),			\
> +			 "RCU-list traversed in non-reader section!")
> +
> +static inline void __list_check_rcu_cond(int dummy, ...)
> +{
> +	va_list ap;
> +	int cond;
> +
> +	va_start(ap, dummy);
> +	cond = va_arg(ap, int);
> +	va_end(ap);
> +
> +	RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),
> +			 "RCU-list traversed in non-reader section!");
> +}
>  /*
>   * Insert a new entry between two known consecutive entries.
>   *
> @@ -338,6 +357,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
>  						  member) : NULL; \
>  })
>  
> +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
> +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
> +>  /**
>   * list_for_each_entry_rcu	-	iterate over rcu list of given type
>   * @pos:	the type * to use as a loop cursor.
> @@ -348,9 +370,14 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
>   * 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...)		\
> +	if (COUNT_VARGS(cond) != 0) {					\
> +		__list_check_rcu_cond(0, ## cond);			\
> +	} else {							\
> +		__list_check_rcu();					\
> +	}								\
> +	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> +		&pos->member != (head);					\
>  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))

Wouldn't something as simple as

#define __list_check_rcu(dummy, cond, ...) \
       RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
			 "RCU-list traversed in non-reader section!");

for ( ({ __list_check_rcu(junk, ##cond, 0); }), pos = ... )

work just as well (i.e., no need for two list_check_rcu and
list_check_rcu_cond variants)? If there's an optional cond, we use that,
if not, we pick the trailing 0, so !cond disappears and it reduces to
your __list_check_rcu(). Moreover, this ensures the RCU_LOCKDEP_WARN
expansion actually picks up the __LINE__ and __FILE__ where the for loop
is used, and not the __FILE__ and __LINE__ of the static inline function
from the header file. It also makes it a bit more type safe/type generic
(if the cond expression happened to have type long or u64 something
rather odd could happen with the inline vararg function).

Rasmus

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

* Re: [RFC 1/6] rcu: Add support for consolidated-RCU reader checking
  2019-06-04 10:53       ` Steven Rostedt
@ 2019-06-04 17:48         ` Joel Fernandes
  0 siblings, 0 replies; 21+ messages in thread
From: Joel Fernandes @ 2019-06-04 17:48 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Peter Zijlstra, linux-kernel, Alexey Kuznetsov, Bjorn Helgaas,
	Borislav Petkov, David S. Miller, edumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Josh Triplett,
	keescook, kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, Rafael J. Wysocki, rcu,
	Tejun Heo, Thomas Gleixner,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Tue, Jun 04, 2019 at 06:53:58AM -0400, Steven Rostedt wrote:
> On Mon, 3 Jun 2019 10:18:47 -0400
> Joel Fernandes <joel@joelfernandes.org> wrote:
> 
> > On Mon, Jun 03, 2019 at 10:01:28AM +0200, Peter Zijlstra wrote:
> > > On Sat, Jun 01, 2019 at 06:27:33PM -0400, Joel Fernandes (Google) wrote:  
> > > > +#define list_for_each_entry_rcu(pos, head, member, cond...)		\
> > > > +	if (COUNT_VARGS(cond) != 0) {					\
> > > > +		__list_check_rcu_cond(0, ## cond);			\
> > > > +	} else {							\
> > > > +		__list_check_rcu();					\
> > > > +	}								\
> > > > +	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> > > > +		&pos->member != (head);					\
> > > >  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> > > >  
> > > >  /**
> > > > @@ -621,7 +648,12 @@ static inline void hlist_add_behind_rcu(struct hlist_node *n,
> > > >   * 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, cond...)		\
> > > > +	if (COUNT_VARGS(cond) != 0) {					\
> > > > +		__list_check_rcu_cond(0, ## cond);			\
> > > > +	} else {							\
> > > > +		__list_check_rcu();					\
> > > > +	}								\
> > > >  	for (pos = hlist_entry_safe (rcu_dereference_raw(hlist_first_rcu(head)),\
> > > >  			typeof(*(pos)), member);			\
> > > >  		pos;							\  
> > > 
> > > 
> > > This breaks code like:
> > > 
> > > 	if (...)
> > > 		list_for_each_entry_rcu(...);
> > > 
> > > as they are no longer a single statement. You'll have to frob it into
> > > the initializer part of the for statement.  
> > 
> > Thanks a lot for that. I fixed it as below (diff is on top of the patch):
> > 
> > If not for that '##' , I could have abstracted the whole if/else
> > expression into its own macro and called it from list_for_each_entry_rcu() to
> > keep it more clean.
> > 
> > ---8<-----------------------
> > 
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index b641fdd9f1a2..cc742d294bb0 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -371,12 +372,15 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> >   * as long as the traversal is guarded by rcu_read_lock().
> >   */
> >  #define list_for_each_entry_rcu(pos, head, member, cond...)		\
> > -	if (COUNT_VARGS(cond) != 0) {					\
> > -		__list_check_rcu_cond(0, ## cond);			\
> > -	} else {							\
> > -		__list_check_rcu();					\
> > -	}								\
> > -	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> > +	for (								\
> > +	     ({								\
> > +		if (COUNT_VARGS(cond) != 0) {				\
> > +			__list_check_rcu_cond(0, ## cond);		\
> > +		} else {						\
> > +			__list_check_rcu_nocond();			\
> > +		}							\
> > +	      }),							\
> 
> For easier to read I would do something like this:
> 
> #define check_rcu_list(cond)						\
> 	({								\
> 		if (COUNT_VARGS(cond) != 0)				\
> 			__list_check_rcu_cond(0, ## cond);		\
> 		else							\
> 			__list_check_rcu_nocond();			\
> 	})
> 
> #define list_for_each_entry_rcu(pos, head, member, cond...)		\
> 	for (check_rcu_list(cond),					\

Yes, already doing it this way as I replied to Peter here:
https://lore.kernel.org/patchwork/patch/1082846/#1278489

Thanks!

 - Joel



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

* Re: [RFC 1/6] rcu: Add support for consolidated-RCU reader checking
  2019-06-04 14:01   ` Rasmus Villemoes
@ 2019-06-04 23:57     ` Joel Fernandes
  0 siblings, 0 replies; 21+ messages in thread
From: Joel Fernandes @ 2019-06-04 23:57 UTC (permalink / raw)
  To: Rasmus Villemoes
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Tue, Jun 04, 2019 at 04:01:00PM +0200, Rasmus Villemoes wrote:
> On 02/06/2019 00.27, 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>
> > ---
> >  include/linux/rculist.h  | 40 ++++++++++++++++++++++++++++++++++++----
> >  include/linux/rcupdate.h |  7 +++++++
> >  kernel/rcu/update.c      | 26 ++++++++++++++++++++++++++
> >  3 files changed, 69 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index e91ec9ddcd30..b641fdd9f1a2 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -40,6 +40,25 @@ 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
> > + */
> > +#define __list_check_rcu()						\
> > +	RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),			\
> > +			 "RCU-list traversed in non-reader section!")
> > +
> > +static inline void __list_check_rcu_cond(int dummy, ...)
> > +{
> > +	va_list ap;
> > +	int cond;
> > +
> > +	va_start(ap, dummy);
> > +	cond = va_arg(ap, int);
> > +	va_end(ap);
> > +
> > +	RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(),
> > +			 "RCU-list traversed in non-reader section!");
> > +}
> >  /*
> >   * Insert a new entry between two known consecutive entries.
> >   *
> > @@ -338,6 +357,9 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> >  						  member) : NULL; \
> >  })
> >  
> > +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
> > +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
> > +>  /**
> >   * list_for_each_entry_rcu	-	iterate over rcu list of given type
> >   * @pos:	the type * to use as a loop cursor.
> > @@ -348,9 +370,14 @@ static inline void list_splice_tail_init_rcu(struct list_head *list,
> >   * 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...)		\
> > +	if (COUNT_VARGS(cond) != 0) {					\
> > +		__list_check_rcu_cond(0, ## cond);			\
> > +	} else {							\
> > +		__list_check_rcu();					\
> > +	}								\
> > +	for (pos = list_entry_rcu((head)->next, typeof(*pos), member);	\
> > +		&pos->member != (head);					\
> >  		pos = list_entry_rcu(pos->member.next, typeof(*pos), member))
> 
> Wouldn't something as simple as
> 
> #define __list_check_rcu(dummy, cond, ...) \
>        RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
> 			 "RCU-list traversed in non-reader section!");
> 
> for ( ({ __list_check_rcu(junk, ##cond, 0); }), pos = ... )
> 
> work just as well (i.e., no need for two list_check_rcu and
> list_check_rcu_cond variants)? If there's an optional cond, we use that,
> if not, we pick the trailing 0, so !cond disappears and it reduces to
> your __list_check_rcu(). Moreover, this ensures the RCU_LOCKDEP_WARN
> expansion actually picks up the __LINE__ and __FILE__ where the for loop
> is used, and not the __FILE__ and __LINE__ of the static inline function
> from the header file. It also makes it a bit more type safe/type generic
> (if the cond expression happened to have type long or u64 something
> rather odd could happen with the inline vararg function).

This is much better. I will do it this way. Thank you!

 - Joel


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

* Re: [RFC 4/6] workqueue: Convert for_each_wq to use built-in list check
  2019-06-01 22:27 ` [RFC 4/6] workqueue: Convert for_each_wq to use built-in list check Joel Fernandes (Google)
@ 2019-06-05  1:24   ` Daniel Jordan
  2019-06-05 13:04     ` Joel Fernandes
  0 siblings, 1 reply; 21+ messages in thread
From: Daniel Jordan @ 2019-06-05  1:24 UTC (permalink / raw)
  To: Joel Fernandes (Google)
  Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
	H. Peter Anvin, Ingo Molnar, Josh Triplett, keescook,
	kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
	linux-pci, linux-pm, Mathieu Desnoyers, neilb, netdev, oleg,
	Paul E. McKenney, Pavel Machek, peterz, Rafael J. Wysocki, rcu,
	Steven Rostedt, Tejun Heo, Thomas Gleixner,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Sat, Jun 01, 2019 at 06:27:36PM -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>
> ---
>  kernel/workqueue.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 9657315405de..91ed7aca16e5 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -424,9 +424,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))
>  

I think the definition of assert_rcu_or_wq_mutex can also be deleted.

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

* Re: [RFC 4/6] workqueue: Convert for_each_wq to use built-in list check
  2019-06-05  1:24   ` Daniel Jordan
@ 2019-06-05 13:04     ` Joel Fernandes
  0 siblings, 0 replies; 21+ messages in thread
From: Joel Fernandes @ 2019-06-05 13:04 UTC (permalink / raw)
  To: Daniel Jordan
  Cc: LKML, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
	David S. Miller, Eric Dumazet, Greg Kroah-Hartman,
	Hideaki YOSHIFUJI, H. Peter Anvin, Ingo Molnar, Josh Triplett,
	Kees Cook, Kernel Hardening, Lai Jiangshan, Len Brown,
	linux-acpi, linux-pci, Linux PM, Mathieu Desnoyers, Neil Brown,
	netdev, Oleg Nesterov, Paul E. McKenney, Pavel Machek,
	Peter Zilstra, Rafael J. Wysocki, rcu, Steven Rostedt, Tejun Heo,
	Thomas Gleixner, maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)

On Tue, Jun 4, 2019 at 9:25 PM Daniel Jordan <daniel.m.jordan@oracle.com> wrote:
>
> On Sat, Jun 01, 2019 at 06:27:36PM -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>
> > ---
> >  kernel/workqueue.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 9657315405de..91ed7aca16e5 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -424,9 +424,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))
> >
>
> I think the definition of assert_rcu_or_wq_mutex can also be deleted.

Sure, will do. Thank you.

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

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-01 22:27 [RFC 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
2019-06-01 22:27 ` [RFC 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
2019-06-03  8:01   ` Peter Zijlstra
2019-06-03 14:18     ` Joel Fernandes
2019-06-03 19:42       ` Joel Fernandes
2019-06-04 10:53       ` Steven Rostedt
2019-06-04 17:48         ` Joel Fernandes
2019-06-04 14:01   ` Rasmus Villemoes
2019-06-04 23:57     ` Joel Fernandes
2019-06-01 22:27 ` [RFC 2/6] ipv4: add lockdep condition to fix for_each_entry Joel Fernandes (Google)
2019-06-02  7:00   ` Pavel Machek
2019-06-02 12:20     ` Joel Fernandes
2019-06-02 12:24       ` Joel Fernandes
2019-06-03  6:42         ` Pavel Machek
2019-06-03 12:28           ` Joel Fernandes
2019-06-01 22:27 ` [RFC 3/6] driver/core: Convert to use built-in RCU list checking Joel Fernandes (Google)
2019-06-01 22:27 ` [RFC 4/6] workqueue: Convert for_each_wq to use built-in list check Joel Fernandes (Google)
2019-06-05  1:24   ` Daniel Jordan
2019-06-05 13:04     ` Joel Fernandes
2019-06-01 22:27 ` [RFC 5/6] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator Joel Fernandes (Google)
2019-06-01 22:27 ` [RFC 6/6] acpi: Use built-in RCU list checking for acpi_ioremaps list Joel Fernandes (Google)

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org linux-acpi@archiver.kernel.org
	public-inbox-index linux-acpi


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/ public-inbox