Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1261583 > unrolled thread

[PATCH 0/4] watchdog: address various races (CPU hotplug, timer expiry)

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] watchdog: address various races (CPU hotplug, timer expiry) Ulrich Obergfell <uobergfe@redhat.com> - 2015-11-03 16:20 +0100
    [PATCH 3/4] watchdog: remove {get|put}_online_cpus() from watchdog_{park|unpark}_threads() Ulrich Obergfell <uobergfe@redhat.com> - 2015-11-03 16:20 +0100
    [PATCH 2/4] watchdog: avoid races between /proc handlers and CPU hotplug Ulrich Obergfell <uobergfe@redhat.com> - 2015-11-03 16:20 +0100
    [PATCH 1/4] watchdog: avoid race between lockup detector suspend/resume and CPU hotplug Ulrich Obergfell <uobergfe@redhat.com> - 2015-11-03 17:20 +0100
    Re: [PATCH 0/4] watchdog: address various races (CPU hotplug, timer  expiry) Don Zickus <dzickus@redhat.com> - 2015-11-05 15:50 +0100
    Re: [PATCH 0/4] watchdog: address various races (CPU hotplug, timer  expiry) Aaron Tomlin <atomlin@redhat.com> - 2015-11-05 22:00 +0100

#1261583 — [PATCH 0/4] watchdog: address various races (CPU hotplug, timer expiry)

This patch set addresses various races in relation to CPU hotplug
and a race in relation to watchdog timer expiry. I discovered the
corner cases during code inspection. I haven't seen any of these
issues occur in practice.

Ulrich Obergfell (4):
  watchdog: avoid race between lockup detector suspend/resume and CPU
    hotplug
  watchdog: avoid races between /proc handlers and CPU hotplug
  watchdog: remove {get|put}_online_cpus() from
    watchdog_{park|unpark}_threads()
  watchdog: fix race between proc_watchdog_thresh() and
    watchdog_timer_fn()

 kernel/watchdog.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1261584 — [PATCH 3/4] watchdog: remove {get|put}_online_cpus() from watchdog_{park|unpark}_threads()

watchdog_{park|unpark}_threads() are now called in code paths that
protect themselves against CPU hotplug, so {get|put}_online_cpus()
calls are redundant and can be removed.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 kernel/watchdog.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 13fdda1..84c4744 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -683,33 +683,35 @@ static struct smp_hotplug_thread watchdog_threads = {
  * be parked and the watchdog threads of other CPUs can still be runnable.
  * Callers are expected to handle this special condition as appropriate in
  * their context.
+ *
+ * This function may only be called in a context that is protected against
+ * races with CPU hotplug - for example, via get_online_cpus().
  */
 static int watchdog_park_threads(void)
 {
 	int cpu, ret = 0;
 
-	get_online_cpus();
 	for_each_watchdog_cpu(cpu) {
 		ret = kthread_park(per_cpu(softlockup_watchdog, cpu));
 		if (ret)
 			break;
 	}
-	put_online_cpus();
 
 	return ret;
 }
 
 /*
  * unpark all watchdog threads that are specified in 'watchdog_cpumask'
+ *
+ * This function may only be called in a context that is protected against
+ * races with CPU hotplug - for example, via get_online_cpus().
  */
 static void watchdog_unpark_threads(void)
 {
 	int cpu;
 
-	get_online_cpus();
 	for_each_watchdog_cpu(cpu)
 		kthread_unpark(per_cpu(softlockup_watchdog, cpu));
-	put_online_cpus();
 }
 
 /*
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261590 — [PATCH 2/4] watchdog: avoid races between /proc handlers and CPU hotplug

The handler functions for watchdog parameters in /proc/sys/kernel
do not protect themselves against races with CPU hotplug. Hence,
theoretically it is possible that a new watchdog thread is started
on a hotplugged CPU while a parameter is being modified, and the
thread could thus use a parameter value that is 'in transition'.

For example, if 'watchdog_thresh' is being set to zero (note: this
disables the lockup detectors) the thread would erroneously use the
value zero as the sample period.

To avoid such races and to keep the /proc handler code consistent,
call
     {get|put}_online_cpus() in proc_watchdog_common()
     {get|put}_online_cpus() in proc_watchdog_thresh()
     {get|put}_online_cpus() in proc_watchdog_cpumask()

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 kernel/watchdog.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 7357842..13fdda1 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -857,6 +857,7 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 	int err, old, new;
 	int *watchdog_param = (int *)table->data;
 
+	get_online_cpus();
 	mutex_lock(&watchdog_proc_mutex);
 
 	if (watchdog_suspended) {
@@ -908,6 +909,7 @@ static int proc_watchdog_common(int which, struct ctl_table *table, int write,
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);
+	put_online_cpus();
 	return err;
 }
 
@@ -949,6 +951,7 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 {
 	int err, old;
 
+	get_online_cpus();
 	mutex_lock(&watchdog_proc_mutex);
 
 	if (watchdog_suspended) {
@@ -974,6 +977,7 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);
+	put_online_cpus();
 	return err;
 }
 
@@ -988,6 +992,7 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 {
 	int err;
 
+	get_online_cpus();
 	mutex_lock(&watchdog_proc_mutex);
 
 	if (watchdog_suspended) {
@@ -1015,6 +1020,7 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);
+	put_online_cpus();
 	return err;
 }
 
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261685 — [PATCH 1/4] watchdog: avoid race between lockup detector suspend/resume and CPU hotplug

The lockup detector suspend/resume interface that was introduced by
commit 8c073d27d7ad293bf734cc8475689413afadab81 does not protect
itself against races with CPU hotplug. Hence, theoretically it is
possible that a new watchdog thread is started on a hotplugged CPU
while the lockup detector is suspended, and the thread could thus
interfere unexpectedly with the code that requested to suspend the
lockup detector. Avoid the race by calling

  get_online_cpus() in lockup_detector_suspend()
  put_online_cpus() in lockup_detector_resume()

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
---
 kernel/watchdog.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 0a23125..7357842 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -719,6 +719,7 @@ int lockup_detector_suspend(void)
 {
 	int ret = 0;
 
+	get_online_cpus();
 	mutex_lock(&watchdog_proc_mutex);
 	/*
 	 * Multiple suspend requests can be active in parallel (counted by
@@ -759,6 +760,7 @@ void lockup_detector_resume(void)
 		watchdog_unpark_threads();
 
 	mutex_unlock(&watchdog_proc_mutex);
+	put_online_cpus();
 }
 
 static int update_watchdog_all_cpus(void)
-- 
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263279 — Re: [PATCH 0/4] watchdog: address various races (CPU hotplug, timer expiry)

On Tue, Nov 03, 2015 at 04:20:57PM +0100, Ulrich Obergfell wrote:
> This patch set addresses various races in relation to CPU hotplug
> and a race in relation to watchdog timer expiry. I discovered the
> corner cases during code inspection. I haven't seen any of these
> issues occur in practice.

Series looks fine to me.  I have run some local panic tests with no
problems, along with modifying various values and everything seems fine.

Acked-by: Don Zickus <dzickus@redhat.com>

> 
> Ulrich Obergfell (4):
>   watchdog: avoid race between lockup detector suspend/resume and CPU
>     hotplug
>   watchdog: avoid races between /proc handlers and CPU hotplug
>   watchdog: remove {get|put}_online_cpus() from
>     watchdog_{park|unpark}_threads()
>   watchdog: fix race between proc_watchdog_thresh() and
>     watchdog_timer_fn()
> 
>  kernel/watchdog.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> -- 
> 1.7.11.7
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263553 — Re: [PATCH 0/4] watchdog: address various races (CPU hotplug, timer expiry)

[Multipart message — attachments visible in raw view] — view raw

On Tue 2015-11-03 16:20 +0100, Ulrich Obergfell wrote:
> This patch set addresses various races in relation to CPU hotplug
> and a race in relation to watchdog timer expiry. I discovered the
> corner cases during code inspection. I haven't seen any of these
> issues occur in practice.

This patch series does adequately address the race conditions mentioned
above. Thanks.

Reviewed-by: Aaron Tomlin <atomlin@redhat.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web