Skip to content

Commit f16a797

Browse files
Luo Gengkungregkh
authored andcommitted
watchdog: fix watchdog may detect false positive of softlockup
commit 7123dbb upstream. When updating `watchdog_thresh`, there is a race condition between writing the new `watchdog_thresh` value and stopping the old watchdog timer. If the old timer triggers during this window, it may falsely detect a softlockup due to the old interval and the new `watchdog_thresh` value being used. The problem can be described as follow: # We asuume previous watchdog_thresh is 60, so the watchdog timer is # coming every 24s. echo 10 > /proc/sys/kernel/watchdog_thresh (User space) | +------>+ update watchdog_thresh (We are in kernel now) | | # using old interval and new `watchdog_thresh` +------>+ watchdog hrtimer (irq context: detect softlockup) | | +-------+ | | + softlockup_stop_all To fix this problem, introduce a shadow variable for `watchdog_thresh`. The update to the actual `watchdog_thresh` is delayed until after the old timer is stopped, preventing false positives. The following testcase may help to understand this problem. --------------------------------------------- echo RT_RUNTIME_SHARE > /sys/kernel/debug/sched/features echo -1 > /proc/sys/kernel/sched_rt_runtime_us echo 0 > /sys/kernel/debug/sched/fair_server/cpu3/runtime echo 60 > /proc/sys/kernel/watchdog_thresh taskset -c 3 chrt -r 99 /bin/bash -c "while true;do true; done" & echo 10 > /proc/sys/kernel/watchdog_thresh & --------------------------------------------- The test case above first removes the throttling restrictions for real-time tasks. It then sets watchdog_thresh to 60 and executes a real-time task ,a simple while(1) loop, on cpu3. Consequently, the final command gets blocked because the presence of this real-time thread prevents kworker:3 from being selected by the scheduler. This eventually triggers a softlockup detection on cpu3 due to watchdog_timer_fn operating with inconsistent variable - using both the old interval and the updated watchdog_thresh simultaneously. [[email protected]: fix the SOFTLOCKUP_DETECTOR=n case] Link: https://lkml.kernel.org/r/[email protected] Link: https://lkml.kernel.org/r/[email protected] Signed-off-by: Luo Gengkun <[email protected]> Signed-off-by: Nysal Jan K.A. <[email protected]> Cc: Doug Anderson <[email protected]> Cc: Joel Granados <[email protected]> Cc: Song Liu <[email protected]> Cc: Thomas Gleinxer <[email protected]> Cc: "Nysal Jan K.A." <[email protected]> Cc: Venkat Rao Bagalkote <[email protected]> Cc: <[email protected]> Signed-off-by: Andrew Morton <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
1 parent 68c173e commit f16a797

File tree

1 file changed

+27
-14
lines changed

1 file changed

+27
-14
lines changed

kernel/watchdog.c

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ int __read_mostly watchdog_user_enabled = 1;
4747
static int __read_mostly watchdog_hardlockup_user_enabled = WATCHDOG_HARDLOCKUP_DEFAULT;
4848
static int __read_mostly watchdog_softlockup_user_enabled = 1;
4949
int __read_mostly watchdog_thresh = 10;
50+
static int __read_mostly watchdog_thresh_next;
5051
static int __read_mostly watchdog_hardlockup_available;
5152

5253
struct cpumask watchdog_cpumask __read_mostly;
@@ -863,12 +864,20 @@ int lockup_detector_offline_cpu(unsigned int cpu)
863864
return 0;
864865
}
865866

866-
static void __lockup_detector_reconfigure(void)
867+
static void __lockup_detector_reconfigure(bool thresh_changed)
867868
{
868869
cpus_read_lock();
869870
watchdog_hardlockup_stop();
870871

871872
softlockup_stop_all();
873+
/*
874+
* To prevent watchdog_timer_fn from using the old interval and
875+
* the new watchdog_thresh at the same time, which could lead to
876+
* false softlockup reports, it is necessary to update the
877+
* watchdog_thresh after the softlockup is completed.
878+
*/
879+
if (thresh_changed)
880+
watchdog_thresh = READ_ONCE(watchdog_thresh_next);
872881
set_sample_period();
873882
lockup_detector_update_enable();
874883
if (watchdog_enabled && watchdog_thresh)
@@ -881,7 +890,7 @@ static void __lockup_detector_reconfigure(void)
881890
void lockup_detector_reconfigure(void)
882891
{
883892
mutex_lock(&watchdog_mutex);
884-
__lockup_detector_reconfigure();
893+
__lockup_detector_reconfigure(false);
885894
mutex_unlock(&watchdog_mutex);
886895
}
887896

@@ -901,27 +910,29 @@ static __init void lockup_detector_setup(void)
901910
return;
902911

903912
mutex_lock(&watchdog_mutex);
904-
__lockup_detector_reconfigure();
913+
__lockup_detector_reconfigure(false);
905914
softlockup_initialized = true;
906915
mutex_unlock(&watchdog_mutex);
907916
}
908917

909918
#else /* CONFIG_SOFTLOCKUP_DETECTOR */
910-
static void __lockup_detector_reconfigure(void)
919+
static void __lockup_detector_reconfigure(bool thresh_changed)
911920
{
912921
cpus_read_lock();
913922
watchdog_hardlockup_stop();
923+
if (thresh_changed)
924+
watchdog_thresh = READ_ONCE(watchdog_thresh_next);
914925
lockup_detector_update_enable();
915926
watchdog_hardlockup_start();
916927
cpus_read_unlock();
917928
}
918929
void lockup_detector_reconfigure(void)
919930
{
920-
__lockup_detector_reconfigure();
931+
__lockup_detector_reconfigure(false);
921932
}
922933
static inline void lockup_detector_setup(void)
923934
{
924-
__lockup_detector_reconfigure();
935+
__lockup_detector_reconfigure(false);
925936
}
926937
#endif /* !CONFIG_SOFTLOCKUP_DETECTOR */
927938

@@ -939,11 +950,11 @@ void lockup_detector_soft_poweroff(void)
939950
#ifdef CONFIG_SYSCTL
940951

941952
/* Propagate any changes to the watchdog infrastructure */
942-
static void proc_watchdog_update(void)
953+
static void proc_watchdog_update(bool thresh_changed)
943954
{
944955
/* Remove impossible cpus to keep sysctl output clean. */
945956
cpumask_and(&watchdog_cpumask, &watchdog_cpumask, cpu_possible_mask);
946-
__lockup_detector_reconfigure();
957+
__lockup_detector_reconfigure(thresh_changed);
947958
}
948959

949960
/*
@@ -976,7 +987,7 @@ static int proc_watchdog_common(int which, const struct ctl_table *table, int wr
976987
old = READ_ONCE(*param);
977988
err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
978989
if (!err && old != READ_ONCE(*param))
979-
proc_watchdog_update();
990+
proc_watchdog_update(false);
980991
}
981992
mutex_unlock(&watchdog_mutex);
982993
return err;
@@ -1027,11 +1038,13 @@ static int proc_watchdog_thresh(const struct ctl_table *table, int write,
10271038

10281039
mutex_lock(&watchdog_mutex);
10291040

1030-
old = READ_ONCE(watchdog_thresh);
1041+
watchdog_thresh_next = READ_ONCE(watchdog_thresh);
1042+
1043+
old = watchdog_thresh_next;
10311044
err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
10321045

1033-
if (!err && write && old != READ_ONCE(watchdog_thresh))
1034-
proc_watchdog_update();
1046+
if (!err && write && old != READ_ONCE(watchdog_thresh_next))
1047+
proc_watchdog_update(true);
10351048

10361049
mutex_unlock(&watchdog_mutex);
10371050
return err;
@@ -1052,7 +1065,7 @@ static int proc_watchdog_cpumask(const struct ctl_table *table, int write,
10521065

10531066
err = proc_do_large_bitmap(table, write, buffer, lenp, ppos);
10541067
if (!err && write)
1055-
proc_watchdog_update();
1068+
proc_watchdog_update(false);
10561069

10571070
mutex_unlock(&watchdog_mutex);
10581071
return err;
@@ -1072,7 +1085,7 @@ static struct ctl_table watchdog_sysctls[] = {
10721085
},
10731086
{
10741087
.procname = "watchdog_thresh",
1075-
.data = &watchdog_thresh,
1088+
.data = &watchdog_thresh_next,
10761089
.maxlen = sizeof(int),
10771090
.mode = 0644,
10781091
.proc_handler = proc_watchdog_thresh,

0 commit comments

Comments
 (0)