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

Groups > linux.kernel > #1732767 > unrolled thread

Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG: unable to handle kernel NULL pointer dereference at 0000000000000208

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG:  unable to handle kernel NULL pointer dereference at 0000000000000208 Thomas Gleixner <tglx@linutronix.de> - 2017-09-15 10:00 +0200
    Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG:  unable to handle kernel NULL pointer dereference at 0000000000000208 Thomas Gleixner <tglx@linutronix.de> - 2017-09-15 15:00 +0200
      Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG:  unable to handle kernel NULL pointer dereference at 0000000000000208 Thomas Gleixner <tglx@linutronix.de> - 2017-09-15 18:30 +0200
        Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG:  unable to handle kernel NULL pointer dereference at 0000000000000208 Fengguang Wu <fengguang.wu@intel.com> - 2017-09-16 14:50 +0200
          Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG:  unable to handle kernel NULL pointer dereference at 0000000000000208 Thomas Gleixner <tglx@linutronix.de> - 2017-09-16 19:40 +0200
            Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG:  unable to handle kernel NULL pointer dereference at 0000000000000208 Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-16 20:00 +0200
              Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG:  unable to handle kernel NULL pointer dereference at 0000000000000208 Thomas Gleixner <tglx@linutronix.de> - 2017-09-16 20:20 +0200
                Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG:  unable to handle kernel NULL pointer dereference at 0000000000000208 Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-16 20:30 +0200
                  Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG:  unable to handle kernel NULL pointer dereference at 0000000000000208 Thomas Gleixner <tglx@linutronix.de> - 2017-09-16 23:50 +0200
                    Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG:  unable to handle kernel NULL pointer dereference at 0000000000000208 Linus Torvalds <torvalds@linux-foundation.org> - 2017-09-17 00:10 +0200

#1732767 — Re: d57108d4f6 ("watchdog/core: Get rid of the thread .."): BUG: unable to handle kernel NULL pointer dereference at 0000000000000208

On Fri, 15 Sep 2017, kernel test robot wrote:
> [    0.035023] CPU: Intel Common KVM processor (family: 0xf, model: 0x6, stepping: 0x1)
> [    0.042302] Performance Events: unsupported Netburst CPU model 6 no PMU driver, software events only.

Cute. So there is no supported PMU, but for some unknown reason the lockup
detector can create an event, otherwise the perf availaibility check in
lockup_detector_init() would fail ....

Peter???

> [    0.051650] BUG: unable to handle kernel NULL pointer dereference at 0000000000000208
> [    0.052000] IP: perf_event_release_kernel+0x14/0x1ed
> [    0.052000] PGD 0 P4D 0 
> [    0.052000] Oops: 0000 [#1]
> [    0.052000] CPU: 0 PID: 1 Comm: swapper Not tainted 4.13.0-11828-gd57108d #1
> [    0.052000] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014
> [    0.052000] task: ffff88001e532000 task.stack: ffffc90000008000
> [    0.052000] RIP: 0010:perf_event_release_kernel+0x14/0x1ed
> [    0.052000] RSP: 0000:ffffc9000000be38 EFLAGS: 00010292
> [    0.052000] RAX: 0000000000000001 RBX: 0000000000000000 RCX: 000000000313fa45
> [    0.052000] RDX: 0000000000000001 RSI: ffffffff828265e0 RDI: 0000000000000000
> [    0.052000] RBP: ffffc9000000beb8 R08: ffffc9000000bdc8 R09: 0000000000000000
> [    0.052000] R10: ffff88001e532000 R11: ffffc9000000bd68 R12: 0000000000000000
> [    0.052000] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> [    0.052000] FS:  0000000000000000(0000) GS:ffffffff8262f000(0000) knlGS:0000000000000000
> [    0.052000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    0.052000] CR2: 0000000000000208 CR3: 0000000002618000 CR4: 00000000000006b0
> [    0.052000] Call Trace:
> [    0.052000]  ? trace_hardirqs_on_caller+0x162/0x198
> [    0.052000]  hardlockup_detector_perf_cleanup+0x20/0x2d
> [    0.052000]  __lockup_detector_cleanup+0x2c/0x2e
> [    0.052000]  softlockup_update_smpboot_threads+0x48/0x4a
> [    0.052000]  softlockup_reconfigure_threads+0x21/0x5a
> [    0.052000]  lockup_detector_init+0x75/0x83
> [    0.052000]  kernel_init_freeable+0x67/0xf0
> [    0.052000]  ? rest_init+0x13a/0x13a
> [    0.052000]  kernel_init+0xe/0xf8
> [    0.052000]  ret_from_fork+0x2a/0x40
> [    0.052000] Code: df e8 0c b2 ff ff 4c 89 e7 e8 89 13 bd 00 41 5b 5b 41 5c 41 5d c9 c3 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 89 fb 48 83 ec 58 <48> 83 bf 08 02 00 00 00 75 14 f6 87 8c 00 00 00 03 0f 84 9b 01 
> [    0.052000] RIP: perf_event_release_kernel+0x14/0x1ed RSP: ffffc9000000be38
> [    0.052000] CR2: 0000000000000208
> [    0.052000] ---[ end trace ed0d2c92e47d08db ]---
> [    0.052000] Kernel panic - not syncing: Fatal exception

[toc] | [next] | [standalone]

#1732873

On Fri, 15 Sep 2017, Thomas Gleixner wrote:

> On Fri, 15 Sep 2017, kernel test robot wrote:
> > [    0.035023] CPU: Intel Common KVM processor (family: 0xf, model: 0x6, stepping: 0x1)
> > [    0.042302] Performance Events: unsupported Netburst CPU model 6 no PMU driver, software events only.
> 
> Cute. So there is no supported PMU, but for some unknown reason the lockup
> detector can create an event, otherwise the perf availaibility check in
> lockup_detector_init() would fail ....
> 
> Peter???

In my VM the corresponding dmesg is:

[    0.038086] Performance Events: unsupported p6 CPU model 61 no PMU driver, software events only.
[    0.041031] Hierarchical SRCU implementation.
[    0.046210] NMI watchdog: Perf event create on CPU 0 failed with -2
[    0.046980] NMI watchdog: Perf NMI watchdog permanetely disabled

Confused

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

#1732944

On Fri, 15 Sep 2017, Thomas Gleixner wrote:

> On Fri, 15 Sep 2017, Thomas Gleixner wrote:
> 
> > On Fri, 15 Sep 2017, kernel test robot wrote:
> > > [    0.035023] CPU: Intel Common KVM processor (family: 0xf, model: 0x6, stepping: 0x1)
> > > [    0.042302] Performance Events: unsupported Netburst CPU model 6 no PMU driver, software events only.
> > 
> > Cute. So there is no supported PMU, but for some unknown reason the lockup
> > detector can create an event, otherwise the perf availaibility check in
> > lockup_detector_init() would fail ....
> > 
> > Peter???
> 
> In my VM the corresponding dmesg is:
> 
> [    0.038086] Performance Events: unsupported p6 CPU model 61 no PMU driver, software events only.
> [    0.041031] Hierarchical SRCU implementation.
> [    0.046210] NMI watchdog: Perf event create on CPU 0 failed with -2
> [    0.046980] NMI watchdog: Perf NMI watchdog permanetely disabled
> 
> Confused

I still can't reproduce. Can you please apply the debug patch below and
provide the output?

Thanks,

	tglx

8<-----------------

diff --git a/kernel/watchdog_hld.c b/kernel/watchdog_hld.c
index b2931154b5f2..e6c9ca516945 100644
--- a/kernel/watchdog_hld.c
+++ b/kernel/watchdog_hld.c
@@ -171,6 +171,7 @@ static int hardlockup_detector_event_create(void)
 	/* Try to register using hardware perf events */
 	evt = perf_event_create_kernel_counter(wd_attr, cpu, NULL,
 					       watchdog_overflow_callback, NULL);
+	pr_info("EVT create on CPU %u returned %p\n", cpu, evt);
 	if (IS_ERR(evt)) {
 		pr_info("Perf event create on CPU %d failed with %ld\n", cpu,
 			PTR_ERR(evt));
@@ -221,7 +222,10 @@ void hardlockup_detector_perf_cleanup(void)
 		struct perf_event *event = per_cpu(watchdog_ev, cpu);
 
 		per_cpu(watchdog_ev, cpu) = NULL;
-		perf_event_release_kernel(event);
+		pr_info("EVT on CPU %u in dead mask: %p\n",  cpu, event);
+		if (event)
+			perf_event_release_kernel(event);
+
 	}
 	cpumask_clear(&dead_events_mask);
 }

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

#1733277

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

On Fri, Sep 15, 2017 at 06:24:20PM +0200, Thomas Gleixner wrote:
>On Fri, 15 Sep 2017, Thomas Gleixner wrote:
>
>> On Fri, 15 Sep 2017, Thomas Gleixner wrote:
>>
>> > On Fri, 15 Sep 2017, kernel test robot wrote:
>> > > [    0.035023] CPU: Intel Common KVM processor (family: 0xf, model: 0x6, stepping: 0x1)
>> > > [    0.042302] Performance Events: unsupported Netburst CPU model 6 no PMU driver, software events only.
>> >
>> > Cute. So there is no supported PMU, but for some unknown reason the lockup
>> > detector can create an event, otherwise the perf availaibility check in
>> > lockup_detector_init() would fail ....
>> >
>> > Peter???
>>
>> In my VM the corresponding dmesg is:
>>
>> [    0.038086] Performance Events: unsupported p6 CPU model 61 no PMU driver, software events only.

What's your host CPU? I can reproduce it in Nehalem, Haswell and Sandy
Bridge machines with the attached script.

>> [    0.041031] Hierarchical SRCU implementation.
>> [    0.046210] NMI watchdog: Perf event create on CPU 0 failed with -2
>> [    0.046980] NMI watchdog: Perf NMI watchdog permanetely disabled
>>
>> Confused
>
>I still can't reproduce. Can you please apply the debug patch below and
>provide the output?

OK. I'll try and report back tomorrow.

Thanks,
Fengguang

>8<-----------------
>
>diff --git a/kernel/watchdog_hld.c b/kernel/watchdog_hld.c
>index b2931154b5f2..e6c9ca516945 100644
>--- a/kernel/watchdog_hld.c
>+++ b/kernel/watchdog_hld.c
>@@ -171,6 +171,7 @@ static int hardlockup_detector_event_create(void)
> 	/* Try to register using hardware perf events */
> 	evt = perf_event_create_kernel_counter(wd_attr, cpu, NULL,
> 					       watchdog_overflow_callback, NULL);
>+	pr_info("EVT create on CPU %u returned %p\n", cpu, evt);
> 	if (IS_ERR(evt)) {
> 		pr_info("Perf event create on CPU %d failed with %ld\n", cpu,
> 			PTR_ERR(evt));
>@@ -221,7 +222,10 @@ void hardlockup_detector_perf_cleanup(void)
> 		struct perf_event *event = per_cpu(watchdog_ev, cpu);
>
> 		per_cpu(watchdog_ev, cpu) = NULL;
>-		perf_event_release_kernel(event);
>+		pr_info("EVT on CPU %u in dead mask: %p\n",  cpu, event);
>+		if (event)
>+			perf_event_release_kernel(event);
>+
> 	}
> 	cpumask_clear(&dead_events_mask);
> }

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

#1733320

On Sat, 16 Sep 2017, Fengguang Wu wrote:
> > > [    0.038086] Performance Events: unsupported p6 CPU model 61 no PMU
> > > driver, software events only.
> 
> What's your host CPU? I can reproduce it in Nehalem, Haswell and Sandy
> Bridge machines with the attached script.

My bad. I booted the wrong config ....

> > > [    0.041031] Hierarchical SRCU implementation.
> > > [    0.046210] NMI watchdog: Perf event create on CPU 0 failed with -2
> > > [    0.046980] NMI watchdog: Perf NMI watchdog permanetely disabled
> > > 
> > > Confused
> > 
> > I still can't reproduce. Can you please apply the debug patch below and
> > provide the output?
> 
> OK. I'll try and report back tomorrow.

Don't bother. I found it already. On UP we have:

#define for_each_cpu(cpu, mask)               \
        for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)

which is a total fail as it breaks any code which uses for_each_cpu() or
any of the other variants on UP by assuming that all cpumask have bit 0
set.

That means any code which does not have conditional code for some of the
cpumask functions is potentially broken. Sigh.

The simple cure for the watchdog is below.

Thanks,

	tglx
8<------------------

diff --git a/kernel/watchdog_hld.c b/kernel/watchdog_hld.c
index b2931154b5f2..d4c0f75b189e 100644
--- a/kernel/watchdog_hld.c
+++ b/kernel/watchdog_hld.c
@@ -221,7 +221,12 @@ void hardlockup_detector_perf_cleanup(void)
 		struct perf_event *event = per_cpu(watchdog_ev, cpu);
 
 		per_cpu(watchdog_ev, cpu) = NULL;
-		perf_event_release_kernel(event);
+		/*
+		 * Check the event, because on UP for_each_cpu() assumes
+		 * idiotically that all masks handed in have bit 0 set.
+		 */
+		if (event)
+			perf_event_release_kernel(event);
 	}
 	cpumask_clear(&dead_events_mask);
 }

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

#1733323

On Sat, Sep 16, 2017 at 10:35 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Don't bother. I found it already. On UP we have:
>
> #define for_each_cpu(cpu, mask)               \
>         for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
>
> which is a total fail as it breaks any code which uses for_each_cpu() or
> any of the other variants on UP by assuming that all cpumask have bit 0
> set.

It's fairly fundamental. UP assumes that all CPU masks are always that
"one CPU set". Not just here - everywhere.

I guess we could somehow try to move away from that, but really, the
assumption of fixed masks ends up simplifying the code generation a
lot, so it made tons of sense back when UP was a primary target.

So it's an approach that is somewhat historical, but I'm not sure it's
worth re-visiting that old decision. People should simply not expect
to traverse over empty masks in anything that is UP.

So I suspect your perf fix is the right one, and maybe we could/should
just make people more aware of the empty cpumask issue with UP.

            Linus

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

#1733329

On Sat, 16 Sep 2017, Linus Torvalds wrote:
> On Sat, Sep 16, 2017 at 10:35 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > Don't bother. I found it already. On UP we have:
> >
> > #define for_each_cpu(cpu, mask)               \
> >         for ((cpu) = 0; (cpu) < 1; (cpu)++, (void)mask)
> >
> > which is a total fail as it breaks any code which uses for_each_cpu() or
> > any of the other variants on UP by assuming that all cpumask have bit 0
> > set.
> 
> It's fairly fundamental. UP assumes that all CPU masks are always that
> "one CPU set". Not just here - everywhere.
> 
> I guess we could somehow try to move away from that, but really, the
> assumption of fixed masks ends up simplifying the code generation a
> lot, so it made tons of sense back when UP was a primary target.
>
> So it's an approach that is somewhat historical, but I'm not sure it's
> worth re-visiting that old decision. People should simply not expect
> to traverse over empty masks in anything that is UP.
>
> So I suspect your perf fix is the right one, and maybe we could/should
> just make people more aware of the empty cpumask issue with UP.

Right, I just got a bit frightened as I really was not aware about that
'opmtimization' which means that so far I just was lucky not to trip over
it.

Thanks,

	tglx

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

#1733331

On Sat, Sep 16, 2017 at 11:12 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> So I suspect your perf fix is the right one, and maybe we could/should
>> just make people more aware of the empty cpumask issue with UP.
>
> Right, I just got a bit frightened as I really was not aware about that
> 'opmtimization' which means that so far I just was lucky not to trip over
> it.

Yeah. I can't say that I was really aware of it either in a every-day
kind of way, it was only when I looked it up that I went "Oh, right,
that's what we did".

So it's subtle and unexpected, and the saving grace is basically that
empty cpumasks are really the exception to begin with. They basically
don't happen in normal situations.

               Linus

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

#1733349

On Sat, 16 Sep 2017, Linus Torvalds wrote:
> On Sat, Sep 16, 2017 at 11:12 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >> So I suspect your perf fix is the right one, and maybe we could/should
> >> just make people more aware of the empty cpumask issue with UP.
> >
> > Right, I just got a bit frightened as I really was not aware about that
> > 'opmtimization' which means that so far I just was lucky not to trip over
> > it.
> 
> Yeah. I can't say that I was really aware of it either in a every-day
> kind of way, it was only when I looked it up that I went "Oh, right,
> that's what we did".
> 
> So it's subtle and unexpected, and the saving grace is basically that
> empty cpumasks are really the exception to begin with. They basically
> don't happen in normal situations.

Yes and no. We get more code which uses cpumasks to store state, just like
I did, and while a lot of the cpumask functions just work as expected a
subset including for_each_cpu does not. That's confusing at best and I
rather avoid the hard to debug issues on UP, which probably gets less
testing anyway.

Thanks,

	tglx

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

#1733350

On Sat, Sep 16, 2017 at 2:47 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Yes and no. We get more code which uses cpumasks to store state, just like
> I did, and while a lot of the cpumask functions just work as expected a
> subset including for_each_cpu does not. That's confusing at best and I
> rather avoid the hard to debug issues on UP, which probably gets less
> testing anyway.

I certainly agree that the UP situation has changed over the years.

But I'd hate to make all those loops etc (that the compiler can
currently almost always trivially turn into trivial unconditional
non-loops) be sometrhing that ends up testing a bit and having a
conditional.

I wonder if we could have some checking mode or something that at
least makes those things easier to notice.  But I don't see a sane way
to do that statically.

Looking at that patch of yours, it seems to depend on
'watchdog_allowed_mask' having just been initialized as empty, which
is the case that doesn't work well for UP. So there isn't even any
code to trigger on, it would have to be some added warning to all
users that does something along the lines of 'WARN_ON_ONCE(cpumask !=
1)'

.. and then hardly anybody would run that configuration anyway.

Annoying.

              Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web