linux-kernel - Re: [PATCH] watchdog: don't run proc_watchdog

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160314162955.GQ194535@redhat.com>
Date:	Mon, 14 Mar 2016 12:29:55 -0400
From:	Don Zickus <dzickus@...hat.com>
To:	Josh Hunt <johunt@...mai.com>
Cc:	akpm@...ux-foundation.org, uobergfe@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] watchdog: don't run proc_watchdog_update if new value is
 same as old

On Mon, Mar 14, 2016 at 09:45:26AM -0500, Josh Hunt wrote:
> On 03/14/2016 09:34 AM, Don Zickus wrote:
> >On Sat, Mar 12, 2016 at 06:50:26PM -0500, Joshua Hunt wrote:
> >>While working on a script to restore all sysctl params before a series of
> >>tests I found that writing any value into the
> >>/proc/sys/kernel/{nmi_watchdog,soft_watchdog,watchdog,watchdog_thresh}
> >>causes them to call proc_watchdog_update(). Not only that, but when I
> >>wrote to these proc files in a loop I could easily trigger a soft lockup.
> >>
> >>[  955.756196] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> >>[  955.765994] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> >>[  955.774619] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> >>[  955.783182] NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> >>[  959.788319] NMI watchdog: BUG: soft lockup - CPU#4 stuck for 30s! [swapper/4:0]
> >>[  959.788325] NMI watchdog: BUG: soft lockup - CPU#5 stuck for 30s! [swapper/5:0]
> >>
> >>There doesn't appear to be a reason for doing this work other every time a
> >>write occurs, so only do the work when the values change.
> >
> >Hi Josh,
> >
> >Thanks for the patch.  I have no objections to it, but Uli and myself were
> >interested in the reason for the softlockups.  Uli is going to provide a
> >test patch to see if his theory is correct.  That way we fix the underlying
> >issue and then apply your patch on top. Make sense?
> 
> Yep. Sounds good. I meant to mention I didn't diagnose the soft-lockup. If
> you provide a patch I'm happy to test. I can also attempt to debug that part
> more if needed.

Hi Josh,

I believe Uli thought the below patch might fix it.

Cheers,
Don

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index b3ace6e..dd298d2 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -517,12 +517,12 @@ static void watchdog_enable(unsigned int cpu)
 	/* Enable the perf event */
 	watchdog_nmi_enable(cpu);
 
+	watchdog_set_prio(SCHED_FIFO, MAX_RT_PRIO - 1);
 	/* done here because hrtimer_start can only pin to smp_processor_id() */
 	hrtimer_start(hrtimer, ns_to_ktime(sample_period),
 		      HRTIMER_MODE_REL_PINNED);
 
 	/* initialize timestamp */
-	watchdog_set_prio(SCHED_FIFO, MAX_RT_PRIO - 1);
 	__touch_watchdog();
 }
 
@@ -530,8 +530,8 @@ static void watchdog_disable(unsigned int cpu)
 {
 	struct hrtimer *hrtimer = raw_cpu_ptr(&watchdog_hrtimer);
 
-	watchdog_set_prio(SCHED_NORMAL, 0);
 	hrtimer_cancel(hrtimer);
+	watchdog_set_prio(SCHED_NORMAL, 0);
 	/* disable the perf event */
 	watchdog_nmi_disable(cpu);
 }