[bionic/linux-azure, cosmic/linux-azure][PATCH 0/2] [Hyper-V] 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/linux-azure, cosmic/linux-azure][PATCH 0/2] [Hyper-V] srcu: Lock srcu_data structure in srcu_gp_start()

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

comic/linux-azure 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/linux-azure][PATCH 1/2] srcu: Prohibit call_srcu() use under raw spinlocks

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

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

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]>
(cherry picked from commit d633198088bd9e358566c470ed182994403acc7a)
Signed-off-by: Marcelo Henrique Cerri <[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/linux-azure][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)
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/linux-azure][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)
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
|

ACK/Cmnt: [bionic/linux-azure, cosmic/linux-azure][PATCH 0/2] [Hyper-V] srcu: Lock srcu_data structure in srcu_gp_start()

Stefan Bader-2
In reply to this post by Marcelo Henrique Cerri
On 01.02.19 20:34, Marcelo Henrique Cerri wrote:

> BugLink: http://bugs.launchpad.net/bugs/1802021
>
> comic/linux-azure 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(-)
>
The cosmic and bionic versions of the patch adding additional locking claim to
be cherry picks both. If it is really the true, then it would be better to just
submit it once and just annotate it as "[bionic+cosmic/linux-azure] ..." or similar.

Acked-by: Stefan Bader <[hidden email]>


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

ACK/cmnt: [bionic/linux-azure, cosmic/linux-azure][PATCH 0/2] [Hyper-V] srcu: Lock srcu_data structure in srcu_gp_start()

Kleber Souza
In reply to this post by Marcelo Henrique Cerri
On 2/1/19 8:34 PM, Marcelo Henrique Cerri wrote:

> BugLink: http://bugs.launchpad.net/bugs/1802021
>
> comic/linux-azure 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(-)
>
There are some reports on the bug of the problem happening with the
generic 4.15 kernel as well. Are you considering submitting it later for
the generic kernel?

Acked-by: Kleber Sacilotto de Souza <[hidden email]>


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

Re: ACK/Cmnt: [bionic/linux-azure, cosmic/linux-azure][PATCH 0/2] [Hyper-V] srcu: Lock srcu_data structure in srcu_gp_start()

Marcelo Henrique Cerri
In reply to this post by Stefan Bader-2
That's usually true, but for this case the patch picked to one series
wasn't applying cleanly on the other due to small context
changes. `git cherry-pick` has more information to work on anyway, so
it's normal that sometimes that will happen.

--
Regards,
Marcelo

On Mon, Feb 04, 2019 at 09:21:56AM +0100, Stefan Bader wrote:

> On 01.02.19 20:34, Marcelo Henrique Cerri wrote:
> > BugLink: http://bugs.launchpad.net/bugs/1802021
> >
> > comic/linux-azure 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(-)
> >
> The cosmic and bionic versions of the patch adding additional locking claim to
> be cherry picks both. If it is really the true, then it would be better to just
> submit it once and just annotate it as "[bionic+cosmic/linux-azure] ..." or similar.
>
> Acked-by: Stefan Bader <[hidden email]>
>



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

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

Re: ACK/cmnt: [bionic/linux-azure, cosmic/linux-azure][PATCH 0/2] [Hyper-V] srcu: Lock srcu_data structure in srcu_gp_start()

Marcelo Henrique Cerri
In reply to this post by Kleber Souza

On Mon, Feb 04, 2019 at 12:33:09PM +0100, Kleber Souza wrote:

> On 2/1/19 8:34 PM, Marcelo Henrique Cerri wrote:
> > BugLink: http://bugs.launchpad.net/bugs/1802021
> >
> > comic/linux-azure 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(-)
> >
> There are some reports on the bug of the problem happening with the
> generic 4.15 kernel as well. Are you considering submitting it later for
> the generic kernel?
Eventually, yes. Microsoft reported the problem on linux-azure and SEG
is handling a similar problem for another user in linux-hwe. While MS
has already given us feedback saying that those commits actually fix
their problem, SEG is still working on that with that other
user. Because of that, we decided to apply that for linux-azure for
the time being.

>
> Acked-by: Kleber Sacilotto de Souza <[hidden email]>
>

--
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: ACK/Cmnt: [bionic/linux-azure, cosmic/linux-azure][PATCH 0/2] [Hyper-V] srcu: Lock srcu_data structure in srcu_gp_start()

Stefan Bader-2
In reply to this post by Marcelo Henrique Cerri
On 04.02.19 12:43, Marcelo Henrique Cerri wrote:
> That's usually true, but for this case the patch picked to one series
> wasn't applying cleanly on the other due to small context
> changes. `git cherry-pick` has more information to work on anyway, so
> it's normal that sometimes that will happen.
>
In this case I would put a

(backported ...)
[Minor context adjustments]
Signed-off...

into the one that needed anything beyond a -C2 to apply.

-Stefan


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

APPLIED: [bionic/linux-azure,cosmic/linux-azure][PATCH 0/2] [Hyper-V] srcu: Lock srcu_data structure in srcu_gp_start()

Marcelo Henrique Cerri
In reply to this post by Marcelo Henrique Cerri
--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (499 bytes) Download Attachment