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

Groups > linux.kernel > #1307794 > unrolled thread

Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint

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: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint "Shi, Yang" <yang.shi@linaro.org> - 2016-01-12 21:10 +0100
    Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint Will Deacon <will.deacon@arm.com> - 2016-01-13 11:30 +0100
      Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint "Shi, Yang" <yang.shi@linaro.org> - 2016-01-13 18:20 +0100
        Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint Will Deacon <will.deacon@arm.com> - 2016-01-13 18:30 +0100
          Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint "Shi, Yang" <yang.shi@linaro.org> - 2016-01-13 19:20 +0100

#1307794 — Re: [PATCH] arm64: reenable interrupt when handling ptrace breakpoint

On 12/21/2015 9:00 AM, Will Deacon wrote:
> On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote:
>> On Mon, 21 Dec 2015, Will Deacon wrote:
>>> +static void send_user_sigtrap(int si_code)
>>> +{
>>> +	struct pt_regs *regs = current_pt_regs();
>>> +	siginfo_t info = {
>>> +		.si_signo	= SIGTRAP,
>>> +		.si_errno	= 0,
>>> +		.si_code	= si_code,
>>> +		.si_addr	= (void __user *)instruction_pointer(regs),
>>> +	};
>>> +
>>> +	if (WARN_ON(!user_mode(regs)))
>>> +		return;
>>> +
>>> +	preempt_disable();
>>
>> That doesn't work on RT either. force_sig_info() takes task->sighand->siglock,
>> which is a 'sleeping' spinlock on RT.
>
> Ah, I missed that :/
>
>> Why would we need to disable preemption here at all? What's the problem of
>> being preempted or even migrated?
>
> There *might* not be a problem, I'm just really nervous about changing
> the behaviour on the debug path and subtly changing how ptrace behaves.
>
> My worry was that you could somehow get back into the tracer, and it
> could remove a software breakpoint in the knowledge that it wouldn't
> see any future (spurious) SIGTRAPs for that location.
>
> Without a concrete example, however, I guess I'll bite the bullet and
> enable irqs across the call to force_sig_info, since there is clearly a
> real issue here on RT.

Hi Will,

This might be buried in email storm during the holiday. Just want to 
double check the status. I'm supposed there is no objection for getting 
it merged in upstream?

Thanks,
Yang

>
> Will
>

[toc] | [next] | [standalone]

#1308256

On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote:
> On 12/21/2015 9:00 AM, Will Deacon wrote:
> >On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote:
> >>On Mon, 21 Dec 2015, Will Deacon wrote:
> >>>+static void send_user_sigtrap(int si_code)
> >>>+{
> >>>+	struct pt_regs *regs = current_pt_regs();
> >>>+	siginfo_t info = {
> >>>+		.si_signo	= SIGTRAP,
> >>>+		.si_errno	= 0,
> >>>+		.si_code	= si_code,
> >>>+		.si_addr	= (void __user *)instruction_pointer(regs),
> >>>+	};
> >>>+
> >>>+	if (WARN_ON(!user_mode(regs)))
> >>>+		return;
> >>>+
> >>>+	preempt_disable();
> >>
> >>That doesn't work on RT either. force_sig_info() takes task->sighand->siglock,
> >>which is a 'sleeping' spinlock on RT.
> >
> >Ah, I missed that :/
> >
> >>Why would we need to disable preemption here at all? What's the problem of
> >>being preempted or even migrated?
> >
> >There *might* not be a problem, I'm just really nervous about changing
> >the behaviour on the debug path and subtly changing how ptrace behaves.
> >
> >My worry was that you could somehow get back into the tracer, and it
> >could remove a software breakpoint in the knowledge that it wouldn't
> >see any future (spurious) SIGTRAPs for that location.
> >
> >Without a concrete example, however, I guess I'll bite the bullet and
> >enable irqs across the call to force_sig_info, since there is clearly a
> >real issue here on RT.
> 
> This might be buried in email storm during the holiday. Just want to double
> check the status. I'm supposed there is no objection for getting it merged
> in upstream?

Sorry, when you replied with:

> I think we could just extend the "signal delay send" approach from x86-64
> to arm64, which is currently used by x86-64 on -rt kernel only.

I understood that you were going to fix -rt, so I dropped this pending
anything more from you.

What's the plan?

Will

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

#1308652

On 1/13/2016 2:26 AM, Will Deacon wrote:
> On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote:
>> On 12/21/2015 9:00 AM, Will Deacon wrote:
>>> On Mon, Dec 21, 2015 at 05:51:22PM +0100, Thomas Gleixner wrote:
>>>> On Mon, 21 Dec 2015, Will Deacon wrote:
>>>>> +static void send_user_sigtrap(int si_code)
>>>>> +{
>>>>> +	struct pt_regs *regs = current_pt_regs();
>>>>> +	siginfo_t info = {
>>>>> +		.si_signo	= SIGTRAP,
>>>>> +		.si_errno	= 0,
>>>>> +		.si_code	= si_code,
>>>>> +		.si_addr	= (void __user *)instruction_pointer(regs),
>>>>> +	};
>>>>> +
>>>>> +	if (WARN_ON(!user_mode(regs)))
>>>>> +		return;
>>>>> +
>>>>> +	preempt_disable();
>>>>
>>>> That doesn't work on RT either. force_sig_info() takes task->sighand->siglock,
>>>> which is a 'sleeping' spinlock on RT.
>>>
>>> Ah, I missed that :/
>>>
>>>> Why would we need to disable preemption here at all? What's the problem of
>>>> being preempted or even migrated?
>>>
>>> There *might* not be a problem, I'm just really nervous about changing
>>> the behaviour on the debug path and subtly changing how ptrace behaves.
>>>
>>> My worry was that you could somehow get back into the tracer, and it
>>> could remove a software breakpoint in the knowledge that it wouldn't
>>> see any future (spurious) SIGTRAPs for that location.
>>>
>>> Without a concrete example, however, I guess I'll bite the bullet and
>>> enable irqs across the call to force_sig_info, since there is clearly a
>>> real issue here on RT.
>>
>> This might be buried in email storm during the holiday. Just want to double
>> check the status. I'm supposed there is no objection for getting it merged
>> in upstream?
>
> Sorry, when you replied with:
>
>> I think we could just extend the "signal delay send" approach from x86-64
>> to arm64, which is currently used by x86-64 on -rt kernel only.
>
> I understood that you were going to fix -rt, so I dropped this pending
> anything more from you.
>
> What's the plan?

Sorry for the confusion. The "signal delay send" approach used by x86-64 
-rt should be not necessary for arm64 right now. Reenabling interrupt is 
still the preferred approach.

Since x86-64 has per-CPU IST exception stack, so preemption has to be 
disabled all the time. However, it is not applicable to other 
architectures for now, including arm64.

Thanks,
Yang

>
> Will
>

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

#1308662

On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote:
> On 1/13/2016 2:26 AM, Will Deacon wrote:
> >On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote:
> >>This might be buried in email storm during the holiday. Just want to double
> >>check the status. I'm supposed there is no objection for getting it merged
> >>in upstream?
> >
> >Sorry, when you replied with:
> >
> >>I think we could just extend the "signal delay send" approach from x86-64
> >>to arm64, which is currently used by x86-64 on -rt kernel only.
> >
> >I understood that you were going to fix -rt, so I dropped this pending
> >anything more from you.
> >
> >What's the plan?
> 
> Sorry for the confusion. The "signal delay send" approach used by x86-64 -rt
> should be not necessary for arm64 right now. Reenabling interrupt is still
> the preferred approach.
> 
> Since x86-64 has per-CPU IST exception stack, so preemption has to be
> disabled all the time. However, it is not applicable to other architectures
> for now, including arm64.

Actually, we grew support for a separate IRQ stack in the recent merge
window. Does that change things here, or are you referring to something
else?

Will

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

#1308703

On 1/13/2016 9:23 AM, Will Deacon wrote:
> On Wed, Jan 13, 2016 at 09:17:46AM -0800, Shi, Yang wrote:
>> On 1/13/2016 2:26 AM, Will Deacon wrote:
>>> On Tue, Jan 12, 2016 at 11:59:54AM -0800, Shi, Yang wrote:
>>>> This might be buried in email storm during the holiday. Just want to double
>>>> check the status. I'm supposed there is no objection for getting it merged
>>>> in upstream?
>>>
>>> Sorry, when you replied with:
>>>
>>>> I think we could just extend the "signal delay send" approach from x86-64
>>>> to arm64, which is currently used by x86-64 on -rt kernel only.
>>>
>>> I understood that you were going to fix -rt, so I dropped this pending
>>> anything more from you.
>>>
>>> What's the plan?
>>
>> Sorry for the confusion. The "signal delay send" approach used by x86-64 -rt
>> should be not necessary for arm64 right now. Reenabling interrupt is still
>> the preferred approach.
>>
>> Since x86-64 has per-CPU IST exception stack, so preemption has to be
>> disabled all the time. However, it is not applicable to other architectures
>> for now, including arm64.
>
> Actually, we grew support for a separate IRQ stack in the recent merge
> window. Does that change things here, or are you referring to something
> else?

Had a quick look at the patches, it looks the irq stack is not nestable 
and it switches back to the original stack as long as irq handler is 
done before preempt happens. So, it sounds it won't change things here.

Thanks.,
Yang

>
> Will
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web