[bionic, cosmic][PATCH 0/2, 0/1] srcu: Lock srcu_data structure in srcu_gp_start()

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[bionic, cosmic][PATCH 0/2, 0/1] srcu: Lock srcu_data structure in srcu_gp_start()

Marcelo Henrique Cerri
BugLink: http://bugs.launchpad.net/bugs/1802021

comic only requires one of the patches.

Dennis Krein (1):
  srcu: Lock srcu_data structure in srcu_gp_start()

Paul E. McKenney (1):
  srcu: Prohibit call_srcu() use under raw spinlocks

 include/linux/srcutree.h |   8 +--
 kernel/rcu/srcutree.c    | 111 ++++++++++++++++++++++++---------------
 2 files changed, 74 insertions(+), 45 deletions(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[bionic][PATCH 1/2] srcu: Prohibit call_srcu() use under raw spinlocks

Marcelo Henrique Cerri
From: Paul E. McKenney <[hidden email]>

Invoking queue_delayed_work() while holding a raw spinlock is forbidden
in -rt kernels, which is exactly what __call_srcu() does, indirectly via
srcu_funnel_gp_start().  This commit therefore downgrades Tree SRCU's
locking from raw to non-raw spinlocks, which works because call_srcu()
is not ever called while holding a raw spinlock.

Reported-by: Sebastian Andrzej Siewior <[hidden email]>
Signed-off-by: Paul E. McKenney <[hidden email]>
---
 include/linux/srcutree.h |   8 +--
 kernel/rcu/srcutree.c    | 109 ++++++++++++++++++++++++---------------
 2 files changed, 72 insertions(+), 45 deletions(-)

diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
index a949f4f9e4d7..4eda108abee0 100644
--- a/include/linux/srcutree.h
+++ b/include/linux/srcutree.h
@@ -40,7 +40,7 @@ struct srcu_data {
  unsigned long srcu_unlock_count[2]; /* Unlocks per CPU. */
 
  /* Update-side state. */
- raw_spinlock_t __private lock ____cacheline_internodealigned_in_smp;
+ spinlock_t __private lock ____cacheline_internodealigned_in_smp;
  struct rcu_segcblist srcu_cblist; /* List of callbacks.*/
  unsigned long srcu_gp_seq_needed; /* Furthest future GP needed. */
  unsigned long srcu_gp_seq_needed_exp; /* Furthest future exp GP. */
@@ -58,7 +58,7 @@ struct srcu_data {
  * Node in SRCU combining tree, similar in function to rcu_data.
  */
 struct srcu_node {
- raw_spinlock_t __private lock;
+ spinlock_t __private lock;
  unsigned long srcu_have_cbs[4]; /* GP seq for children */
  /*  having CBs, but only */
  /*  is > ->srcu_gq_seq. */
@@ -78,7 +78,7 @@ struct srcu_struct {
  struct srcu_node *level[RCU_NUM_LVLS + 1];
  /* First node at each level. */
  struct mutex srcu_cb_mutex; /* Serialize CB preparation. */
- raw_spinlock_t __private lock; /* Protect counters */
+ spinlock_t __private lock; /* Protect counters */
  struct mutex srcu_gp_mutex; /* Serialize GP work. */
  unsigned int srcu_idx; /* Current rdr array element. */
  unsigned long srcu_gp_seq; /* Grace-period seq #. */
@@ -107,7 +107,7 @@ struct srcu_struct {
 #define __SRCU_STRUCT_INIT(name) \
  { \
  .sda = &name##_srcu_data, \
- .lock = __RAW_SPIN_LOCK_UNLOCKED(name.lock), \
+ .lock = __SPIN_LOCK_UNLOCKED(name.lock), \
  .srcu_gp_seq_needed = 0 - 1, \
  __SRCU_DEP_MAP_INIT(name) \
  }
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index 6d5880089ff6..d5cea81378cc 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -53,6 +53,33 @@ static void srcu_invoke_callbacks(struct work_struct *work);
 static void srcu_reschedule(struct srcu_struct *sp, unsigned long delay);
 static void process_srcu(struct work_struct *work);
 
+/* Wrappers for lock acquisition and release, see raw_spin_lock_rcu_node(). */
+#define spin_lock_rcu_node(p) \
+do { \
+ spin_lock(&ACCESS_PRIVATE(p, lock)); \
+ smp_mb__after_unlock_lock(); \
+} while (0)
+
+#define spin_unlock_rcu_node(p) spin_unlock(&ACCESS_PRIVATE(p, lock))
+
+#define spin_lock_irq_rcu_node(p) \
+do { \
+ spin_lock_irq(&ACCESS_PRIVATE(p, lock)); \
+ smp_mb__after_unlock_lock(); \
+} while (0)
+
+#define spin_unlock_irq_rcu_node(p) \
+ spin_unlock_irq(&ACCESS_PRIVATE(p, lock))
+
+#define spin_lock_irqsave_rcu_node(p, flags) \
+do { \
+ spin_lock_irqsave(&ACCESS_PRIVATE(p, lock), flags); \
+ smp_mb__after_unlock_lock(); \
+} while (0)
+
+#define spin_unlock_irqrestore_rcu_node(p, flags) \
+ spin_unlock_irqrestore(&ACCESS_PRIVATE(p, lock), flags) \
+
 /*
  * Initialize SRCU combining tree.  Note that statically allocated
  * srcu_struct structures might already have srcu_read_lock() and
@@ -77,7 +104,7 @@ static void init_srcu_struct_nodes(struct srcu_struct *sp, bool is_static)
 
  /* Each pass through this loop initializes one srcu_node structure. */
  rcu_for_each_node_breadth_first(sp, snp) {
- raw_spin_lock_init(&ACCESS_PRIVATE(snp, lock));
+ spin_lock_init(&ACCESS_PRIVATE(snp, lock));
  WARN_ON_ONCE(ARRAY_SIZE(snp->srcu_have_cbs) !=
      ARRAY_SIZE(snp->srcu_data_have_cbs));
  for (i = 0; i < ARRAY_SIZE(snp->srcu_have_cbs); i++) {
@@ -111,7 +138,7 @@ static void init_srcu_struct_nodes(struct srcu_struct *sp, bool is_static)
  snp_first = sp->level[level];
  for_each_possible_cpu(cpu) {
  sdp = per_cpu_ptr(sp->sda, cpu);
- raw_spin_lock_init(&ACCESS_PRIVATE(sdp, lock));
+ spin_lock_init(&ACCESS_PRIVATE(sdp, lock));
  rcu_segcblist_init(&sdp->srcu_cblist);
  sdp->srcu_cblist_invoking = false;
  sdp->srcu_gp_seq_needed = sp->srcu_gp_seq;
@@ -170,7 +197,7 @@ int __init_srcu_struct(struct srcu_struct *sp, const char *name,
  /* Don't re-initialize a lock while it is held. */
  debug_check_no_locks_freed((void *)sp, sizeof(*sp));
  lockdep_init_map(&sp->dep_map, name, key, 0);
- raw_spin_lock_init(&ACCESS_PRIVATE(sp, lock));
+ spin_lock_init(&ACCESS_PRIVATE(sp, lock));
  return init_srcu_struct_fields(sp, false);
 }
 EXPORT_SYMBOL_GPL(__init_srcu_struct);
@@ -187,7 +214,7 @@ EXPORT_SYMBOL_GPL(__init_srcu_struct);
  */
 int init_srcu_struct(struct srcu_struct *sp)
 {
- raw_spin_lock_init(&ACCESS_PRIVATE(sp, lock));
+ spin_lock_init(&ACCESS_PRIVATE(sp, lock));
  return init_srcu_struct_fields(sp, false);
 }
 EXPORT_SYMBOL_GPL(init_srcu_struct);
@@ -210,13 +237,13 @@ static void check_init_srcu_struct(struct srcu_struct *sp)
  /* The smp_load_acquire() pairs with the smp_store_release(). */
  if (!rcu_seq_state(smp_load_acquire(&sp->srcu_gp_seq_needed))) /*^^^*/
  return; /* Already initialized. */
- raw_spin_lock_irqsave_rcu_node(sp, flags);
+ spin_lock_irqsave_rcu_node(sp, flags);
  if (!rcu_seq_state(sp->srcu_gp_seq_needed)) {
- raw_spin_unlock_irqrestore_rcu_node(sp, flags);
+ spin_unlock_irqrestore_rcu_node(sp, flags);
  return;
  }
  init_srcu_struct_fields(sp, true);
- raw_spin_unlock_irqrestore_rcu_node(sp, flags);
+ spin_unlock_irqrestore_rcu_node(sp, flags);
 }
 
 /*
@@ -513,7 +540,7 @@ static void srcu_gp_end(struct srcu_struct *sp)
  mutex_lock(&sp->srcu_cb_mutex);
 
  /* End the current grace period. */
- raw_spin_lock_irq_rcu_node(sp);
+ spin_lock_irq_rcu_node(sp);
  idx = rcu_seq_state(sp->srcu_gp_seq);
  WARN_ON_ONCE(idx != SRCU_STATE_SCAN2);
  cbdelay = srcu_get_delay(sp);
@@ -522,7 +549,7 @@ static void srcu_gp_end(struct srcu_struct *sp)
  gpseq = rcu_seq_current(&sp->srcu_gp_seq);
  if (ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, gpseq))
  sp->srcu_gp_seq_needed_exp = gpseq;
- raw_spin_unlock_irq_rcu_node(sp);
+ spin_unlock_irq_rcu_node(sp);
  mutex_unlock(&sp->srcu_gp_mutex);
  /* A new grace period can start at this point.  But only one. */
 
@@ -530,7 +557,7 @@ static void srcu_gp_end(struct srcu_struct *sp)
  idx = rcu_seq_ctr(gpseq) % ARRAY_SIZE(snp->srcu_have_cbs);
  idxnext = (idx + 1) % ARRAY_SIZE(snp->srcu_have_cbs);
  rcu_for_each_node_breadth_first(sp, snp) {
- raw_spin_lock_irq_rcu_node(snp);
+ spin_lock_irq_rcu_node(snp);
  cbs = false;
  if (snp >= sp->level[rcu_num_lvls - 1])
  cbs = snp->srcu_have_cbs[idx] == gpseq;
@@ -540,7 +567,7 @@ static void srcu_gp_end(struct srcu_struct *sp)
  snp->srcu_gp_seq_needed_exp = gpseq;
  mask = snp->srcu_data_have_cbs[idx];
  snp->srcu_data_have_cbs[idx] = 0;
- raw_spin_unlock_irq_rcu_node(snp);
+ spin_unlock_irq_rcu_node(snp);
  if (cbs)
  srcu_schedule_cbs_snp(sp, snp, mask, cbdelay);
 
@@ -548,11 +575,11 @@ static void srcu_gp_end(struct srcu_struct *sp)
  if (!(gpseq & counter_wrap_check))
  for (cpu = snp->grplo; cpu <= snp->grphi; cpu++) {
  sdp = per_cpu_ptr(sp->sda, cpu);
- raw_spin_lock_irqsave_rcu_node(sdp, flags);
+ spin_lock_irqsave_rcu_node(sdp, flags);
  if (ULONG_CMP_GE(gpseq,
  sdp->srcu_gp_seq_needed + 100))
  sdp->srcu_gp_seq_needed = gpseq;
- raw_spin_unlock_irqrestore_rcu_node(sdp, flags);
+ spin_unlock_irqrestore_rcu_node(sdp, flags);
  }
  }
 
@@ -560,17 +587,17 @@ static void srcu_gp_end(struct srcu_struct *sp)
  mutex_unlock(&sp->srcu_cb_mutex);
 
  /* Start a new grace period if needed. */
- raw_spin_lock_irq_rcu_node(sp);
+ spin_lock_irq_rcu_node(sp);
  gpseq = rcu_seq_current(&sp->srcu_gp_seq);
  if (!rcu_seq_state(gpseq) &&
     ULONG_CMP_LT(gpseq, sp->srcu_gp_seq_needed)) {
  srcu_gp_start(sp);
- raw_spin_unlock_irq_rcu_node(sp);
+ spin_unlock_irq_rcu_node(sp);
  /* Throttle expedited grace periods: Should be rare! */
  srcu_reschedule(sp, rcu_seq_ctr(gpseq) & 0x3ff
     ? 0 : SRCU_INTERVAL);
  } else {
- raw_spin_unlock_irq_rcu_node(sp);
+ spin_unlock_irq_rcu_node(sp);
  }
 }
 
@@ -590,18 +617,18 @@ static void srcu_funnel_exp_start(struct srcu_struct *sp, struct srcu_node *snp,
  if (rcu_seq_done(&sp->srcu_gp_seq, s) ||
     ULONG_CMP_GE(READ_ONCE(snp->srcu_gp_seq_needed_exp), s))
  return;
- raw_spin_lock_irqsave_rcu_node(snp, flags);
+ spin_lock_irqsave_rcu_node(snp, flags);
  if (ULONG_CMP_GE(snp->srcu_gp_seq_needed_exp, s)) {
- raw_spin_unlock_irqrestore_rcu_node(snp, flags);
+ spin_unlock_irqrestore_rcu_node(snp, flags);
  return;
  }
  WRITE_ONCE(snp->srcu_gp_seq_needed_exp, s);
- raw_spin_unlock_irqrestore_rcu_node(snp, flags);
+ spin_unlock_irqrestore_rcu_node(snp, flags);
  }
- raw_spin_lock_irqsave_rcu_node(sp, flags);
+ spin_lock_irqsave_rcu_node(sp, flags);
  if (!ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, s))
  sp->srcu_gp_seq_needed_exp = s;
- raw_spin_unlock_irqrestore_rcu_node(sp, flags);
+ spin_unlock_irqrestore_rcu_node(sp, flags);
 }
 
 /*
@@ -623,12 +650,12 @@ static void srcu_funnel_gp_start(struct srcu_struct *sp, struct srcu_data *sdp,
  for (; snp != NULL; snp = snp->srcu_parent) {
  if (rcu_seq_done(&sp->srcu_gp_seq, s) && snp != sdp->mynode)
  return; /* GP already done and CBs recorded. */
- raw_spin_lock_irqsave_rcu_node(snp, flags);
+ spin_lock_irqsave_rcu_node(snp, flags);
  if (ULONG_CMP_GE(snp->srcu_have_cbs[idx], s)) {
  snp_seq = snp->srcu_have_cbs[idx];
  if (snp == sdp->mynode && snp_seq == s)
  snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
- raw_spin_unlock_irqrestore_rcu_node(snp, flags);
+ spin_unlock_irqrestore_rcu_node(snp, flags);
  if (snp == sdp->mynode && snp_seq != s) {
  srcu_schedule_cbs_sdp(sdp, do_norm
    ? SRCU_INTERVAL
@@ -644,11 +671,11 @@ static void srcu_funnel_gp_start(struct srcu_struct *sp, struct srcu_data *sdp,
  snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
  if (!do_norm && ULONG_CMP_LT(snp->srcu_gp_seq_needed_exp, s))
  snp->srcu_gp_seq_needed_exp = s;
- raw_spin_unlock_irqrestore_rcu_node(snp, flags);
+ spin_unlock_irqrestore_rcu_node(snp, flags);
  }
 
  /* Top of tree, must ensure the grace period will be started. */
- raw_spin_lock_irqsave_rcu_node(sp, flags);
+ spin_lock_irqsave_rcu_node(sp, flags);
  if (ULONG_CMP_LT(sp->srcu_gp_seq_needed, s)) {
  /*
  * Record need for grace period s.  Pair with load
@@ -667,7 +694,7 @@ static void srcu_funnel_gp_start(struct srcu_struct *sp, struct srcu_data *sdp,
  queue_delayed_work(system_power_efficient_wq, &sp->work,
    srcu_get_delay(sp));
  }
- raw_spin_unlock_irqrestore_rcu_node(sp, flags);
+ spin_unlock_irqrestore_rcu_node(sp, flags);
 }
 
 /*
@@ -830,7 +857,7 @@ void __call_srcu(struct srcu_struct *sp, struct rcu_head *rhp,
  rhp->func = func;
  local_irq_save(flags);
  sdp = this_cpu_ptr(sp->sda);
- raw_spin_lock_rcu_node(sdp);
+ spin_lock_rcu_node(sdp);
  rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp, false);
  rcu_segcblist_advance(&sdp->srcu_cblist,
       rcu_seq_current(&sp->srcu_gp_seq));
@@ -844,7 +871,7 @@ void __call_srcu(struct srcu_struct *sp, struct rcu_head *rhp,
  sdp->srcu_gp_seq_needed_exp = s;
  needexp = true;
  }
- raw_spin_unlock_irqrestore_rcu_node(sdp, flags);
+ spin_unlock_irqrestore_rcu_node(sdp, flags);
  if (needgp)
  srcu_funnel_gp_start(sp, sdp, s, do_norm);
  else if (needexp)
@@ -900,7 +927,7 @@ static void __synchronize_srcu(struct srcu_struct *sp, bool do_norm)
 
  /*
  * Make sure that later code is ordered after the SRCU grace
- * period.  This pairs with the raw_spin_lock_irq_rcu_node()
+ * period.  This pairs with the spin_lock_irq_rcu_node()
  * in srcu_invoke_callbacks().  Unlike Tree RCU, this is needed
  * because the current CPU might have been totally uninvolved with
  * (and thus unordered against) that grace period.
@@ -1024,7 +1051,7 @@ void srcu_barrier(struct srcu_struct *sp)
  */
  for_each_possible_cpu(cpu) {
  sdp = per_cpu_ptr(sp->sda, cpu);
- raw_spin_lock_irq_rcu_node(sdp);
+ spin_lock_irq_rcu_node(sdp);
  atomic_inc(&sp->srcu_barrier_cpu_cnt);
  sdp->srcu_barrier_head.func = srcu_barrier_cb;
  debug_rcu_head_queue(&sdp->srcu_barrier_head);
@@ -1033,7 +1060,7 @@ void srcu_barrier(struct srcu_struct *sp)
  debug_rcu_head_unqueue(&sdp->srcu_barrier_head);
  atomic_dec(&sp->srcu_barrier_cpu_cnt);
  }
- raw_spin_unlock_irq_rcu_node(sdp);
+ spin_unlock_irq_rcu_node(sdp);
  }
 
  /* Remove the initial count, at which point reaching zero can happen. */
@@ -1082,17 +1109,17 @@ static void srcu_advance_state(struct srcu_struct *sp)
  */
  idx = rcu_seq_state(smp_load_acquire(&sp->srcu_gp_seq)); /* ^^^ */
  if (idx == SRCU_STATE_IDLE) {
- raw_spin_lock_irq_rcu_node(sp);
+ spin_lock_irq_rcu_node(sp);
  if (ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed)) {
  WARN_ON_ONCE(rcu_seq_state(sp->srcu_gp_seq));
- raw_spin_unlock_irq_rcu_node(sp);
+ spin_unlock_irq_rcu_node(sp);
  mutex_unlock(&sp->srcu_gp_mutex);
  return;
  }
  idx = rcu_seq_state(READ_ONCE(sp->srcu_gp_seq));
  if (idx == SRCU_STATE_IDLE)
  srcu_gp_start(sp);
- raw_spin_unlock_irq_rcu_node(sp);
+ spin_unlock_irq_rcu_node(sp);
  if (idx != SRCU_STATE_IDLE) {
  mutex_unlock(&sp->srcu_gp_mutex);
  return; /* Someone else started the grace period. */
@@ -1141,19 +1168,19 @@ static void srcu_invoke_callbacks(struct work_struct *work)
  sdp = container_of(work, struct srcu_data, work.work);
  sp = sdp->sp;
  rcu_cblist_init(&ready_cbs);
- raw_spin_lock_irq_rcu_node(sdp);
+ spin_lock_irq_rcu_node(sdp);
  rcu_segcblist_advance(&sdp->srcu_cblist,
       rcu_seq_current(&sp->srcu_gp_seq));
  if (sdp->srcu_cblist_invoking ||
     !rcu_segcblist_ready_cbs(&sdp->srcu_cblist)) {
- raw_spin_unlock_irq_rcu_node(sdp);
+ spin_unlock_irq_rcu_node(sdp);
  return;  /* Someone else on the job or nothing to do. */
  }
 
  /* We are on the job!  Extract and invoke ready callbacks. */
  sdp->srcu_cblist_invoking = true;
  rcu_segcblist_extract_done_cbs(&sdp->srcu_cblist, &ready_cbs);
- raw_spin_unlock_irq_rcu_node(sdp);
+ spin_unlock_irq_rcu_node(sdp);
  rhp = rcu_cblist_dequeue(&ready_cbs);
  for (; rhp != NULL; rhp = rcu_cblist_dequeue(&ready_cbs)) {
  debug_rcu_head_unqueue(rhp);
@@ -1166,13 +1193,13 @@ static void srcu_invoke_callbacks(struct work_struct *work)
  * Update counts, accelerate new callbacks, and if needed,
  * schedule another round of callback invocation.
  */
- raw_spin_lock_irq_rcu_node(sdp);
+ spin_lock_irq_rcu_node(sdp);
  rcu_segcblist_insert_count(&sdp->srcu_cblist, &ready_cbs);
  (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
        rcu_seq_snap(&sp->srcu_gp_seq));
  sdp->srcu_cblist_invoking = false;
  more = rcu_segcblist_ready_cbs(&sdp->srcu_cblist);
- raw_spin_unlock_irq_rcu_node(sdp);
+ spin_unlock_irq_rcu_node(sdp);
  if (more)
  srcu_schedule_cbs_sdp(sdp, 0);
 }
@@ -1185,7 +1212,7 @@ static void srcu_reschedule(struct srcu_struct *sp, unsigned long delay)
 {
  bool pushgp = true;
 
- raw_spin_lock_irq_rcu_node(sp);
+ spin_lock_irq_rcu_node(sp);
  if (ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed)) {
  if (!WARN_ON_ONCE(rcu_seq_state(sp->srcu_gp_seq))) {
  /* All requests fulfilled, time to go idle. */
@@ -1195,7 +1222,7 @@ static void srcu_reschedule(struct srcu_struct *sp, unsigned long delay)
  /* Outstanding request and no GP.  Start one. */
  srcu_gp_start(sp);
  }
- raw_spin_unlock_irq_rcu_node(sp);
+ spin_unlock_irq_rcu_node(sp);
 
  if (pushgp)
  queue_delayed_work(system_power_efficient_wq, &sp->work, delay);
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[bionic][PATCH 2/2] srcu: Lock srcu_data structure in srcu_gp_start()

Marcelo Henrique Cerri
In reply to this post by Marcelo Henrique Cerri
From: Dennis Krein <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1802021

The srcu_gp_start() function is called with the srcu_struct structure's
->lock held, but not with the srcu_data structure's ->lock.  This is
problematic because this function accesses and updates the srcu_data
structure's ->srcu_cblist, which is protected by that lock.  Failing to
hold this lock can result in corruption of the SRCU callback lists,
which in turn can result in arbitrarily bad results.

This commit therefore makes srcu_gp_start() acquire the srcu_data
structure's ->lock across the calls to rcu_segcblist_advance() and
rcu_segcblist_accelerate(), thus preventing this corruption.

Reported-by: Bart Van Assche <[hidden email]>
Reported-by: Christoph Hellwig <[hidden email]>
Reported-by: Sebastian Kuzminsky <[hidden email]>
Signed-off-by: Dennis Krein <[hidden email]>
Signed-off-by: Paul E. McKenney <[hidden email]>
Tested-by: Dennis Krein <[hidden email]>
Cc: <[hidden email]> # 4.16.x
(cherry picked from commit eb4c2382272ae7ae5d81fdfa5b7a6c86146eaaa4)
Acked-by: Stefan Bader <[hidden email]>
Acked-by: Kleber Sacilotto de Souza <[hidden email]>
Signed-off-by: Marcelo Henrique Cerri <[hidden email]>
---
 kernel/rcu/srcutree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index d5cea81378cc..b3e5e9873582 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -441,10 +441,12 @@ static void srcu_gp_start(struct srcu_struct *sp)
 
  lockdep_assert_held(&sp->lock);
  WARN_ON_ONCE(ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed));
+ spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
  rcu_segcblist_advance(&sdp->srcu_cblist,
       rcu_seq_current(&sp->srcu_gp_seq));
  (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
        rcu_seq_snap(&sp->srcu_gp_seq));
+ spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
  smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
  rcu_seq_start(&sp->srcu_gp_seq);
  state = rcu_seq_state(READ_ONCE(sp->srcu_gp_seq));
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[cosmic][PATCH] srcu: Lock srcu_data structure in srcu_gp_start()

Marcelo Henrique Cerri
In reply to this post by Marcelo Henrique Cerri
From: Dennis Krein <[hidden email]>

BugLink: http://bugs.launchpad.net/bugs/1802021

The srcu_gp_start() function is called with the srcu_struct structure's
->lock held, but not with the srcu_data structure's ->lock.  This is
problematic because this function accesses and updates the srcu_data
structure's ->srcu_cblist, which is protected by that lock.  Failing to
hold this lock can result in corruption of the SRCU callback lists,
which in turn can result in arbitrarily bad results.

This commit therefore makes srcu_gp_start() acquire the srcu_data
structure's ->lock across the calls to rcu_segcblist_advance() and
rcu_segcblist_accelerate(), thus preventing this corruption.

Reported-by: Bart Van Assche <[hidden email]>
Reported-by: Christoph Hellwig <[hidden email]>
Reported-by: Sebastian Kuzminsky <[hidden email]>
Signed-off-by: Dennis Krein <[hidden email]>
Signed-off-by: Paul E. McKenney <[hidden email]>
Tested-by: Dennis Krein <[hidden email]>
Cc: <[hidden email]> # 4.16.x
(cherry picked from commit eb4c2382272ae7ae5d81fdfa5b7a6c86146eaaa4)
Acked-by: Stefan Bader <[hidden email]>
Acked-by: Kleber Sacilotto de Souza <[hidden email]>
Signed-off-by: Marcelo Henrique Cerri <[hidden email]>
---
 kernel/rcu/srcutree.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
index b4123d7a2cec..bd0fad13e6a9 100644
--- a/kernel/rcu/srcutree.c
+++ b/kernel/rcu/srcutree.c
@@ -445,10 +445,12 @@ static void srcu_gp_start(struct srcu_struct *sp)
 
  lockdep_assert_held(&ACCESS_PRIVATE(sp, lock));
  WARN_ON_ONCE(ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed));
+ spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
  rcu_segcblist_advance(&sdp->srcu_cblist,
       rcu_seq_current(&sp->srcu_gp_seq));
  (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
        rcu_seq_snap(&sp->srcu_gp_seq));
+ spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
  smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
  rcu_seq_start(&sp->srcu_gp_seq);
  state = rcu_seq_state(READ_ONCE(sp->srcu_gp_seq));
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [bionic][PATCH 1/2] srcu: Prohibit call_srcu() use under raw spinlocks

Thadeu Lima de Souza Cascardo-3
In reply to this post by Marcelo Henrique Cerri
On Wed, Feb 20, 2019 at 08:28:36PM -0300, Marcelo Henrique Cerri wrote:
> From: Paul E. McKenney <[hidden email]>

No BugLink, cherry-picked from or SOB.

>
> Invoking queue_delayed_work() while holding a raw spinlock is forbidden
> in -rt kernels, which is exactly what __call_srcu() does, indirectly via
> srcu_funnel_gp_start().  This commit therefore downgrades Tree SRCU's
> locking from raw to non-raw spinlocks, which works because call_srcu()
> is not ever called while holding a raw spinlock.
>
> Reported-by: Sebastian Andrzej Siewior <[hidden email]>
> Signed-off-by: Paul E. McKenney <[hidden email]>
> ---
>  include/linux/srcutree.h |   8 +--
>  kernel/rcu/srcutree.c    | 109 ++++++++++++++++++++++++---------------
>  2 files changed, 72 insertions(+), 45 deletions(-)
>
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index a949f4f9e4d7..4eda108abee0 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -40,7 +40,7 @@ struct srcu_data {
>   unsigned long srcu_unlock_count[2]; /* Unlocks per CPU. */
>  
>   /* Update-side state. */
> - raw_spinlock_t __private lock ____cacheline_internodealigned_in_smp;
> + spinlock_t __private lock ____cacheline_internodealigned_in_smp;
>   struct rcu_segcblist srcu_cblist; /* List of callbacks.*/
>   unsigned long srcu_gp_seq_needed; /* Furthest future GP needed. */
>   unsigned long srcu_gp_seq_needed_exp; /* Furthest future exp GP. */
> @@ -58,7 +58,7 @@ struct srcu_data {
>   * Node in SRCU combining tree, similar in function to rcu_data.
>   */
>  struct srcu_node {
> - raw_spinlock_t __private lock;
> + spinlock_t __private lock;
>   unsigned long srcu_have_cbs[4]; /* GP seq for children */
>   /*  having CBs, but only */
>   /*  is > ->srcu_gq_seq. */
> @@ -78,7 +78,7 @@ struct srcu_struct {
>   struct srcu_node *level[RCU_NUM_LVLS + 1];
>   /* First node at each level. */
>   struct mutex srcu_cb_mutex; /* Serialize CB preparation. */
> - raw_spinlock_t __private lock; /* Protect counters */
> + spinlock_t __private lock; /* Protect counters */
>   struct mutex srcu_gp_mutex; /* Serialize GP work. */
>   unsigned int srcu_idx; /* Current rdr array element. */
>   unsigned long srcu_gp_seq; /* Grace-period seq #. */
> @@ -107,7 +107,7 @@ struct srcu_struct {
>  #define __SRCU_STRUCT_INIT(name) \
>   { \
>   .sda = &name##_srcu_data, \
> - .lock = __RAW_SPIN_LOCK_UNLOCKED(name.lock), \
> + .lock = __SPIN_LOCK_UNLOCKED(name.lock), \
>   .srcu_gp_seq_needed = 0 - 1, \
>   __SRCU_DEP_MAP_INIT(name) \
>   }
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 6d5880089ff6..d5cea81378cc 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -53,6 +53,33 @@ static void srcu_invoke_callbacks(struct work_struct *work);
>  static void srcu_reschedule(struct srcu_struct *sp, unsigned long delay);
>  static void process_srcu(struct work_struct *work);
>  
> +/* Wrappers for lock acquisition and release, see raw_spin_lock_rcu_node(). */
> +#define spin_lock_rcu_node(p) \
> +do { \
> + spin_lock(&ACCESS_PRIVATE(p, lock)); \
> + smp_mb__after_unlock_lock(); \
> +} while (0)
> +
> +#define spin_unlock_rcu_node(p) spin_unlock(&ACCESS_PRIVATE(p, lock))
> +
> +#define spin_lock_irq_rcu_node(p) \
> +do { \
> + spin_lock_irq(&ACCESS_PRIVATE(p, lock)); \
> + smp_mb__after_unlock_lock(); \
> +} while (0)
> +
> +#define spin_unlock_irq_rcu_node(p) \
> + spin_unlock_irq(&ACCESS_PRIVATE(p, lock))
> +
> +#define spin_lock_irqsave_rcu_node(p, flags) \
> +do { \
> + spin_lock_irqsave(&ACCESS_PRIVATE(p, lock), flags); \
> + smp_mb__after_unlock_lock(); \
> +} while (0)
> +
> +#define spin_unlock_irqrestore_rcu_node(p, flags) \
> + spin_unlock_irqrestore(&ACCESS_PRIVATE(p, lock), flags) \
> +
>  /*
>   * Initialize SRCU combining tree.  Note that statically allocated
>   * srcu_struct structures might already have srcu_read_lock() and
> @@ -77,7 +104,7 @@ static void init_srcu_struct_nodes(struct srcu_struct *sp, bool is_static)
>  
>   /* Each pass through this loop initializes one srcu_node structure. */
>   rcu_for_each_node_breadth_first(sp, snp) {
> - raw_spin_lock_init(&ACCESS_PRIVATE(snp, lock));
> + spin_lock_init(&ACCESS_PRIVATE(snp, lock));
>   WARN_ON_ONCE(ARRAY_SIZE(snp->srcu_have_cbs) !=
>       ARRAY_SIZE(snp->srcu_data_have_cbs));
>   for (i = 0; i < ARRAY_SIZE(snp->srcu_have_cbs); i++) {
> @@ -111,7 +138,7 @@ static void init_srcu_struct_nodes(struct srcu_struct *sp, bool is_static)
>   snp_first = sp->level[level];
>   for_each_possible_cpu(cpu) {
>   sdp = per_cpu_ptr(sp->sda, cpu);
> - raw_spin_lock_init(&ACCESS_PRIVATE(sdp, lock));
> + spin_lock_init(&ACCESS_PRIVATE(sdp, lock));
>   rcu_segcblist_init(&sdp->srcu_cblist);
>   sdp->srcu_cblist_invoking = false;
>   sdp->srcu_gp_seq_needed = sp->srcu_gp_seq;
> @@ -170,7 +197,7 @@ int __init_srcu_struct(struct srcu_struct *sp, const char *name,
>   /* Don't re-initialize a lock while it is held. */
>   debug_check_no_locks_freed((void *)sp, sizeof(*sp));
>   lockdep_init_map(&sp->dep_map, name, key, 0);
> - raw_spin_lock_init(&ACCESS_PRIVATE(sp, lock));
> + spin_lock_init(&ACCESS_PRIVATE(sp, lock));
>   return init_srcu_struct_fields(sp, false);
>  }
>  EXPORT_SYMBOL_GPL(__init_srcu_struct);
> @@ -187,7 +214,7 @@ EXPORT_SYMBOL_GPL(__init_srcu_struct);
>   */
>  int init_srcu_struct(struct srcu_struct *sp)
>  {
> - raw_spin_lock_init(&ACCESS_PRIVATE(sp, lock));
> + spin_lock_init(&ACCESS_PRIVATE(sp, lock));
>   return init_srcu_struct_fields(sp, false);
>  }
>  EXPORT_SYMBOL_GPL(init_srcu_struct);
> @@ -210,13 +237,13 @@ static void check_init_srcu_struct(struct srcu_struct *sp)
>   /* The smp_load_acquire() pairs with the smp_store_release(). */
>   if (!rcu_seq_state(smp_load_acquire(&sp->srcu_gp_seq_needed))) /*^^^*/
>   return; /* Already initialized. */
> - raw_spin_lock_irqsave_rcu_node(sp, flags);
> + spin_lock_irqsave_rcu_node(sp, flags);
>   if (!rcu_seq_state(sp->srcu_gp_seq_needed)) {
> - raw_spin_unlock_irqrestore_rcu_node(sp, flags);
> + spin_unlock_irqrestore_rcu_node(sp, flags);
>   return;
>   }
>   init_srcu_struct_fields(sp, true);
> - raw_spin_unlock_irqrestore_rcu_node(sp, flags);
> + spin_unlock_irqrestore_rcu_node(sp, flags);
>  }
>  
>  /*
> @@ -513,7 +540,7 @@ static void srcu_gp_end(struct srcu_struct *sp)
>   mutex_lock(&sp->srcu_cb_mutex);
>  
>   /* End the current grace period. */
> - raw_spin_lock_irq_rcu_node(sp);
> + spin_lock_irq_rcu_node(sp);
>   idx = rcu_seq_state(sp->srcu_gp_seq);
>   WARN_ON_ONCE(idx != SRCU_STATE_SCAN2);
>   cbdelay = srcu_get_delay(sp);
> @@ -522,7 +549,7 @@ static void srcu_gp_end(struct srcu_struct *sp)
>   gpseq = rcu_seq_current(&sp->srcu_gp_seq);
>   if (ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, gpseq))
>   sp->srcu_gp_seq_needed_exp = gpseq;
> - raw_spin_unlock_irq_rcu_node(sp);
> + spin_unlock_irq_rcu_node(sp);
>   mutex_unlock(&sp->srcu_gp_mutex);
>   /* A new grace period can start at this point.  But only one. */
>  
> @@ -530,7 +557,7 @@ static void srcu_gp_end(struct srcu_struct *sp)
>   idx = rcu_seq_ctr(gpseq) % ARRAY_SIZE(snp->srcu_have_cbs);
>   idxnext = (idx + 1) % ARRAY_SIZE(snp->srcu_have_cbs);
>   rcu_for_each_node_breadth_first(sp, snp) {
> - raw_spin_lock_irq_rcu_node(snp);
> + spin_lock_irq_rcu_node(snp);
>   cbs = false;
>   if (snp >= sp->level[rcu_num_lvls - 1])
>   cbs = snp->srcu_have_cbs[idx] == gpseq;
> @@ -540,7 +567,7 @@ static void srcu_gp_end(struct srcu_struct *sp)
>   snp->srcu_gp_seq_needed_exp = gpseq;
>   mask = snp->srcu_data_have_cbs[idx];
>   snp->srcu_data_have_cbs[idx] = 0;
> - raw_spin_unlock_irq_rcu_node(snp);
> + spin_unlock_irq_rcu_node(snp);
>   if (cbs)
>   srcu_schedule_cbs_snp(sp, snp, mask, cbdelay);
>  
> @@ -548,11 +575,11 @@ static void srcu_gp_end(struct srcu_struct *sp)
>   if (!(gpseq & counter_wrap_check))
>   for (cpu = snp->grplo; cpu <= snp->grphi; cpu++) {
>   sdp = per_cpu_ptr(sp->sda, cpu);
> - raw_spin_lock_irqsave_rcu_node(sdp, flags);
> + spin_lock_irqsave_rcu_node(sdp, flags);
>   if (ULONG_CMP_GE(gpseq,
>   sdp->srcu_gp_seq_needed + 100))
>   sdp->srcu_gp_seq_needed = gpseq;
> - raw_spin_unlock_irqrestore_rcu_node(sdp, flags);
> + spin_unlock_irqrestore_rcu_node(sdp, flags);
>   }
>   }
>  
> @@ -560,17 +587,17 @@ static void srcu_gp_end(struct srcu_struct *sp)
>   mutex_unlock(&sp->srcu_cb_mutex);
>  
>   /* Start a new grace period if needed. */
> - raw_spin_lock_irq_rcu_node(sp);
> + spin_lock_irq_rcu_node(sp);
>   gpseq = rcu_seq_current(&sp->srcu_gp_seq);
>   if (!rcu_seq_state(gpseq) &&
>      ULONG_CMP_LT(gpseq, sp->srcu_gp_seq_needed)) {
>   srcu_gp_start(sp);
> - raw_spin_unlock_irq_rcu_node(sp);
> + spin_unlock_irq_rcu_node(sp);
>   /* Throttle expedited grace periods: Should be rare! */
>   srcu_reschedule(sp, rcu_seq_ctr(gpseq) & 0x3ff
>      ? 0 : SRCU_INTERVAL);
>   } else {
> - raw_spin_unlock_irq_rcu_node(sp);
> + spin_unlock_irq_rcu_node(sp);
>   }
>  }
>  
> @@ -590,18 +617,18 @@ static void srcu_funnel_exp_start(struct srcu_struct *sp, struct srcu_node *snp,
>   if (rcu_seq_done(&sp->srcu_gp_seq, s) ||
>      ULONG_CMP_GE(READ_ONCE(snp->srcu_gp_seq_needed_exp), s))
>   return;
> - raw_spin_lock_irqsave_rcu_node(snp, flags);
> + spin_lock_irqsave_rcu_node(snp, flags);
>   if (ULONG_CMP_GE(snp->srcu_gp_seq_needed_exp, s)) {
> - raw_spin_unlock_irqrestore_rcu_node(snp, flags);
> + spin_unlock_irqrestore_rcu_node(snp, flags);
>   return;
>   }
>   WRITE_ONCE(snp->srcu_gp_seq_needed_exp, s);
> - raw_spin_unlock_irqrestore_rcu_node(snp, flags);
> + spin_unlock_irqrestore_rcu_node(snp, flags);
>   }
> - raw_spin_lock_irqsave_rcu_node(sp, flags);
> + spin_lock_irqsave_rcu_node(sp, flags);
>   if (!ULONG_CMP_LT(sp->srcu_gp_seq_needed_exp, s))
>   sp->srcu_gp_seq_needed_exp = s;
> - raw_spin_unlock_irqrestore_rcu_node(sp, flags);
> + spin_unlock_irqrestore_rcu_node(sp, flags);
>  }
>  
>  /*
> @@ -623,12 +650,12 @@ static void srcu_funnel_gp_start(struct srcu_struct *sp, struct srcu_data *sdp,
>   for (; snp != NULL; snp = snp->srcu_parent) {
>   if (rcu_seq_done(&sp->srcu_gp_seq, s) && snp != sdp->mynode)
>   return; /* GP already done and CBs recorded. */
> - raw_spin_lock_irqsave_rcu_node(snp, flags);
> + spin_lock_irqsave_rcu_node(snp, flags);
>   if (ULONG_CMP_GE(snp->srcu_have_cbs[idx], s)) {
>   snp_seq = snp->srcu_have_cbs[idx];
>   if (snp == sdp->mynode && snp_seq == s)
>   snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
> - raw_spin_unlock_irqrestore_rcu_node(snp, flags);
> + spin_unlock_irqrestore_rcu_node(snp, flags);
>   if (snp == sdp->mynode && snp_seq != s) {
>   srcu_schedule_cbs_sdp(sdp, do_norm
>     ? SRCU_INTERVAL
> @@ -644,11 +671,11 @@ static void srcu_funnel_gp_start(struct srcu_struct *sp, struct srcu_data *sdp,
>   snp->srcu_data_have_cbs[idx] |= sdp->grpmask;
>   if (!do_norm && ULONG_CMP_LT(snp->srcu_gp_seq_needed_exp, s))
>   snp->srcu_gp_seq_needed_exp = s;
> - raw_spin_unlock_irqrestore_rcu_node(snp, flags);
> + spin_unlock_irqrestore_rcu_node(snp, flags);
>   }
>  
>   /* Top of tree, must ensure the grace period will be started. */
> - raw_spin_lock_irqsave_rcu_node(sp, flags);
> + spin_lock_irqsave_rcu_node(sp, flags);
>   if (ULONG_CMP_LT(sp->srcu_gp_seq_needed, s)) {
>   /*
>   * Record need for grace period s.  Pair with load
> @@ -667,7 +694,7 @@ static void srcu_funnel_gp_start(struct srcu_struct *sp, struct srcu_data *sdp,
>   queue_delayed_work(system_power_efficient_wq, &sp->work,
>     srcu_get_delay(sp));
>   }
> - raw_spin_unlock_irqrestore_rcu_node(sp, flags);
> + spin_unlock_irqrestore_rcu_node(sp, flags);
>  }
>  
>  /*
> @@ -830,7 +857,7 @@ void __call_srcu(struct srcu_struct *sp, struct rcu_head *rhp,
>   rhp->func = func;
>   local_irq_save(flags);
>   sdp = this_cpu_ptr(sp->sda);
> - raw_spin_lock_rcu_node(sdp);
> + spin_lock_rcu_node(sdp);
>   rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp, false);
>   rcu_segcblist_advance(&sdp->srcu_cblist,
>        rcu_seq_current(&sp->srcu_gp_seq));
> @@ -844,7 +871,7 @@ void __call_srcu(struct srcu_struct *sp, struct rcu_head *rhp,
>   sdp->srcu_gp_seq_needed_exp = s;
>   needexp = true;
>   }
> - raw_spin_unlock_irqrestore_rcu_node(sdp, flags);
> + spin_unlock_irqrestore_rcu_node(sdp, flags);
>   if (needgp)
>   srcu_funnel_gp_start(sp, sdp, s, do_norm);
>   else if (needexp)
> @@ -900,7 +927,7 @@ static void __synchronize_srcu(struct srcu_struct *sp, bool do_norm)
>  
>   /*
>   * Make sure that later code is ordered after the SRCU grace
> - * period.  This pairs with the raw_spin_lock_irq_rcu_node()
> + * period.  This pairs with the spin_lock_irq_rcu_node()
>   * in srcu_invoke_callbacks().  Unlike Tree RCU, this is needed
>   * because the current CPU might have been totally uninvolved with
>   * (and thus unordered against) that grace period.
> @@ -1024,7 +1051,7 @@ void srcu_barrier(struct srcu_struct *sp)
>   */
>   for_each_possible_cpu(cpu) {
>   sdp = per_cpu_ptr(sp->sda, cpu);
> - raw_spin_lock_irq_rcu_node(sdp);
> + spin_lock_irq_rcu_node(sdp);
>   atomic_inc(&sp->srcu_barrier_cpu_cnt);
>   sdp->srcu_barrier_head.func = srcu_barrier_cb;
>   debug_rcu_head_queue(&sdp->srcu_barrier_head);
> @@ -1033,7 +1060,7 @@ void srcu_barrier(struct srcu_struct *sp)
>   debug_rcu_head_unqueue(&sdp->srcu_barrier_head);
>   atomic_dec(&sp->srcu_barrier_cpu_cnt);
>   }
> - raw_spin_unlock_irq_rcu_node(sdp);
> + spin_unlock_irq_rcu_node(sdp);
>   }
>  
>   /* Remove the initial count, at which point reaching zero can happen. */
> @@ -1082,17 +1109,17 @@ static void srcu_advance_state(struct srcu_struct *sp)
>   */
>   idx = rcu_seq_state(smp_load_acquire(&sp->srcu_gp_seq)); /* ^^^ */
>   if (idx == SRCU_STATE_IDLE) {
> - raw_spin_lock_irq_rcu_node(sp);
> + spin_lock_irq_rcu_node(sp);
>   if (ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed)) {
>   WARN_ON_ONCE(rcu_seq_state(sp->srcu_gp_seq));
> - raw_spin_unlock_irq_rcu_node(sp);
> + spin_unlock_irq_rcu_node(sp);
>   mutex_unlock(&sp->srcu_gp_mutex);
>   return;
>   }
>   idx = rcu_seq_state(READ_ONCE(sp->srcu_gp_seq));
>   if (idx == SRCU_STATE_IDLE)
>   srcu_gp_start(sp);
> - raw_spin_unlock_irq_rcu_node(sp);
> + spin_unlock_irq_rcu_node(sp);
>   if (idx != SRCU_STATE_IDLE) {
>   mutex_unlock(&sp->srcu_gp_mutex);
>   return; /* Someone else started the grace period. */
> @@ -1141,19 +1168,19 @@ static void srcu_invoke_callbacks(struct work_struct *work)
>   sdp = container_of(work, struct srcu_data, work.work);
>   sp = sdp->sp;
>   rcu_cblist_init(&ready_cbs);
> - raw_spin_lock_irq_rcu_node(sdp);
> + spin_lock_irq_rcu_node(sdp);
>   rcu_segcblist_advance(&sdp->srcu_cblist,
>        rcu_seq_current(&sp->srcu_gp_seq));
>   if (sdp->srcu_cblist_invoking ||
>      !rcu_segcblist_ready_cbs(&sdp->srcu_cblist)) {
> - raw_spin_unlock_irq_rcu_node(sdp);
> + spin_unlock_irq_rcu_node(sdp);
>   return;  /* Someone else on the job or nothing to do. */
>   }
>  
>   /* We are on the job!  Extract and invoke ready callbacks. */
>   sdp->srcu_cblist_invoking = true;
>   rcu_segcblist_extract_done_cbs(&sdp->srcu_cblist, &ready_cbs);
> - raw_spin_unlock_irq_rcu_node(sdp);
> + spin_unlock_irq_rcu_node(sdp);
>   rhp = rcu_cblist_dequeue(&ready_cbs);
>   for (; rhp != NULL; rhp = rcu_cblist_dequeue(&ready_cbs)) {
>   debug_rcu_head_unqueue(rhp);
> @@ -1166,13 +1193,13 @@ static void srcu_invoke_callbacks(struct work_struct *work)
>   * Update counts, accelerate new callbacks, and if needed,
>   * schedule another round of callback invocation.
>   */
> - raw_spin_lock_irq_rcu_node(sdp);
> + spin_lock_irq_rcu_node(sdp);
>   rcu_segcblist_insert_count(&sdp->srcu_cblist, &ready_cbs);
>   (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
>         rcu_seq_snap(&sp->srcu_gp_seq));
>   sdp->srcu_cblist_invoking = false;
>   more = rcu_segcblist_ready_cbs(&sdp->srcu_cblist);
> - raw_spin_unlock_irq_rcu_node(sdp);
> + spin_unlock_irq_rcu_node(sdp);
>   if (more)
>   srcu_schedule_cbs_sdp(sdp, 0);
>  }
> @@ -1185,7 +1212,7 @@ static void srcu_reschedule(struct srcu_struct *sp, unsigned long delay)
>  {
>   bool pushgp = true;
>  
> - raw_spin_lock_irq_rcu_node(sp);
> + spin_lock_irq_rcu_node(sp);
>   if (ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed)) {
>   if (!WARN_ON_ONCE(rcu_seq_state(sp->srcu_gp_seq))) {
>   /* All requests fulfilled, time to go idle. */
> @@ -1195,7 +1222,7 @@ static void srcu_reschedule(struct srcu_struct *sp, unsigned long delay)
>   /* Outstanding request and no GP.  Start one. */
>   srcu_gp_start(sp);
>   }
> - raw_spin_unlock_irq_rcu_node(sp);
> + spin_unlock_irq_rcu_node(sp);
>  
>   if (pushgp)
>   queue_delayed_work(system_power_efficient_wq, &sp->work, delay);
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [bionic][PATCH 2/2] srcu: Lock srcu_data structure in srcu_gp_start()

Stefan Bader-2
In reply to this post by Marcelo Henrique Cerri
On 21.02.19 00:28, Marcelo Henrique Cerri wrote:

> From: Dennis Krein <[hidden email]>
>
> BugLink: http://bugs.launchpad.net/bugs/1802021
>
> The srcu_gp_start() function is called with the srcu_struct structure's
> ->lock held, but not with the srcu_data structure's ->lock.  This is
> problematic because this function accesses and updates the srcu_data
> structure's ->srcu_cblist, which is protected by that lock.  Failing to
> hold this lock can result in corruption of the SRCU callback lists,
> which in turn can result in arbitrarily bad results.
>
> This commit therefore makes srcu_gp_start() acquire the srcu_data
> structure's ->lock across the calls to rcu_segcblist_advance() and
> rcu_segcblist_accelerate(), thus preventing this corruption.
>
> Reported-by: Bart Van Assche <[hidden email]>
> Reported-by: Christoph Hellwig <[hidden email]>
> Reported-by: Sebastian Kuzminsky <[hidden email]>
> Signed-off-by: Dennis Krein <[hidden email]>
> Signed-off-by: Paul E. McKenney <[hidden email]>
> Tested-by: Dennis Krein <[hidden email]>
> Cc: <[hidden email]> # 4.16.x
> (cherry picked from commit eb4c2382272ae7ae5d81fdfa5b7a6c86146eaaa4)
> Acked-by: Stefan Bader <[hidden email]>
> Acked-by: Kleber Sacilotto de Souza <[hidden email]>
> Signed-off-by: Marcelo Henrique Cerri <[hidden email]>
> ---
Cannot remember having acked this yet...

>  kernel/rcu/srcutree.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index d5cea81378cc..b3e5e9873582 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -441,10 +441,12 @@ static void srcu_gp_start(struct srcu_struct *sp)
>  
>   lockdep_assert_held(&sp->lock);
>   WARN_ON_ONCE(ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed));
> + spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
>   rcu_segcblist_advance(&sdp->srcu_cblist,
>        rcu_seq_current(&sp->srcu_gp_seq));
>   (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
>         rcu_seq_snap(&sp->srcu_gp_seq));
> + spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
>   smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
>   rcu_seq_start(&sp->srcu_gp_seq);
>   state = rcu_seq_state(READ_ONCE(sp->srcu_gp_seq));
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [bionic, cosmic][PATCH 0/2, 0/1] srcu: Lock srcu_data structure in srcu_gp_start()

Stefan Bader-2
In reply to this post by Marcelo Henrique Cerri
On 21.02.19 00:28, Marcelo Henrique Cerri wrote:

> BugLink: http://bugs.launchpad.net/bugs/1802021
>
> comic only requires one of the patches.
>
> Dennis Krein (1):
>   srcu: Lock srcu_data structure in srcu_gp_start()
>
> Paul E. McKenney (1):
>   srcu: Prohibit call_srcu() use under raw spinlocks
>
>  include/linux/srcutree.h |   8 +--
>  kernel/rcu/srcutree.c    | 111 ++++++++++++++++++++++++---------------
>  2 files changed, 74 insertions(+), 45 deletions(-)
>
Also in general, the bug report is missing SRU justification


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [bionic][PATCH 2/2] srcu: Lock srcu_data structure in srcu_gp_start()

Marcelo Henrique Cerri
In reply to this post by Stefan Bader-2
My mistake. I used the patches that went into linux-azure to prepare it.

I will update it and re-submit it first thing in the morning.

On Wed, Feb 27, 2019 at 10:38:23AM +0100, Stefan Bader wrote:

> On 21.02.19 00:28, Marcelo Henrique Cerri wrote:
> > From: Dennis Krein <[hidden email]>
> >
> > BugLink: http://bugs.launchpad.net/bugs/1802021
> >
> > The srcu_gp_start() function is called with the srcu_struct structure's
> > ->lock held, but not with the srcu_data structure's ->lock.  This is
> > problematic because this function accesses and updates the srcu_data
> > structure's ->srcu_cblist, which is protected by that lock.  Failing to
> > hold this lock can result in corruption of the SRCU callback lists,
> > which in turn can result in arbitrarily bad results.
> >
> > This commit therefore makes srcu_gp_start() acquire the srcu_data
> > structure's ->lock across the calls to rcu_segcblist_advance() and
> > rcu_segcblist_accelerate(), thus preventing this corruption.
> >
> > Reported-by: Bart Van Assche <[hidden email]>
> > Reported-by: Christoph Hellwig <[hidden email]>
> > Reported-by: Sebastian Kuzminsky <[hidden email]>
> > Signed-off-by: Dennis Krein <[hidden email]>
> > Signed-off-by: Paul E. McKenney <[hidden email]>
> > Tested-by: Dennis Krein <[hidden email]>
> > Cc: <[hidden email]> # 4.16.x
> > (cherry picked from commit eb4c2382272ae7ae5d81fdfa5b7a6c86146eaaa4)
> > Acked-by: Stefan Bader <[hidden email]>
> > Acked-by: Kleber Sacilotto de Souza <[hidden email]>
> > Signed-off-by: Marcelo Henrique Cerri <[hidden email]>
> > ---
>
> Cannot remember having acked this yet...
>
> >  kernel/rcu/srcutree.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index d5cea81378cc..b3e5e9873582 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -441,10 +441,12 @@ static void srcu_gp_start(struct srcu_struct *sp)
> >  
> >   lockdep_assert_held(&sp->lock);
> >   WARN_ON_ONCE(ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed));
> > + spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
> >   rcu_segcblist_advance(&sdp->srcu_cblist,
> >        rcu_seq_current(&sp->srcu_gp_seq));
> >   (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
> >         rcu_seq_snap(&sp->srcu_gp_seq));
> > + spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
> >   smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
> >   rcu_seq_start(&sp->srcu_gp_seq);
> >   state = rcu_seq_state(READ_ONCE(sp->srcu_gp_seq));
> >
>
>



--
Regards,
Marcelo


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (499 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [bionic][PATCH 2/2] srcu: Lock srcu_data structure in srcu_gp_start()

Joshua R. Poulson
I did submit this one in
https://bugs.launchpad.net/ubuntu/+source/linux-azure/+bug/1802021 for
an Azure customer that was having an issue. This patch addressed their
problem, but later in that same bug was another user who was
experiencing issues that was looking for the same fix in the generic
kernels.

Thanks, --jrp

On Wed, Feb 27, 2019 at 3:22 PM Marcelo Henrique Cerri
<[hidden email]> wrote:

>
> My mistake. I used the patches that went into linux-azure to prepare it.
>
> I will update it and re-submit it first thing in the morning.
>
> On Wed, Feb 27, 2019 at 10:38:23AM +0100, Stefan Bader wrote:
> > On 21.02.19 00:28, Marcelo Henrique Cerri wrote:
> > > From: Dennis Krein <[hidden email]>
> > >
> > > BugLink: http://bugs.launchpad.net/bugs/1802021
> > >
> > > The srcu_gp_start() function is called with the srcu_struct structure's
> > > ->lock held, but not with the srcu_data structure's ->lock.  This is
> > > problematic because this function accesses and updates the srcu_data
> > > structure's ->srcu_cblist, which is protected by that lock.  Failing to
> > > hold this lock can result in corruption of the SRCU callback lists,
> > > which in turn can result in arbitrarily bad results.
> > >
> > > This commit therefore makes srcu_gp_start() acquire the srcu_data
> > > structure's ->lock across the calls to rcu_segcblist_advance() and
> > > rcu_segcblist_accelerate(), thus preventing this corruption.
> > >
> > > Reported-by: Bart Van Assche <[hidden email]>
> > > Reported-by: Christoph Hellwig <[hidden email]>
> > > Reported-by: Sebastian Kuzminsky <[hidden email]>
> > > Signed-off-by: Dennis Krein <[hidden email]>
> > > Signed-off-by: Paul E. McKenney <[hidden email]>
> > > Tested-by: Dennis Krein <[hidden email]>
> > > Cc: <[hidden email]> # 4.16.x
> > > (cherry picked from commit eb4c2382272ae7ae5d81fdfa5b7a6c86146eaaa4)
> > > Acked-by: Stefan Bader <[hidden email]>
> > > Acked-by: Kleber Sacilotto de Souza <[hidden email]>
> > > Signed-off-by: Marcelo Henrique Cerri <[hidden email]>
> > > ---
> >
> > Cannot remember having acked this yet...
> >
> > >  kernel/rcu/srcutree.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > > index d5cea81378cc..b3e5e9873582 100644
> > > --- a/kernel/rcu/srcutree.c
> > > +++ b/kernel/rcu/srcutree.c
> > > @@ -441,10 +441,12 @@ static void srcu_gp_start(struct srcu_struct *sp)
> > >
> > >     lockdep_assert_held(&sp->lock);
> > >     WARN_ON_ONCE(ULONG_CMP_GE(sp->srcu_gp_seq, sp->srcu_gp_seq_needed));
> > > +   spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
> > >     rcu_segcblist_advance(&sdp->srcu_cblist,
> > >                           rcu_seq_current(&sp->srcu_gp_seq));
> > >     (void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
> > >                                    rcu_seq_snap(&sp->srcu_gp_seq));
> > > +   spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
> > >     smp_mb(); /* Order prior store to ->srcu_gp_seq_needed vs. GP start. */
> > >     rcu_seq_start(&sp->srcu_gp_seq);
> > >     state = rcu_seq_state(READ_ONCE(sp->srcu_gp_seq));
> > >
> >
> >
>
>
>
>
> --
> Regards,
> Marcelo
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK: [bionic, cosmic][PATCH 0/2, 0/1] srcu: Lock srcu_data structure in srcu_gp_start()

Marcelo Henrique Cerri
In reply to this post by Marcelo Henrique Cerri

Sending a v2.

On Wed, Feb 20, 2019 at 08:28:35PM -0300, Marcelo Henrique Cerri wrote:

> BugLink: http://bugs.launchpad.net/bugs/1802021
>
> comic only requires one of the patches.
>
> Dennis Krein (1):
>   srcu: Lock srcu_data structure in srcu_gp_start()
>
> Paul E. McKenney (1):
>   srcu: Prohibit call_srcu() use under raw spinlocks
>
>  include/linux/srcutree.h |   8 +--
>  kernel/rcu/srcutree.c    | 111 ++++++++++++++++++++++++---------------
>  2 files changed, 74 insertions(+), 45 deletions(-)
>
> --
> 2.17.1
>
--
Regards,
Marcelo


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (499 bytes) Download Attachment