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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 04 Oct 2016 15:29:33 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Peter Zijlstra <peterz@...radead.org>, Jiri Olsa <jolsa@...hat.com>
Cc:     Michael Neuling <mikey@...ling.org>,
        Paul Mackerras <paulus@...ba.org>,
        Jiri Olsa <jolsa@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jan Stancek <jstancek@...hat.com>
Subject: Re: [PATCH] perf powerpc: Don't call perf_event_disable from atomic context

Peter Zijlstra <peterz@...radead.org> writes:

> On Mon, Oct 03, 2016 at 03:29:32PM +0200, Jiri Olsa wrote:
>> On Fri, Sep 23, 2016 at 06:37:47PM +0200, Peter Zijlstra wrote:
>> > On Wed, Sep 21, 2016 at 03:55:34PM +0200, Jiri Olsa wrote:
>> > >   stack backtrace:
>> > >   CPU: 9 PID: 2998 Comm: ls Tainted: G        W       4.8.0-rc5+ #7
>> > >   Call Trace:
>> > >   [c0000002f7933150] [c00000000094b1f8] .dump_stack+0xe0/0x14c (unreliable)
>> > >   [c0000002f79331e0] [c00000000013c468] .lockdep_rcu_suspicious+0x138/0x180
>> > >   [c0000002f7933270] [c0000000001005d8] .___might_sleep+0x278/0x2e0
>> > >   [c0000002f7933300] [c000000000935584] .mutex_lock_nested+0x64/0x5a0
>> > >   [c0000002f7933410] [c00000000023084c] .perf_event_ctx_lock_nested+0x16c/0x380
>> > >   [c0000002f7933500] [c000000000230a80] .perf_event_disable+0x20/0x60
>> > >   [c0000002f7933580] [c00000000093aeec] .hw_breakpoint_handler+0x29c/0x2b0
>> > >   [c0000002f7933630] [c0000000000f671c] .notifier_call_chain+0x7c/0xf0
>> > >   [c0000002f79336d0] [c0000000000f6abc] .__atomic_notifier_call_chain+0xbc/0x1c0
>> > >   [c0000002f7933780] [c0000000000f6c40] .notify_die+0x70/0xd0
>> > >   [c0000002f7933820] [c00000000001a74c] .do_break+0x4c/0x100
>> > >   [c0000002f7933920] [c0000000000089fc] handle_dabr_fault+0x14/0x48
>> > 
>> > Well, that lockdep warning only says you should not be taking sleeping
>> > locks while holding rcu_read_lock(), which is true. It does not say the
>> > context you're doing this is cannot sleep.
>> > 
>> > I'm not familiar enough with the PPC stuff to tell if the DIE_DABR_MATCH
>> > trap context is atomic or not and this Changelog doesn't tell me.
>> 
>> ping
>
> So I think all the DIE notifiers are atomic, which means this would
> indeed be the thing to do. That said, I didn't see anything similar on
> other BP implementations.

Seems everyone is being called from the same notifier, which is atomic,
but powerpc is the only arch that does perf_event_disable().

> So it would be good to also explain why PPC needs this in the first
> place.

Unfortunately I don't really know the code, and the original author is AWOL.

But AFAICS perf_event_disable() is only called here:

	if (!stepped) {
		WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
			"0x%lx will be disabled.", info->address);
		perf_event_disable(bp);
		goto out;
	}

Which is where we cope with the possibility that we couldn't emulate the
instruction that hit the breakpoint. Seems that is not an issue on x86,
or it's handled elsewhere?

We should fix emulate_step() if it failed to emulate something it
should have, but there will always be the possibility that it fails.

Instead of calling perf_event_disable() we could just add a flag to
arch_hw_breakpoint that says we hit an error on the event, and block
reinstalling it in arch_install_hw_breakpoint().

cheers

Powered by blists - more mailing lists