* [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-11 23:43 [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
@ 2019-07-11 23:43 ` Joel Fernandes (Google)
2019-07-12 4:49 ` Joel Fernandes
` (3 more replies)
2019-07-11 23:43 ` [PATCH v1 2/6] ipv4: add lockdep condition to fix for_each_entry Joel Fernandes (Google)
` (5 subsequent siblings)
6 siblings, 4 replies; 22+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-11 23:43 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, 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,
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 | 29 ++++++++++++++++++++++++-----
include/linux/rcupdate.h | 7 +++++++
kernel/rcu/Kconfig.debug | 11 +++++++++++
kernel/rcu/update.c | 26 ++++++++++++++++++++++++++
4 files changed, 68 insertions(+), 5 deletions(-)
diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index e91ec9ddcd30..78c15ec6b2c9 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -40,6 +40,23 @@ 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 SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
+#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
+
+#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.
*
@@ -348,9 +365,10 @@ 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...) \
+ 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))
/**
@@ -621,8 +639,9 @@ 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) \
- 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 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/Kconfig.debug b/kernel/rcu/Kconfig.debug
index 0ec7d1d33a14..b20d0e2903d1 100644
--- a/kernel/rcu/Kconfig.debug
+++ b/kernel/rcu/Kconfig.debug
@@ -7,6 +7,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 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.410.gd8fdbe21b5-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-11 23:43 ` [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
@ 2019-07-12 4:49 ` Joel Fernandes
2019-07-12 11:01 ` Peter Zijlstra
` (2 subsequent siblings)
3 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes @ 2019-07-12 4:49 UTC (permalink / raw)
To: linux-kernel
Cc: Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
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,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Thu, Jul 11, 2019 at 07:43:56PM -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>
> ---
> include/linux/rculist.h | 29 ++++++++++++++++++++++++-----
> include/linux/rcupdate.h | 7 +++++++
> kernel/rcu/Kconfig.debug | 11 +++++++++++
> kernel/rcu/update.c | 26 ++++++++++++++++++++++++++
> 4 files changed, 68 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index e91ec9ddcd30..78c15ec6b2c9 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -40,6 +40,23 @@ 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 SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
> +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
Fyi, I made a cosmetic change by deleting the above 2 unused macros.
- Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-11 23:43 ` [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
2019-07-12 4:49 ` Joel Fernandes
@ 2019-07-12 11:01 ` Peter Zijlstra
2019-07-12 14:49 ` Joel Fernandes
2019-07-12 11:11 ` Peter Zijlstra
2019-07-12 12:12 ` Oleg Nesterov
3 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2019-07-12 11:01 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, 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,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Thu, Jul 11, 2019 at 07:43:56PM -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>
> ---
> include/linux/rculist.h | 29 ++++++++++++++++++++++++-----
> include/linux/rcupdate.h | 7 +++++++
> kernel/rcu/Kconfig.debug | 11 +++++++++++
> kernel/rcu/update.c | 26 ++++++++++++++++++++++++++
> 4 files changed, 68 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> index e91ec9ddcd30..78c15ec6b2c9 100644
> --- a/include/linux/rculist.h
> +++ b/include/linux/rculist.h
> @@ -40,6 +40,23 @@ 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 SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
> +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
You don't seem to actually use it in this patch; also linux/kernel.h has
COUNT_ARGS().
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-12 11:01 ` Peter Zijlstra
@ 2019-07-12 14:49 ` Joel Fernandes
0 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes @ 2019-07-12 14:49 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, 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,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Fri, Jul 12, 2019 at 01:01:42PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 11, 2019 at 07:43:56PM -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>
> > ---
> > include/linux/rculist.h | 29 ++++++++++++++++++++++++-----
> > include/linux/rcupdate.h | 7 +++++++
> > kernel/rcu/Kconfig.debug | 11 +++++++++++
> > kernel/rcu/update.c | 26 ++++++++++++++++++++++++++
> > 4 files changed, 68 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index e91ec9ddcd30..78c15ec6b2c9 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -40,6 +40,23 @@ 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 SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6
> > +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0)
>
> You don't seem to actually use it in this patch; also linux/kernel.h has
> COUNT_ARGS().
Yes, I replied after sending patches that I fixed this. I will remove them.
thanks,
- Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-11 23:43 ` [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
2019-07-12 4:49 ` Joel Fernandes
2019-07-12 11:01 ` Peter Zijlstra
@ 2019-07-12 11:11 ` Peter Zijlstra
2019-07-12 15:10 ` Joel Fernandes
2019-07-12 12:12 ` Oleg Nesterov
3 siblings, 1 reply; 22+ messages in thread
From: Peter Zijlstra @ 2019-07-12 11:11 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, 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,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> +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))
you forgot debug_locks here.
> + return 1;
> +
> + /* BH flavor */
> + if (in_softirq() || irqs_disabled())
I'm not sure I'd put irqs_disabled() under BH, also this entire
condition is superfluous, see below.
> + return 1;
> +
> + /* Sched flavor */
> + if (debug_locks)
> + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> + return lockdep_opinion || !preemptible();
that !preemptible() turns into:
!(preempt_count()==0 && !irqs_disabled())
which is:
preempt_count() != 0 || irqs_disabled()
and already includes irqs_disabled() and in_softirq().
> +}
So maybe something lke:
if (debug_locks && (lock_is_held(&rcu_lock_map) ||
lock_is_held(&rcu_sched_lock_map)))
return true;
return !preemptible();
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-12 11:11 ` Peter Zijlstra
@ 2019-07-12 15:10 ` Joel Fernandes
2019-07-12 15:58 ` Peter Zijlstra
2019-07-12 16:45 ` Paul E. McKenney
0 siblings, 2 replies; 22+ messages in thread
From: Joel Fernandes @ 2019-07-12 15:10 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, 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,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > +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))
>
> you forgot debug_locks here.
Actually, it turns out debug_locks checking is not even needed. If
debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would not
get to this point.
> > + return 1;
> > +
> > + /* BH flavor */
> > + if (in_softirq() || irqs_disabled())
>
> I'm not sure I'd put irqs_disabled() under BH, also this entire
> condition is superfluous, see below.
>
> > + return 1;
> > +
> > + /* Sched flavor */
> > + if (debug_locks)
> > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > + return lockdep_opinion || !preemptible();
>
> that !preemptible() turns into:
>
> !(preempt_count()==0 && !irqs_disabled())
>
> which is:
>
> preempt_count() != 0 || irqs_disabled()
>
> and already includes irqs_disabled() and in_softirq().
>
> > +}
>
> So maybe something lke:
>
> if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> lock_is_held(&rcu_sched_lock_map)))
> return true;
Agreed, I will do it this way (without the debug_locks) like:
---8<-----------------------
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index ba861d1716d3..339aebc330db 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -296,27 +296,15 @@ 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();
+ if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
+ return 1;
+ return !preemptible();
}
EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-12 15:10 ` Joel Fernandes
@ 2019-07-12 15:58 ` Peter Zijlstra
2019-07-12 16:45 ` Paul E. McKenney
1 sibling, 0 replies; 22+ messages in thread
From: Peter Zijlstra @ 2019-07-12 15:58 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, 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,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> Agreed, I will do it this way (without the debug_locks) like:
>
> ---8<-----------------------
>
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index ba861d1716d3..339aebc330db 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
>
> int rcu_read_lock_any_held(void)
> {
> if (!debug_lockdep_rcu_enabled())
> return 1;
> if (!rcu_is_watching())
> return 0;
> if (!rcu_lockdep_current_cpu_online())
> return 0;
> + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
> + return 1;
> + return !preemptible();
> }
> EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
OK, that looks sane. Thanks!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-12 15:10 ` Joel Fernandes
2019-07-12 15:58 ` Peter Zijlstra
@ 2019-07-12 16:45 ` Paul E. McKenney
2019-07-12 17:06 ` Joel Fernandes
1 sibling, 1 reply; 22+ messages in thread
From: Paul E. McKenney @ 2019-07-12 16:45 UTC (permalink / raw)
To: Joel Fernandes
Cc: Peter Zijlstra, linux-kernel, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, 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, Pavel Machek,
Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > > +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))
> >
> > you forgot debug_locks here.
>
> Actually, it turns out debug_locks checking is not even needed. If
> debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would not
> get to this point.
>
> > > + return 1;
> > > +
> > > + /* BH flavor */
> > > + if (in_softirq() || irqs_disabled())
> >
> > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > condition is superfluous, see below.
> >
> > > + return 1;
> > > +
> > > + /* Sched flavor */
> > > + if (debug_locks)
> > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > + return lockdep_opinion || !preemptible();
> >
> > that !preemptible() turns into:
> >
> > !(preempt_count()==0 && !irqs_disabled())
> >
> > which is:
> >
> > preempt_count() != 0 || irqs_disabled()
> >
> > and already includes irqs_disabled() and in_softirq().
> >
> > > +}
> >
> > So maybe something lke:
> >
> > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > lock_is_held(&rcu_sched_lock_map)))
> > return true;
>
> Agreed, I will do it this way (without the debug_locks) like:
>
> ---8<-----------------------
>
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index ba861d1716d3..339aebc330db 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -296,27 +296,15 @@ 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();
> + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
Thanx, Paul
> + return 1;
> + return !preemptible();
> }
> EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-12 16:45 ` Paul E. McKenney
@ 2019-07-12 17:06 ` Joel Fernandes
2019-07-12 17:46 ` Paul E. McKenney
0 siblings, 1 reply; 22+ messages in thread
From: Joel Fernandes @ 2019-07-12 17:06 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Peter Zijlstra, linux-kernel, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, 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, Pavel Machek,
Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Fri, Jul 12, 2019 at 09:45:31AM -0700, Paul E. McKenney wrote:
> On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> > On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > > > +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))
> > >
> > > you forgot debug_locks here.
> >
> > Actually, it turns out debug_locks checking is not even needed. If
> > debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would not
> > get to this point.
> >
> > > > + return 1;
> > > > +
> > > > + /* BH flavor */
> > > > + if (in_softirq() || irqs_disabled())
> > >
> > > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > > condition is superfluous, see below.
> > >
> > > > + return 1;
> > > > +
> > > > + /* Sched flavor */
> > > > + if (debug_locks)
> > > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > > + return lockdep_opinion || !preemptible();
> > >
> > > that !preemptible() turns into:
> > >
> > > !(preempt_count()==0 && !irqs_disabled())
> > >
> > > which is:
> > >
> > > preempt_count() != 0 || irqs_disabled()
> > >
> > > and already includes irqs_disabled() and in_softirq().
> > >
> > > > +}
> > >
> > > So maybe something lke:
> > >
> > > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > > lock_is_held(&rcu_sched_lock_map)))
> > > return true;
> >
> > Agreed, I will do it this way (without the debug_locks) like:
> >
> > ---8<-----------------------
> >
> > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > index ba861d1716d3..339aebc330db 100644
> > --- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -296,27 +296,15 @@ 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();
> > + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
>
> OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
Hmm, I was borrowing the strategy from rcu_read_lock_bh_held() which does not
check for a lock held in this map.
Honestly, even lock_is_held(&rcu_sched_lock_map) seems unnecessary per-se
since !preemptible() will catch that? rcu_read_lock_sched() disables
preemption already, so lockdep's opinion of the matter seems redundant there.
Sorry I already sent out patches again before seeing your comment but I can
rework and resend them based on any other suggestions.
thanks,
- Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-12 17:06 ` Joel Fernandes
@ 2019-07-12 17:46 ` Paul E. McKenney
2019-07-12 19:40 ` Joel Fernandes
0 siblings, 1 reply; 22+ messages in thread
From: Paul E. McKenney @ 2019-07-12 17:46 UTC (permalink / raw)
To: Joel Fernandes
Cc: Peter Zijlstra, linux-kernel, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, 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, Pavel Machek,
Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Fri, Jul 12, 2019 at 01:06:31PM -0400, Joel Fernandes wrote:
> On Fri, Jul 12, 2019 at 09:45:31AM -0700, Paul E. McKenney wrote:
> > On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> > > On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > > > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > > > > +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))
> > > >
> > > > you forgot debug_locks here.
> > >
> > > Actually, it turns out debug_locks checking is not even needed. If
> > > debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would not
> > > get to this point.
> > >
> > > > > + return 1;
> > > > > +
> > > > > + /* BH flavor */
> > > > > + if (in_softirq() || irqs_disabled())
> > > >
> > > > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > > > condition is superfluous, see below.
> > > >
> > > > > + return 1;
> > > > > +
> > > > > + /* Sched flavor */
> > > > > + if (debug_locks)
> > > > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > > > + return lockdep_opinion || !preemptible();
> > > >
> > > > that !preemptible() turns into:
> > > >
> > > > !(preempt_count()==0 && !irqs_disabled())
> > > >
> > > > which is:
> > > >
> > > > preempt_count() != 0 || irqs_disabled()
> > > >
> > > > and already includes irqs_disabled() and in_softirq().
> > > >
> > > > > +}
> > > >
> > > > So maybe something lke:
> > > >
> > > > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > > > lock_is_held(&rcu_sched_lock_map)))
> > > > return true;
> > >
> > > Agreed, I will do it this way (without the debug_locks) like:
> > >
> > > ---8<-----------------------
> > >
> > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > index ba861d1716d3..339aebc330db 100644
> > > --- a/kernel/rcu/update.c
> > > +++ b/kernel/rcu/update.c
> > > @@ -296,27 +296,15 @@ 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();
> > > + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
> >
> > OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
>
> Hmm, I was borrowing the strategy from rcu_read_lock_bh_held() which does not
> check for a lock held in this map.
>
> Honestly, even lock_is_held(&rcu_sched_lock_map) seems unnecessary per-se
> since !preemptible() will catch that? rcu_read_lock_sched() disables
> preemption already, so lockdep's opinion of the matter seems redundant there.
Good point! At least as long as the lockdep splats list RCU-bh among
the locks held, which they did last I checked.
Of course, you could make the same argument for getting rid of
rcu_sched_lock_map. Does it make sense to have the one without
the other?
> Sorry I already sent out patches again before seeing your comment but I can
> rework and resend them based on any other suggestions.
Not a problem!
Thax, Paul
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-12 17:46 ` Paul E. McKenney
@ 2019-07-12 19:40 ` Joel Fernandes
2019-07-12 23:27 ` Paul E. McKenney
0 siblings, 1 reply; 22+ messages in thread
From: Joel Fernandes @ 2019-07-12 19:40 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Peter Zijlstra, linux-kernel, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, 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, Pavel Machek,
Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Fri, Jul 12, 2019 at 10:46:30AM -0700, Paul E. McKenney wrote:
> On Fri, Jul 12, 2019 at 01:06:31PM -0400, Joel Fernandes wrote:
> > On Fri, Jul 12, 2019 at 09:45:31AM -0700, Paul E. McKenney wrote:
> > > On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> > > > On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > > > > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > > > > > +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))
> > > > >
> > > > > you forgot debug_locks here.
> > > >
> > > > Actually, it turns out debug_locks checking is not even needed. If
> > > > debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would not
> > > > get to this point.
> > > >
> > > > > > + return 1;
> > > > > > +
> > > > > > + /* BH flavor */
> > > > > > + if (in_softirq() || irqs_disabled())
> > > > >
> > > > > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > > > > condition is superfluous, see below.
> > > > >
> > > > > > + return 1;
> > > > > > +
> > > > > > + /* Sched flavor */
> > > > > > + if (debug_locks)
> > > > > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > > > > + return lockdep_opinion || !preemptible();
> > > > >
> > > > > that !preemptible() turns into:
> > > > >
> > > > > !(preempt_count()==0 && !irqs_disabled())
> > > > >
> > > > > which is:
> > > > >
> > > > > preempt_count() != 0 || irqs_disabled()
> > > > >
> > > > > and already includes irqs_disabled() and in_softirq().
> > > > >
> > > > > > +}
> > > > >
> > > > > So maybe something lke:
> > > > >
> > > > > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > > > > lock_is_held(&rcu_sched_lock_map)))
> > > > > return true;
> > > >
> > > > Agreed, I will do it this way (without the debug_locks) like:
> > > >
> > > > ---8<-----------------------
> > > >
> > > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > > index ba861d1716d3..339aebc330db 100644
> > > > --- a/kernel/rcu/update.c
> > > > +++ b/kernel/rcu/update.c
> > > > @@ -296,27 +296,15 @@ 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();
> > > > + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
> > >
> > > OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
> >
> > Hmm, I was borrowing the strategy from rcu_read_lock_bh_held() which does not
> > check for a lock held in this map.
> >
> > Honestly, even lock_is_held(&rcu_sched_lock_map) seems unnecessary per-se
> > since !preemptible() will catch that? rcu_read_lock_sched() disables
> > preemption already, so lockdep's opinion of the matter seems redundant there.
>
> Good point! At least as long as the lockdep splats list RCU-bh among
> the locks held, which they did last I checked.
>
> Of course, you could make the same argument for getting rid of
> rcu_sched_lock_map. Does it make sense to have the one without
> the other?
It probably makes it inconsistent in the least. I will add the check for
the rcu_bh_lock_map in a separate patch, if that's Ok with you - since I also
want to update the rcu_read_lock_bh_held() logic in the same patch.
That rcu_read_lock_bh_held() could also just return !preemptible as Peter
suggested for the bh case.
> > Sorry I already sent out patches again before seeing your comment but I can
> > rework and resend them based on any other suggestions.
>
> Not a problem!
Thanks. Depending on whether there is any other feedback, I will work on the
bh_ stuff as a separate patch on top of this series, or work it into the next
series revision if I'm reposting. Hopefully that sounds Ok to you.
thanks,
- Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-12 19:40 ` Joel Fernandes
@ 2019-07-12 23:27 ` Paul E. McKenney
0 siblings, 0 replies; 22+ messages in thread
From: Paul E. McKenney @ 2019-07-12 23:27 UTC (permalink / raw)
To: Joel Fernandes
Cc: Peter Zijlstra, linux-kernel, Alexey Kuznetsov, Bjorn Helgaas,
Borislav Petkov, c0d1n61at3, 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, Pavel Machek,
Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Fri, Jul 12, 2019 at 03:40:40PM -0400, Joel Fernandes wrote:
> On Fri, Jul 12, 2019 at 10:46:30AM -0700, Paul E. McKenney wrote:
> > On Fri, Jul 12, 2019 at 01:06:31PM -0400, Joel Fernandes wrote:
> > > On Fri, Jul 12, 2019 at 09:45:31AM -0700, Paul E. McKenney wrote:
> > > > On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> > > > > On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > > > > > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > +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))
> > > > > >
> > > > > > you forgot debug_locks here.
> > > > >
> > > > > Actually, it turns out debug_locks checking is not even needed. If
> > > > > debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would not
> > > > > get to this point.
> > > > >
> > > > > > > + return 1;
> > > > > > > +
> > > > > > > + /* BH flavor */
> > > > > > > + if (in_softirq() || irqs_disabled())
> > > > > >
> > > > > > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > > > > > condition is superfluous, see below.
> > > > > >
> > > > > > > + return 1;
> > > > > > > +
> > > > > > > + /* Sched flavor */
> > > > > > > + if (debug_locks)
> > > > > > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > > > > > + return lockdep_opinion || !preemptible();
> > > > > >
> > > > > > that !preemptible() turns into:
> > > > > >
> > > > > > !(preempt_count()==0 && !irqs_disabled())
> > > > > >
> > > > > > which is:
> > > > > >
> > > > > > preempt_count() != 0 || irqs_disabled()
> > > > > >
> > > > > > and already includes irqs_disabled() and in_softirq().
> > > > > >
> > > > > > > +}
> > > > > >
> > > > > > So maybe something lke:
> > > > > >
> > > > > > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > > > > > lock_is_held(&rcu_sched_lock_map)))
> > > > > > return true;
> > > > >
> > > > > Agreed, I will do it this way (without the debug_locks) like:
> > > > >
> > > > > ---8<-----------------------
> > > > >
> > > > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > > > index ba861d1716d3..339aebc330db 100644
> > > > > --- a/kernel/rcu/update.c
> > > > > +++ b/kernel/rcu/update.c
> > > > > @@ -296,27 +296,15 @@ 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();
> > > > > + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
> > > >
> > > > OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
> > >
> > > Hmm, I was borrowing the strategy from rcu_read_lock_bh_held() which does not
> > > check for a lock held in this map.
> > >
> > > Honestly, even lock_is_held(&rcu_sched_lock_map) seems unnecessary per-se
> > > since !preemptible() will catch that? rcu_read_lock_sched() disables
> > > preemption already, so lockdep's opinion of the matter seems redundant there.
> >
> > Good point! At least as long as the lockdep splats list RCU-bh among
> > the locks held, which they did last I checked.
> >
> > Of course, you could make the same argument for getting rid of
> > rcu_sched_lock_map. Does it make sense to have the one without
> > the other?
>
> It probably makes it inconsistent in the least. I will add the check for
> the rcu_bh_lock_map in a separate patch, if that's Ok with you - since I also
> want to update the rcu_read_lock_bh_held() logic in the same patch.
>
> That rcu_read_lock_bh_held() could also just return !preemptible as Peter
> suggested for the bh case.
Although that seems reasonable, please check the call sites.
> > > Sorry I already sent out patches again before seeing your comment but I can
> > > rework and resend them based on any other suggestions.
> >
> > Not a problem!
>
> Thanks. Depending on whether there is any other feedback, I will work on the
> bh_ stuff as a separate patch on top of this series, or work it into the next
> series revision if I'm reposting. Hopefully that sounds Ok to you.
Agreed -- let's separate concerns. And promote bisectability.
Thanx, Paul
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-11 23:43 ` [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
` (2 preceding siblings ...)
2019-07-12 11:11 ` Peter Zijlstra
@ 2019-07-12 12:12 ` Oleg Nesterov
2019-07-12 13:56 ` Joel Fernandes
3 siblings, 1 reply; 22+ messages in thread
From: Oleg Nesterov @ 2019-07-12 12:12 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, Josh Triplett,
keescook, kernel-hardening, Lai Jiangshan, Len Brown, linux-acpi,
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)
On 07/11, Joel Fernandes (Google) wrote:
>
> +int rcu_read_lock_any_held(void)
rcu_sync_is_idle() wants it. You have my ack in advance ;)
Oleg.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
2019-07-12 12:12 ` Oleg Nesterov
@ 2019-07-12 13:56 ` Joel Fernandes
0 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes @ 2019-07-12 13:56 UTC (permalink / raw)
To: Oleg Nesterov
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, 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,
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 Fri, Jul 12, 2019 at 02:12:00PM +0200, Oleg Nesterov wrote:
> On 07/11, Joel Fernandes (Google) wrote:
> >
> > +int rcu_read_lock_any_held(void)
>
> rcu_sync_is_idle() wants it. You have my ack in advance ;)
Cool, thanks ;)
- Joel
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v1 2/6] ipv4: add lockdep condition to fix for_each_entry
2019-07-11 23:43 [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
2019-07-11 23:43 ` [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
@ 2019-07-11 23:43 ` Joel Fernandes (Google)
2019-07-11 23:43 ` [PATCH v1 3/6] driver/core: Convert to use built-in RCU list checking Joel Fernandes (Google)
` (4 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-11 23:43 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, 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,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
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.410.gd8fdbe21b5-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 3/6] driver/core: Convert to use built-in RCU list checking
2019-07-11 23:43 [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
2019-07-11 23:43 ` [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking Joel Fernandes (Google)
2019-07-11 23:43 ` [PATCH v1 2/6] ipv4: add lockdep condition to fix for_each_entry Joel Fernandes (Google)
@ 2019-07-11 23:43 ` Joel Fernandes (Google)
2019-07-12 5:19 ` Greg Kroah-Hartman
2019-07-11 23:43 ` [PATCH v1 4/6] workqueue: Convert for_each_wq to use built-in list check Joel Fernandes (Google)
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-11 23:43 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, 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,
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.
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.410.gd8fdbe21b5-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v1 3/6] driver/core: Convert to use built-in RCU list checking
2019-07-11 23:43 ` [PATCH v1 3/6] driver/core: Convert to use built-in RCU list checking Joel Fernandes (Google)
@ 2019-07-12 5:19 ` Greg Kroah-Hartman
0 siblings, 0 replies; 22+ messages in thread
From: Greg Kroah-Hartman @ 2019-07-12 5:19 UTC (permalink / raw)
To: Joel Fernandes (Google)
Cc: linux-kernel, Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov,
c0d1n61at3, David S. Miller, edumazet, 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,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Thu, Jul 11, 2019 at 07:43:58PM -0400, Joel Fernandes (Google) wrote:
> 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(-)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v1 4/6] workqueue: Convert for_each_wq to use built-in list check
2019-07-11 23:43 [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
` (2 preceding siblings ...)
2019-07-11 23:43 ` [PATCH v1 3/6] driver/core: Convert to use built-in RCU list checking Joel Fernandes (Google)
@ 2019-07-11 23:43 ` Joel Fernandes (Google)
2019-07-11 23:44 ` [PATCH v1 5/6] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator Joel Fernandes (Google)
` (2 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-11 23:43 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, 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,
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 | 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.410.gd8fdbe21b5-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 5/6] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator
2019-07-11 23:43 [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
` (3 preceding siblings ...)
2019-07-11 23:43 ` [PATCH v1 4/6] workqueue: Convert for_each_wq to use built-in list check Joel Fernandes (Google)
@ 2019-07-11 23:44 ` Joel Fernandes (Google)
2019-07-11 23:44 ` [PATCH v1 6/6] acpi: Use built-in RCU list checking for acpi_ioremaps list Joel Fernandes (Google)
2019-07-11 23:52 ` [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes
6 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-11 23:44 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, 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,
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.410.gd8fdbe21b5-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 6/6] acpi: Use built-in RCU list checking for acpi_ioremaps list
2019-07-11 23:43 [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
` (4 preceding siblings ...)
2019-07-11 23:44 ` [PATCH v1 5/6] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator Joel Fernandes (Google)
@ 2019-07-11 23:44 ` Joel Fernandes (Google)
2019-07-11 23:52 ` [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes
6 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes (Google) @ 2019-07-11 23:44 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, 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,
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 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.410.gd8fdbe21b5-goog
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family
2019-07-11 23:43 [PATCH v1 0/6] Harden list_for_each_entry_rcu() and family Joel Fernandes (Google)
` (5 preceding siblings ...)
2019-07-11 23:44 ` [PATCH v1 6/6] acpi: Use built-in RCU list checking for acpi_ioremaps list Joel Fernandes (Google)
@ 2019-07-11 23:52 ` Joel Fernandes
6 siblings, 0 replies; 22+ messages in thread
From: Joel Fernandes @ 2019-07-11 23:52 UTC (permalink / raw)
To: linux-kernel
Cc: Alexey Kuznetsov, Bjorn Helgaas, Borislav Petkov, c0d1n61at3,
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,
Rasmus Villemoes, rcu, Steven Rostedt, Tejun Heo,
Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
On Thu, Jul 11, 2019 at 07:43:55PM -0400, Joel Fernandes (Google) wrote:
> Hi,
> 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_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.
I forgot to add in my cover letter, 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.
Also, I plans to update the RCU documentation as well which I will do, but do
review this series and thank you!
^ permalink raw reply [flat|nested] 22+ messages in thread