All of lore.kernel.org
 help / color / mirror / Atom feed
From: wangnan0@huawei.com (Wangnan (F))
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2] arm64: Store breakpoint single step state into pstate
Date: Mon, 18 Jan 2016 19:39:06 +0800	[thread overview]
Message-ID: <569CCEDA.6040103@huawei.com> (raw)
In-Reply-To: <20160112170650.GI15737@arm.com>



On 2016/1/13 1:06, Will Deacon wrote:
> On Tue, Jan 05, 2016 at 01:06:15PM +0800, Wangnan (F) wrote:
>> On 2016/1/5 0:55, Will Deacon wrote:
>>> The problem seems to be that we take the debug exception before the
>>> breakpointed instruction has been executed and call perf_bp_event at
>>> that moment, so when we single-step the faulting instruction we actually
>>> step into the SIGIO handler and end up getting stuck.
>>>
>>> Your fix doesn't really address this afaict, in that you don't (can't?)
>>> handle:
>>>
>>>    * A longjmp out of a signal handler
>>>    * A watchpoint and a breakpoint that fire on the same instruction
>>>    * User-controlled single-step from a signal handler that enables a
>>>      breakpoint explicitly
>>>    * Nested signals
>> Please have a look at [1], which I improve test__bp_signal() to
>> check bullet 2 and 4 you mentioned above. Seems my fix is correct.
>>
>> [1] http://lkml.kernel.org/g/1451969880-14877-1-git-send-email-wangnan0 at huawei.com
> I'm still really uneasy about this change. Pairing up the signal delivery
> with the sigreturn to keep track of the debug state is extremely fragile
> and I'm not keen on adding this logic there. I also think we need to
> track the address that the breakpoint is originally taken on so that we
> can only perform the extra sigreturn work if we're returning to the same
> instruction. Furthermore, I wouldn't want to do this for signals other
> than those generated directly by a breakpoint.
>
> An alternative would be to postpone the signal delivery until after the
> stepping has been taken care of, but that's a change in ABI and I worry
> we'll break somebody relying on the current behaviour.
>
> What exactly does x86 do? I couldn't figure it out from the code.

Actually x86 does similar thing as what this patch does.

RF bit in x86_64's eflags prohibit debug exception raises. It is set by
x86_64's debug handler to avoid recursion. x86_64 need setting this bit
in breakpoint handler because it needs to jump back to original
instruction and single-step on it, similar to ARM64.

The RF bit in eflags records a state that the process shouldn't generate
debug exception. It is part of the state of a process, and should be saved
and cleared if transfers to signal handler.

This patch does the same thing: create two bits in pstate to indicate
the states that 'a process should not raises watchpoint/breakpoint 
exceptions',
maintains them in kernel, cleans them for signal handler and save them 
in signal
frame.

Thank you.

WARNING: multiple messages have this Message-ID (diff)
From: "Wangnan (F)" <wangnan0@huawei.com>
To: Will Deacon <will.deacon@arm.com>
Cc: <takahiro.akashi@linaro.org>, <guohanjun@huawei.com>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <pi3orama@163.com>,
	Fengguang Wu <fengguang.wu@intel.com>,
	Jiri Olsa <jolsa@kernel.org>
Subject: Re: [PATCH v2] arm64: Store breakpoint single step state into pstate
Date: Mon, 18 Jan 2016 19:39:06 +0800	[thread overview]
Message-ID: <569CCEDA.6040103@huawei.com> (raw)
In-Reply-To: <20160112170650.GI15737@arm.com>



On 2016/1/13 1:06, Will Deacon wrote:
> On Tue, Jan 05, 2016 at 01:06:15PM +0800, Wangnan (F) wrote:
>> On 2016/1/5 0:55, Will Deacon wrote:
>>> The problem seems to be that we take the debug exception before the
>>> breakpointed instruction has been executed and call perf_bp_event at
>>> that moment, so when we single-step the faulting instruction we actually
>>> step into the SIGIO handler and end up getting stuck.
>>>
>>> Your fix doesn't really address this afaict, in that you don't (can't?)
>>> handle:
>>>
>>>    * A longjmp out of a signal handler
>>>    * A watchpoint and a breakpoint that fire on the same instruction
>>>    * User-controlled single-step from a signal handler that enables a
>>>      breakpoint explicitly
>>>    * Nested signals
>> Please have a look at [1], which I improve test__bp_signal() to
>> check bullet 2 and 4 you mentioned above. Seems my fix is correct.
>>
>> [1] http://lkml.kernel.org/g/1451969880-14877-1-git-send-email-wangnan0@huawei.com
> I'm still really uneasy about this change. Pairing up the signal delivery
> with the sigreturn to keep track of the debug state is extremely fragile
> and I'm not keen on adding this logic there. I also think we need to
> track the address that the breakpoint is originally taken on so that we
> can only perform the extra sigreturn work if we're returning to the same
> instruction. Furthermore, I wouldn't want to do this for signals other
> than those generated directly by a breakpoint.
>
> An alternative would be to postpone the signal delivery until after the
> stepping has been taken care of, but that's a change in ABI and I worry
> we'll break somebody relying on the current behaviour.
>
> What exactly does x86 do? I couldn't figure it out from the code.

Actually x86 does similar thing as what this patch does.

RF bit in x86_64's eflags prohibit debug exception raises. It is set by
x86_64's debug handler to avoid recursion. x86_64 need setting this bit
in breakpoint handler because it needs to jump back to original
instruction and single-step on it, similar to ARM64.

The RF bit in eflags records a state that the process shouldn't generate
debug exception. It is part of the state of a process, and should be saved
and cleared if transfers to signal handler.

This patch does the same thing: create two bits in pstate to indicate
the states that 'a process should not raises watchpoint/breakpoint 
exceptions',
maintains them in kernel, cleans them for signal handler and save them 
in signal
frame.

Thank you.

  parent reply	other threads:[~2016-01-18 11:39 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-23  8:52 [RESEND PATCH] arm64: Store breakpoint single step state into pstate Wang Nan
2015-12-23  8:52 ` Wang Nan
2015-12-23 10:44 ` kbuild test robot
2015-12-23 10:44   ` kbuild test robot
2015-12-24  1:42 ` [PATCH v2] " Wang Nan
2015-12-24  1:42   ` Wang Nan
2016-01-04 16:55   ` Will Deacon
2016-01-04 16:55     ` Will Deacon
2016-01-05  1:41     ` Wangnan (F)
2016-01-05  1:41       ` Wangnan (F)
2016-01-05  4:58     ` [RFC PATCH] arm64: perf test: Improbe bp_signal Wang Nan
2016-01-05  4:58       ` Wang Nan
2016-01-05  5:09       ` Wangnan (F)
2016-01-05  5:09         ` Wangnan (F)
2016-01-05  8:53       ` Jiri Olsa
2016-01-05  8:53         ` Jiri Olsa
2016-01-05  9:00       ` Jiri Olsa
2016-01-05  9:00         ` Jiri Olsa
2016-01-05  9:05       ` Jiri Olsa
2016-01-05  9:05         ` Jiri Olsa
2016-01-05  9:09       ` Jiri Olsa
2016-01-05  9:09         ` Jiri Olsa
2016-01-05  5:06     ` [PATCH v2] arm64: Store breakpoint single step state into pstate Wangnan (F)
2016-01-05  5:06       ` Wangnan (F)
2016-01-12 17:06       ` Will Deacon
2016-01-12 17:06         ` Will Deacon
2016-01-15  8:20         ` xiakaixu
2016-01-15  8:20           ` xiakaixu
2016-01-21  8:06           ` xiakaixu
2016-01-21  8:06             ` xiakaixu
2016-01-18 11:39         ` Wangnan (F) [this message]
2016-01-18 11:39           ` Wangnan (F)
2016-01-05  9:57     ` [RFC PATCH v2] perf test: Improve bp_signal Wang Nan
2016-01-05  9:57       ` Wang Nan
2016-01-05 10:07       ` Jiri Olsa
2016-01-05 10:07         ` Jiri Olsa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=569CCEDA.6040103@huawei.com \
    --to=wangnan0@huawei.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.