Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]

If single-byte updates weren't atomic, then the int3 scheme would not be possible in the first place.  Of course, if you want there to be a synchronization point beyond which the modification is guaranteed to have affected all CPUs, you need an IPI-IRET on all CPUs.

The other thing to watch out for is that the CPU itself is subject to text modification through a different alias, which means some of the hardware SMC protections are ineffective.

"Mathieu Desnoyers" <mathieu.desnoyers@polymtl.ca> wrote:

>* H. Peter Anvin (hpa@zytor.com) wrote:
>> On 01/14/2010 07:32 AM, Steven Rostedt wrote:
>> >> +
>> >> +	/* Replacing 1 byte can be done atomically. */
>> >> +	if (unlikely(len <= 1))
>> >> +		return text_poke(addr, opcode, len);
>> > 
>> > This part bothers me. The text_poke just writes over the text directly
>> > (using a separate mapping). But if that memory is in the pipeline of
>> > another CPU, I think this could cause a GPF.
>> > 
>> 
>> Could you clarify why you think that?
>
>Basically, what Steven and I were concerned about in this particular
>patch version is the fact that this code took a "shortcut" for
>single-byte text modification, thus bypassing the int3-bypass scheme
>altogether.
>
>As mere atomicity of the modification is not the only concern here
>(because we also have to deal with instruction trace cache coherency and
>so forth), then the int3 breakpoint scheme is, I think, also needed for
>single-byte updates.
>
>Thanks,
>
>Mathieu
>
>> 
>> 	-hpa
>> 
>> -- 
>> H. Peter Anvin, Intel Open Source Technology Center
>> I work for Intel.  I don't speak on their behalf.
>> 
>
>-- 
>Mathieu Desnoyers
>OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

--
Sent from my mobile phone, pardon any lack of formatting.

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Mathieu Desnoyers]

* Masami Hiramatsu (mhiramat@redhat.com) wrote:
> Mathieu Desnoyers wrote:
> >> It is *not* necessary to wait for the breakpoint handlers to return, as
> >> long as they will get to IRET eventually, since IRET is a jump and a
> >> serializing instruction.
> > 
> > Ah, I see. So the added smp_mb() would not be needed then, as long as we
> > know that the other CPUs either are currently running the IPI handler or
> > have executed it. IOW: they will execute IRET very soon or they just
> > executed it since the int3 have been written. I am a bit concerned about
> > NMIs coming in this race window, but as they need to have started after
> > we have put the breakpoint, that should be OK. (note: entry_*.S
> > modifications are needed to support nesting breakpoint handlers in NMIs)
> 
> Hmm, if we support this to modify NMI code, it seems that we need to
> support not only nesting breakpoint handling but also nesting NMIs,
> because nesting NMI is unblocked when next IRET (of breakpoint) is
> issued.
> 
> From Intel's Software Developer’s Manual Vol.3A 5.7.1 Handling Multiple NMIs
> said below.
> ---
> While an NMI interrupt handler is executing, the processor disables additional calls to
> the NMI handler until the next IRET instruction is executed. This blocking of subse-
> quent NMIs prevents stacking up calls to the NMI handler. [...]
> ---
> 
> I assume that your below patch tried to solve this issue, right?
> http://lkml.indiana.edu/hypermail/linux/kernel/0804.1/0965.html
> 

Yep. (sorry about late reply).

Mathieu

> Thank you,
> 
> -- 
> Masami Hiramatsu
> 
> Software Engineer
> Hitachi Computer Products (America), Inc.
> Software Solutions Division
> 
> e-mail: mhiramat@redhat.com
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Mathieu Desnoyers]

* H. Peter Anvin (hpa@zytor.com) wrote:
> On 01/18/2010 08:52 AM, Mathieu Desnoyers wrote:
> >>
> >> This really doesn't make much sense to me.  The whole basis for the int3
> >> scheme itself is that single-byte updates are atomic, so if single-byte
> >> updates can't work -- and as I stated, we at Intel OTC currently believe
> >> it safe -- then int3 can't work either.
> > 
> > The additional characteristic of the int3 instruction (compared to the
> > general case of a single-byte instruction) is that, when executed, it
> > will trigger a trap, run a trap handler and return to the original code,
> > typically with iret. This therefore implies that a serializing
> > instruction is executed before returning to the instructions following
> > the modification site when the breakpoint is hit.
> > 
> > So I hand out to Intel's expertise the question of whether single-byte
> > instruction modification is safe or not in the general case. I'm just
> > pointing out that I can very well imagine an aggressive superscalar
> > architecture for which pipeline structure would support single-byte int3
> > patching without any problem due to the implied serialization, but would
> > not support the general-case single-byte modification due to its lack of
> > serialization.
> > 
> 
> This is utter and complete nonsense.   You seem to think that everything
> is guaranteed to hit the breakpoint, which is obviously false.

What I discuss above is: what actually happens when the breakpoint is
hit.

I'm doing no assumption about whether it is hit or not. In the int3+IPI
broadcast scheme, every cpu receive an IPI between seeing the old and
new instructions. Only *some* cpus *may* hit the breakpoint that is put
there temporarily.

> Furthermore, until you have done the serialization, you're not
> guaranteed the *breakpoint* is seen,

Agreed,

> so you have the same condition.

Hrm ? Same as what exactly ? We have either the old instruction in place
or the breakpoint (before the serialization). After the serialization,
we have either the breakpoint or the new instruction.

What I am pointing out is that specifically turning a 1-byte instruction
into a breakpoint can be safer than turning it into another 1-byte
instruction directly, because *if* cpus hit the breakpoint, they *will*
issue a synchronizing instruction at that point (implied by the
breakpoint). This is not the case if you just modify the 1-byte
instruction in place.

> 
> > As we might have to port this algorithm to Itanium in a near future, I
> > prefer to stay on the safe side. Intel's "by the book" recommendation is
> > more or less that a serializing instruction must be executed on all CPUs
> > before new code is executed, without mention of single-vs-multi byte
> > instructions. The int3-based bypass follows this requirement, but the
> > single-byte code patching does not.
> > 
> > Unless there is a visible performance gain to special-case the
> > single-byte instruction, I would recommend to stick to the safest
> > solution, which follows Intel "official" guide-lines too.
> 
> No, it doesn't.  The only thing that follows the "official" guidelines
> is stop_machine.
> 
> As far as other architectures are concerned, other architectures can
> have very different and much stricter rules for I/D coherence.  Trying
> to extrapolate from the x86 rules is aggravated insanity.

I agree that official Intel guidelines for XMC only discuss the
stop_machine() scheme. OK then, let's see how patching single-byte
instructions deals with the official _uniprocessor_ self-modifying code
guidelines.

(ref. http://www.intel.com/Assets/PDF/specupdate/318586.pdf
7.1.3 Handling Self- and Cross-Modifying Code)

(* OPTION 1 *)
Store modified code (as data) into code segment;
Jump to new code or an intermediate location;
Execute new code;

(* OPTION 2 *)
Store modified code (as data) into code segment;
Execute a serializing instruction; (* For example, CPUID instruction *)
Execute new code;

As you can see, if we self-modify the code on a single cpu machine with
text_poke directly, even for a single-byte instruction, we _have_ to
guarantee that either a jump or a serializing instruction is issued
before the new code is executed.

What I discussed above was that int3 is a special-case, because it
generates a trap, and therefore jumps to a different location.

So, back to the case where we could "simply patch-in any single-byte
instruction in a SMP system", I argue that this is against the
uniprocessor part of the errata, which clearly also applies to SMP.

By the way, I've looked at the Itanium documents a few years ago, and
I have not seen any reason at that time why the breakpoint+IPI scheme
would not work if we additionally perform the appropriate I and D cache
flushes. The rest of the requirements are _very_ similar.

Thanks,

Mathieu


> 
> 	-hpa

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[H. Peter Anvin]

On 01/18/2010 12:53 PM, Masami Hiramatsu wrote:
>>
>> This is utter and complete nonsense.   You seem to think that everything
>> is guaranteed to hit the breakpoint, which is obviously false.
>> Furthermore, until you have done the serialization, you're not
>> guaranteed the *breakpoint* is seen, so you have the same condition.
> 
> In that time frame, I guess that the processor sees non-modified
> instruction and executes it. Since we'll wait until serializing on
> each processor, I think it is OK for int3-bypass method.
> 
> (Of course, this can depend on chip, it is possible that there is a chip
>  which causes a fault when it has a cache-discarding signal on current-
>  instruction decoding slot. That's also why we are asking this method
>  is OK for x86 processors.)
> 

Yes, it is possible, however, if that was the case, then int3 wouldn't
work either.  As I said, to the best of our knowledge, at least Intel
processors are okay for a single-byte update (I will wait to try to
state the full general rule until it has been officially approved or
killed.)

	-hpa

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Masami Hiramatsu]

H. Peter Anvin wrote:
> On 01/18/2010 08:52 AM, Mathieu Desnoyers wrote:
>>>
>>> This really doesn't make much sense to me.  The whole basis for the int3
>>> scheme itself is that single-byte updates are atomic, so if single-byte
>>> updates can't work -- and as I stated, we at Intel OTC currently believe
>>> it safe -- then int3 can't work either.
>>
>> The additional characteristic of the int3 instruction (compared to the
>> general case of a single-byte instruction) is that, when executed, it
>> will trigger a trap, run a trap handler and return to the original code,
>> typically with iret. This therefore implies that a serializing
>> instruction is executed before returning to the instructions following
>> the modification site when the breakpoint is hit.
>>
>> So I hand out to Intel's expertise the question of whether single-byte
>> instruction modification is safe or not in the general case. I'm just
>> pointing out that I can very well imagine an aggressive superscalar
>> architecture for which pipeline structure would support single-byte int3
>> patching without any problem due to the implied serialization, but would
>> not support the general-case single-byte modification due to its lack of
>> serialization.
>>
> 
> This is utter and complete nonsense.   You seem to think that everything
> is guaranteed to hit the breakpoint, which is obviously false.
> Furthermore, until you have done the serialization, you're not
> guaranteed the *breakpoint* is seen, so you have the same condition.

In that time frame, I guess that the processor sees non-modified
instruction and executes it. Since we'll wait until serializing on
each processor, I think it is OK for int3-bypass method.

(Of course, this can depend on chip, it is possible that there is a chip
 which causes a fault when it has a cache-discarding signal on current-
 instruction decoding slot. That's also why we are asking this method
 is OK for x86 processors.)

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[H. Peter Anvin]

On 01/18/2010 08:52 AM, Mathieu Desnoyers wrote:
>>
>> This really doesn't make much sense to me.  The whole basis for the int3
>> scheme itself is that single-byte updates are atomic, so if single-byte
>> updates can't work -- and as I stated, we at Intel OTC currently believe
>> it safe -- then int3 can't work either.
> 
> The additional characteristic of the int3 instruction (compared to the
> general case of a single-byte instruction) is that, when executed, it
> will trigger a trap, run a trap handler and return to the original code,
> typically with iret. This therefore implies that a serializing
> instruction is executed before returning to the instructions following
> the modification site when the breakpoint is hit.
> 
> So I hand out to Intel's expertise the question of whether single-byte
> instruction modification is safe or not in the general case. I'm just
> pointing out that I can very well imagine an aggressive superscalar
> architecture for which pipeline structure would support single-byte int3
> patching without any problem due to the implied serialization, but would
> not support the general-case single-byte modification due to its lack of
> serialization.
> 

This is utter and complete nonsense.   You seem to think that everything
is guaranteed to hit the breakpoint, which is obviously false.
Furthermore, until you have done the serialization, you're not
guaranteed the *breakpoint* is seen, so you have the same condition.

> As we might have to port this algorithm to Itanium in a near future, I
> prefer to stay on the safe side. Intel's "by the book" recommendation is
> more or less that a serializing instruction must be executed on all CPUs
> before new code is executed, without mention of single-vs-multi byte
> instructions. The int3-based bypass follows this requirement, but the
> single-byte code patching does not.
> 
> Unless there is a visible performance gain to special-case the
> single-byte instruction, I would recommend to stick to the safest
> solution, which follows Intel "official" guide-lines too.

No, it doesn't.  The only thing that follows the "official" guidelines
is stop_machine.

As far as other architectures are concerned, other architectures can
have very different and much stricter rules for I/D coherence.  Trying
to extrapolate from the x86 rules is aggravated insanity.

	-hpa

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Mathieu Desnoyers]

* Masami Hiramatsu (mhiramat@redhat.com) wrote:
> Mathieu Desnoyers wrote:
> > * Arjan van de Ven (arjan@infradead.org) wrote:
> >> On Mon, 18 Jan 2010 10:59:30 -0500
> >> Masami Hiramatsu <mhiramat@redhat.com> wrote:
> >>
> >>> Yeah, so in the latest patch, I updated it to use int3 even if
> >>> len == 1. :-)
> >>>
> >>
> >>
> >> int3 is not making a difference for your case; there is no guarantee
> >> that the other processor even sees the "int3 inbetween state" at all;
> >> if it's not safe without int3 then it won't be safe with int3 either.
> > 
> > What Masami means is that he updated his patch to use the int3+IPI
> > broadcast scheme.
> 
> Right.
> 
> > 
> > Therefore, the CPUs not seeing the int3 inbetween state will be forced
> > to issue a serializing instruction while the int3 is in place anyway.
> 
> By the way, in kprobes, we just use a text_poke() to put int3.
> I assume that we'd better send IPI afterward, wouldn't it?

Only if you need to ensure that you reached a state where all CPUs are
seeing the int3.

Note that kprobes already issues synchronize_sched() after breakpoint
removal, which should have a similar effect. However, AFAIK, it does not
synchronize after inserting the int3.

Thanks,

Mathieu

> 
> Thank you,
> 
> -- 
> Masami Hiramatsu
> 
> Software Engineer
> Hitachi Computer Products (America), Inc.
> Software Solutions Division
> 
> e-mail: mhiramat@redhat.com
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Masami Hiramatsu]

Mathieu Desnoyers wrote:
> * Arjan van de Ven (arjan@infradead.org) wrote:
>> On Mon, 18 Jan 2010 10:59:30 -0500
>> Masami Hiramatsu <mhiramat@redhat.com> wrote:
>>
>>> Yeah, so in the latest patch, I updated it to use int3 even if
>>> len == 1. :-)
>>>
>>
>>
>> int3 is not making a difference for your case; there is no guarantee
>> that the other processor even sees the "int3 inbetween state" at all;
>> if it's not safe without int3 then it won't be safe with int3 either.
> 
> What Masami means is that he updated his patch to use the int3+IPI
> broadcast scheme.

Right.

> 
> Therefore, the CPUs not seeing the int3 inbetween state will be forced
> to issue a serializing instruction while the int3 is in place anyway.

By the way, in kprobes, we just use a text_poke() to put int3.
I assume that we'd better send IPI afterward, wouldn't it?

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Mathieu Desnoyers]

* Arjan van de Ven (arjan@infradead.org) wrote:
> On Mon, 18 Jan 2010 10:59:30 -0500
> Masami Hiramatsu <mhiramat@redhat.com> wrote:
> 
> > Yeah, so in the latest patch, I updated it to use int3 even if
> > len == 1. :-)
> > 
> 
> 
> int3 is not making a difference for your case; there is no guarantee
> that the other processor even sees the "int3 inbetween state" at all;
> if it's not safe without int3 then it won't be safe with int3 either.

What Masami means is that he updated his patch to use the int3+IPI
broadcast scheme.

Therefore, the CPUs not seeing the int3 inbetween state will be forced
to issue a serializing instruction while the int3 is in place anyway.

Thanks,

Mathieu

> 
> 
> -- 
> Arjan van de Ven 	Intel Open Source Technology Centre
> For development, discussion and tips for power savings, 
> visit http://www.lesswatts.org

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Mathieu Desnoyers]

* H. Peter Anvin (hpa@zytor.com) wrote:
> On 01/18/2010 07:59 AM, Masami Hiramatsu wrote:
> >>>>>
> >>>>> This part bothers me. The text_poke just writes over the text
> >>>>> directly (using a separate mapping). But if that memory is in the
> >>>>> pipeline of another CPU, I think this could cause a GPF.
> >>>>>
> >>>>
> >>>> Could you clarify why you think that?
> >>>
> >>> Basically, what Steven and I were concerned about in this particular
> >>> patch version is the fact that this code took a "shortcut" for
> >>> single-byte text modification, thus bypassing the int3-bypass scheme
> >>> altogether.
> >>
> >> single byte instruction updates are likely 100x safer than any scheme
> >> of multi-byte instruction scheme that I have seen, other than a full
> >> stop_machine().
> >>
> >> That does not mean it is safe, it just means it's an order of
> >> complexity less to analyze ;-)
> > 
> > Yeah, so in the latest patch, I updated it to use int3 even if
> > len == 1. :-)
> > 
> 
> This really doesn't make much sense to me.  The whole basis for the int3
> scheme itself is that single-byte updates are atomic, so if single-byte
> updates can't work -- and as I stated, we at Intel OTC currently believe
> it safe -- then int3 can't work either.

The additional characteristic of the int3 instruction (compared to the
general case of a single-byte instruction) is that, when executed, it
will trigger a trap, run a trap handler and return to the original code,
typically with iret. This therefore implies that a serializing
instruction is executed before returning to the instructions following
the modification site when the breakpoint is hit.

So I hand out to Intel's expertise the question of whether single-byte
instruction modification is safe or not in the general case. I'm just
pointing out that I can very well imagine an aggressive superscalar
architecture for which pipeline structure would support single-byte int3
patching without any problem due to the implied serialization, but would
not support the general-case single-byte modification due to its lack of
serialization.

As we might have to port this algorithm to Itanium in a near future, I
prefer to stay on the safe side. Intel's "by the book" recommendation is
more or less that a serializing instruction must be executed on all CPUs
before new code is executed, without mention of single-vs-multi byte
instructions. The int3-based bypass follows this requirement, but the
single-byte code patching does not.

Unless there is a visible performance gain to special-case the
single-byte instruction, I would recommend to stick to the safest
solution, which follows Intel "official" guide-lines too.

Thanks,

Mathieu

> 
> The one thing to watch out for is that unless you force an IPI/IRET
> cycle afterwards, you can't know when any particular remote processor
> will see the update.
> 
> 	-hpa
> 
> -- 
> H. Peter Anvin, Intel Open Source Technology Center
> I work for Intel.  I don't speak on their behalf.
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Arjan van de Ven]

On Mon, 18 Jan 2010 10:59:30 -0500
Masami Hiramatsu <mhiramat@redhat.com> wrote:

> Yeah, so in the latest patch, I updated it to use int3 even if
> len == 1. :-)
> 


int3 is not making a difference for your case; there is no guarantee
that the other processor even sees the "int3 inbetween state" at all;
if it's not safe without int3 then it won't be safe with int3 either.


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[H. Peter Anvin]

On 01/18/2010 07:59 AM, Masami Hiramatsu wrote:
>>>>>
>>>>> This part bothers me. The text_poke just writes over the text
>>>>> directly (using a separate mapping). But if that memory is in the
>>>>> pipeline of another CPU, I think this could cause a GPF.
>>>>>
>>>>
>>>> Could you clarify why you think that?
>>>
>>> Basically, what Steven and I were concerned about in this particular
>>> patch version is the fact that this code took a "shortcut" for
>>> single-byte text modification, thus bypassing the int3-bypass scheme
>>> altogether.
>>
>> single byte instruction updates are likely 100x safer than any scheme
>> of multi-byte instruction scheme that I have seen, other than a full
>> stop_machine().
>>
>> That does not mean it is safe, it just means it's an order of
>> complexity less to analyze ;-)
> 
> Yeah, so in the latest patch, I updated it to use int3 even if
> len == 1. :-)
> 

This really doesn't make much sense to me.  The whole basis for the int3
scheme itself is that single-byte updates are atomic, so if single-byte
updates can't work -- and as I stated, we at Intel OTC currently believe
it safe -- then int3 can't work either.

The one thing to watch out for is that unless you force an IPI/IRET
cycle afterwards, you can't know when any particular remote processor
will see the update.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Masami Hiramatsu]

Arjan van de Ven wrote:
> On Sun, 17 Jan 2010 13:55:39 -0500
> Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:
> 
>> * H. Peter Anvin (hpa@zytor.com) wrote:
>>> On 01/14/2010 07:32 AM, Steven Rostedt wrote:
>>>>> +
>>>>> +	/* Replacing 1 byte can be done atomically. */
>>>>> +	if (unlikely(len <= 1))
>>>>> +		return text_poke(addr, opcode, len);
>>>>
>>>> This part bothers me. The text_poke just writes over the text
>>>> directly (using a separate mapping). But if that memory is in the
>>>> pipeline of another CPU, I think this could cause a GPF.
>>>>
>>>
>>> Could you clarify why you think that?
>>
>> Basically, what Steven and I were concerned about in this particular
>> patch version is the fact that this code took a "shortcut" for
>> single-byte text modification, thus bypassing the int3-bypass scheme
>> altogether.
> 
> single byte instruction updates are likely 100x safer than any scheme
> of multi-byte instruction scheme that I have seen, other than a full
> stop_machine().
> 
> That does not mean it is safe, it just means it's an order of
> complexity less to analyze ;-)

Yeah, so in the latest patch, I updated it to use int3 even if
len == 1. :-)

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Arjan van de Ven]

On Sun, 17 Jan 2010 13:55:39 -0500
Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca> wrote:

> * H. Peter Anvin (hpa@zytor.com) wrote:
> > On 01/14/2010 07:32 AM, Steven Rostedt wrote:
> > >> +
> > >> +	/* Replacing 1 byte can be done atomically. */
> > >> +	if (unlikely(len <= 1))
> > >> +		return text_poke(addr, opcode, len);
> > > 
> > > This part bothers me. The text_poke just writes over the text
> > > directly (using a separate mapping). But if that memory is in the
> > > pipeline of another CPU, I think this could cause a GPF.
> > > 
> > 
> > Could you clarify why you think that?
> 
> Basically, what Steven and I were concerned about in this particular
> patch version is the fact that this code took a "shortcut" for
> single-byte text modification, thus bypassing the int3-bypass scheme
> altogether.

single byte instruction updates are likely 100x safer than any scheme
of multi-byte instruction scheme that I have seen, other than a full
stop_machine().

That does not mean it is safe, it just means it's an order of
complexity less to analyze ;-)


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Mathieu Desnoyers]

* H. Peter Anvin (hpa@zytor.com) wrote:
> On 01/14/2010 07:32 AM, Steven Rostedt wrote:
> >> +
> >> +	/* Replacing 1 byte can be done atomically. */
> >> +	if (unlikely(len <= 1))
> >> +		return text_poke(addr, opcode, len);
> > 
> > This part bothers me. The text_poke just writes over the text directly
> > (using a separate mapping). But if that memory is in the pipeline of
> > another CPU, I think this could cause a GPF.
> > 
> 
> Could you clarify why you think that?

Basically, what Steven and I were concerned about in this particular
patch version is the fact that this code took a "shortcut" for
single-byte text modification, thus bypassing the int3-bypass scheme
altogether.

As mere atomicity of the modification is not the only concern here
(because we also have to deal with instruction trace cache coherency and
so forth), then the int3 breakpoint scheme is, I think, also needed for
single-byte updates.

Thanks,

Mathieu

> 
> 	-hpa
> 
> -- 
> H. Peter Anvin, Intel Open Source Technology Center
> I work for Intel.  I don't speak on their behalf.
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Masami Hiramatsu]

Mathieu Desnoyers wrote:
>> It is *not* necessary to wait for the breakpoint handlers to return, as
>> long as they will get to IRET eventually, since IRET is a jump and a
>> serializing instruction.
> 
> Ah, I see. So the added smp_mb() would not be needed then, as long as we
> know that the other CPUs either are currently running the IPI handler or
> have executed it. IOW: they will execute IRET very soon or they just
> executed it since the int3 have been written. I am a bit concerned about
> NMIs coming in this race window, but as they need to have started after
> we have put the breakpoint, that should be OK. (note: entry_*.S
> modifications are needed to support nesting breakpoint handlers in NMIs)

Hmm, if we support this to modify NMI code, it seems that we need to
support not only nesting breakpoint handling but also nesting NMIs,
because nesting NMI is unblocked when next IRET (of breakpoint) is
issued.

>From Intel's Software Developer’s Manual Vol.3A 5.7.1 Handling Multiple NMIs
said below.
---
While an NMI interrupt handler is executing, the processor disables additional calls to
the NMI handler until the next IRET instruction is executed. This blocking of subse-
quent NMIs prevents stacking up calls to the NMI handler. [...]
---

I assume that your below patch tried to solve this issue, right?
http://lkml.indiana.edu/hypermail/linux/kernel/0804.1/0965.html

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Jason Baron]

On Thu, Jan 14, 2010 at 11:23:35AM -0500, Masami Hiramatsu wrote:
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> >> On Tue, 2010-01-12 at 11:26 -0500, Jason Baron wrote:
> >>
> >>> +/**
> >>> + * text_poke_fixup() -- cross-modifying kernel text with fixup address.
> >>> + * @addr:	Modifying address.
> >>> + * @opcode:	New instruction.
> >>> + * @len:	length of modifying bytes.
> >>> + * @fixup:	Fixup address.
> >>> + *
> >>> + * Note: You must backup replaced instructions before calling this,
> >>> + * if you need to recover it.
> >>> + * Note: Must be called under text_mutex.
> >>> + */
> >>> +void *__kprobes text_poke_fixup(void *addr, const void *opcode, size_t len,
> >>> +				void *fixup)
> >>> +{
> >>> +	static const unsigned char int3_insn = BREAKPOINT_INSTRUCTION;
> >>> +	static const int int3_size = sizeof(int3_insn);
> >>> +
> >>> +	/* Replacing 1 byte can be done atomically. */
> >>> +	if (unlikely(len <= 1))
> >>> +		return text_poke(addr, opcode, len);
> >>
> >> This part bothers me. The text_poke just writes over the text directly
> >> (using a separate mapping). But if that memory is in the pipeline of
> >> another CPU, I think this could cause a GPF.
> > 
> > It looks like we are thinking along the same lines.
> > 
> > I'm under the impression that I pointed out this exact same issue in the
> > previous round of review a few weeks ago. Does this submission reflect
> > the up-to-date state of this patch ?
> 
> No, the latest patch just skips step 3 if len == 1.
> (Jason, could you update your repository?)
> I thought I sent it the end of the last year ... :)
> 
> http://lkml.org/lkml/2009/12/18/312
> 
> Thank you,
> 

sorry about that...i've updated to the latest.

thanks,

-Jason

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Masami Hiramatsu]

Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
>> On Tue, 2010-01-12 at 11:26 -0500, Jason Baron wrote:
>>
>>> +/**
>>> + * text_poke_fixup() -- cross-modifying kernel text with fixup address.
>>> + * @addr:	Modifying address.
>>> + * @opcode:	New instruction.
>>> + * @len:	length of modifying bytes.
>>> + * @fixup:	Fixup address.
>>> + *
>>> + * Note: You must backup replaced instructions before calling this,
>>> + * if you need to recover it.
>>> + * Note: Must be called under text_mutex.
>>> + */
>>> +void *__kprobes text_poke_fixup(void *addr, const void *opcode, size_t len,
>>> +				void *fixup)
>>> +{
>>> +	static const unsigned char int3_insn = BREAKPOINT_INSTRUCTION;
>>> +	static const int int3_size = sizeof(int3_insn);
>>> +
>>> +	/* Replacing 1 byte can be done atomically. */
>>> +	if (unlikely(len <= 1))
>>> +		return text_poke(addr, opcode, len);
>>
>> This part bothers me. The text_poke just writes over the text directly
>> (using a separate mapping). But if that memory is in the pipeline of
>> another CPU, I think this could cause a GPF.
> 
> It looks like we are thinking along the same lines.
> 
> I'm under the impression that I pointed out this exact same issue in the
> previous round of review a few weeks ago. Does this submission reflect
> the up-to-date state of this patch ?

No, the latest patch just skips step 3 if len == 1.
(Jason, could you update your repository?)
I thought I sent it the end of the last year ... :)

http://lkml.org/lkml/2009/12/18/312

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Tue, 2010-01-12 at 11:26 -0500, Jason Baron wrote:
> 
> > +/**
> > + * text_poke_fixup() -- cross-modifying kernel text with fixup address.
> > + * @addr:	Modifying address.
> > + * @opcode:	New instruction.
> > + * @len:	length of modifying bytes.
> > + * @fixup:	Fixup address.
> > + *
> > + * Note: You must backup replaced instructions before calling this,
> > + * if you need to recover it.
> > + * Note: Must be called under text_mutex.
> > + */
> > +void *__kprobes text_poke_fixup(void *addr, const void *opcode, size_t len,
> > +				void *fixup)
> > +{
> > +	static const unsigned char int3_insn = BREAKPOINT_INSTRUCTION;
> > +	static const int int3_size = sizeof(int3_insn);
> > +
> > +	/* Replacing 1 byte can be done atomically. */
> > +	if (unlikely(len <= 1))
> > +		return text_poke(addr, opcode, len);
> 
> This part bothers me. The text_poke just writes over the text directly
> (using a separate mapping). But if that memory is in the pipeline of
> another CPU, I think this could cause a GPF.

It looks like we are thinking along the same lines.

I'm under the impression that I pointed out this exact same issue in the
previous round of review a few weeks ago. Does this submission reflect
the up-to-date state of this patch ?

Thanks,

Mathieu

> 
> -- Steve
> 
> > +
> > +	/* Preparing */
> > +	patch_fixup_addr = fixup;
> > +	wmb();
> > +	patch_fixup_from = (u8 *)addr + int3_size; /* IP address after int3 */
> > +
> > +	/* Cap by an int3 */
> > +	text_poke(addr, &int3_insn, int3_size);
> > +	sync_core_all();
> > +
> > +	/* Replace tail bytes */
> > +	text_poke((char *)addr + int3_size, (const char *)opcode + int3_size,
> > +		  len - int3_size);
> > +	sync_core_all();
> > +
> > +	/* Replace int3 with head byte */
> > +	text_poke(addr, opcode, int3_size);
> > +	sync_core_all();
> > +
> > +	/* Cleanup */
> > +	patch_fixup_from = NULL;
> > +	wmb();
> > +	return addr;
> > +}
> > +
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[H. Peter Anvin]

On 01/14/2010 07:32 AM, Steven Rostedt wrote:
>> +
>> +	/* Replacing 1 byte can be done atomically. */
>> +	if (unlikely(len <= 1))
>> +		return text_poke(addr, opcode, len);
> 
> This part bothers me. The text_poke just writes over the text directly
> (using a separate mapping). But if that memory is in the pipeline of
> another CPU, I think this could cause a GPF.
> 

Could you clarify why you think that?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Steven Rostedt]

On Tue, 2010-01-12 at 11:26 -0500, Jason Baron wrote:

> +/**
> + * text_poke_fixup() -- cross-modifying kernel text with fixup address.
> + * @addr:	Modifying address.
> + * @opcode:	New instruction.
> + * @len:	length of modifying bytes.
> + * @fixup:	Fixup address.
> + *
> + * Note: You must backup replaced instructions before calling this,
> + * if you need to recover it.
> + * Note: Must be called under text_mutex.
> + */
> +void *__kprobes text_poke_fixup(void *addr, const void *opcode, size_t len,
> +				void *fixup)
> +{
> +	static const unsigned char int3_insn = BREAKPOINT_INSTRUCTION;
> +	static const int int3_size = sizeof(int3_insn);
> +
> +	/* Replacing 1 byte can be done atomically. */
> +	if (unlikely(len <= 1))
> +		return text_poke(addr, opcode, len);

This part bothers me. The text_poke just writes over the text directly
(using a separate mapping). But if that memory is in the pipeline of
another CPU, I think this could cause a GPF.

-- Steve

> +
> +	/* Preparing */
> +	patch_fixup_addr = fixup;
> +	wmb();
> +	patch_fixup_from = (u8 *)addr + int3_size; /* IP address after int3 */
> +
> +	/* Cap by an int3 */
> +	text_poke(addr, &int3_insn, int3_size);
> +	sync_core_all();
> +
> +	/* Replace tail bytes */
> +	text_poke((char *)addr + int3_size, (const char *)opcode + int3_size,
> +		  len - int3_size);
> +	sync_core_all();
> +
> +	/* Replace int3 with head byte */
> +	text_poke(addr, opcode, int3_size);
> +	sync_core_all();
> +
> +	/* Cleanup */
> +	patch_fixup_from = NULL;
> +	wmb();
> +	return addr;
> +}
> +

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Masami Hiramatsu]

Mathieu Desnoyers wrote:
> * H. Peter Anvin (hpa@zytor.com) wrote:
>> On 01/12/2010 06:06 PM, Mathieu Desnoyers wrote:
>>> * H. Peter Anvin (hpa@zytor.com) wrote:
>>>> On 01/12/2010 08:26 AM, Jason Baron wrote:
>>>>> Add text_poke_fixup() which takes a fixup address to where a processor
>>>>> jumps if it hits the modifying address while code modifying.
>>>>> text_poke_fixup() does following steps for this purpose.
>>>>>
>>>>>  1. Setup int3 handler for fixup.
>>>>>  2. Put a breakpoint (int3) on the first byte of modifying region,
>>>>>     and synchronize code on all CPUs.
>>>>>  3. Modify other bytes of modifying region, and synchronize code on all CPUs.
>>>>>  4. Modify the first byte of modifying region, and synchronize code
>>>>>     on all CPUs.
>>>>>  5. Clear int3 handler.
>>>>>
>>>>
>>>> We (Intel OTC) have been able to get an *unofficial* answer as to the
>>>> validity of this procedure; specifically as it applies to Intel hardware
>>>> (obviously).  We are working on getting an officially approved answer,
>>>> but as far as we currently know, the procedure as outlined above should
>>>> work on all Intel hardware.  In fact, we believe the synchronization in
>>>> step 3 is in fact unnecessary (as the synchronization in step 4 provides
>>>> sufficient guard.)
>>>
>>> Hi Peter,
>>>
>>> This is great news! Thanks to Intel OTC and yourself for looking into
>>> this. In the immediate values patches, I am doing the synchronization at
>>> the end of step (3) to ensure that all remote CPUs issue read memory
>>> barriers, so the stores to the instruction are done in this order:
>>>
>>> spin lock
>>> store int3 to 1st byte
>>> smp_wmb()
>>> sync all cores
>>> store new instruction in all but 1st byte
>>> smp_wmb()
>>> issue smp_rmb() on all cores (a sync all cores has this effect)
>>> store new instruction to 1st byte
>>> send IPI to all cores (or call synchronize_sched()) to wait for all
>>>   breakpoint handlers to complete.
>>> spin unlock
>>>
>>> So the question is: are these wmb/rmb pairs actually needed ?  As the
>>> instruction fetch is not performed by instructions per se, I doubt a
>>> rmb() will have any effect on them. I always prefer to stay on the safe
>>> side, but it wouldn't hurt to know.
>>>
>>
>> I don't think the smp_rmb() has any function.
> 
> OK, that's good to know.
> 
>>
>> However, you're being quite inconsistent in your terminology here.  The
>> assumption above is that the "synchronize code on all CPU" step is
>> sending an IPI to all cores and waiting for it to return, so that each
>> core has executed IPI/IRET before continuation.
> 
> To be strictly correct, we cannot assume that the IPI handler issues IRET
> before signaling its completion. It's rather the other way around.
> This is why I add a smp_mb() in the IPI handler for the "synchronize
> code on all CPUs" step.
> 
>>
>> It is *not* necessary to wait for the breakpoint handlers to return, as
>> long as they will get to IRET eventually, since IRET is a jump and a
>> serializing instruction.
> 
> Ah, I see. So the added smp_mb() would not be needed then, as long as we
> know that the other CPUs either are currently running the IPI handler or
> have executed it. IOW: they will execute IRET very soon or they just
> executed it since the int3 have been written. I am a bit concerned about
> NMIs coming in this race window, but as they need to have started after
> we have put the breakpoint, that should be OK. (note: entry_*.S
> modifications are needed to support nesting breakpoint handlers in NMIs)
> 
>>
>>> Hrm. Assuming we have a spinlock protecting all this, given that we
>>> synchronize all cores at step (4) _after_ removing the breakpoint, and
>>> given that the breakpoint handler is an interrupt gate (thus executes
>>> with interrupts off), I am inclined to think that sending the IPIs at
>>> the end of step (4) (and waiting for them to complete) should be enough
>>> to ensure that all in-flight breakpoint handlers for this site have
>>> completed their execution. This would mean that we only have to keep
>>> track of a single site at a time. Or am I missing something ?
>>
>> Yes: the whole point was that you can omit the synchronization in step 4
>> if you leave the breakpoint handler in place (I said "omit step 5", but
>> that wasn't really what I meant.)

Hmm, in that case, how can we reuse the breakpoint handler for another
text poke site? Even if we leave the handler, I think we need to clear
fixup information for next poking...

>>
>> That means that at the cost of two compares in the standard #BP handler,
>> we can get away with only one IPI per atomic instruction poke.
> 
> OK. That makes sense now.

So, let me check the actual replacement steps.

(1) lock text_mutex
(2) setup breakpoint fixup addresses (source and destination)
(3) store int3 to 1st byte, and smp_wmb()
(4) send IPI and issue smp_mb() (or cpuid) with for sync all cores.
(5) store new instruction except 1st byte, and smp_wmb()
(6) store 1st byte of new instruction
(7) send IPI to all cores for waiting for all running breakpoint handlers.
(8) clear fixup addresses
(9) unlock text_mutex

Is this correct?

Thank you,

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Mathieu Desnoyers]

* H. Peter Anvin (hpa@zytor.com) wrote:
> On 01/12/2010 06:06 PM, Mathieu Desnoyers wrote:
> > * H. Peter Anvin (hpa@zytor.com) wrote:
> >> On 01/12/2010 08:26 AM, Jason Baron wrote:
> >>> Add text_poke_fixup() which takes a fixup address to where a processor
> >>> jumps if it hits the modifying address while code modifying.
> >>> text_poke_fixup() does following steps for this purpose.
> >>>
> >>>  1. Setup int3 handler for fixup.
> >>>  2. Put a breakpoint (int3) on the first byte of modifying region,
> >>>     and synchronize code on all CPUs.
> >>>  3. Modify other bytes of modifying region, and synchronize code on all CPUs.
> >>>  4. Modify the first byte of modifying region, and synchronize code
> >>>     on all CPUs.
> >>>  5. Clear int3 handler.
> >>>
> >>
> >> We (Intel OTC) have been able to get an *unofficial* answer as to the
> >> validity of this procedure; specifically as it applies to Intel hardware
> >> (obviously).  We are working on getting an officially approved answer,
> >> but as far as we currently know, the procedure as outlined above should
> >> work on all Intel hardware.  In fact, we believe the synchronization in
> >> step 3 is in fact unnecessary (as the synchronization in step 4 provides
> >> sufficient guard.)
> > 
> > Hi Peter,
> > 
> > This is great news! Thanks to Intel OTC and yourself for looking into
> > this. In the immediate values patches, I am doing the synchronization at
> > the end of step (3) to ensure that all remote CPUs issue read memory
> > barriers, so the stores to the instruction are done in this order:
> > 
> > spin lock
> > store int3 to 1st byte
> > smp_wmb()
> > sync all cores
> > store new instruction in all but 1st byte
> > smp_wmb()
> > issue smp_rmb() on all cores (a sync all cores has this effect)
> > store new instruction to 1st byte
> > send IPI to all cores (or call synchronize_sched()) to wait for all
> >   breakpoint handlers to complete.
> > spin unlock
> > 
> > So the question is: are these wmb/rmb pairs actually needed ?  As the
> > instruction fetch is not performed by instructions per se, I doubt a
> > rmb() will have any effect on them. I always prefer to stay on the safe
> > side, but it wouldn't hurt to know.
> > 
> 
> I don't think the smp_rmb() has any function.

OK, that's good to know.

> 
> However, you're being quite inconsistent in your terminology here.  The
> assumption above is that the "synchronize code on all CPU" step is
> sending an IPI to all cores and waiting for it to return, so that each
> core has executed IPI/IRET before continuation.

To be strictly correct, we cannot assume that the IPI handler issues IRET
before signaling its completion. It's rather the other way around.
This is why I add a smp_mb() in the IPI handler for the "synchronize
code on all CPUs" step.

> 
> It is *not* necessary to wait for the breakpoint handlers to return, as
> long as they will get to IRET eventually, since IRET is a jump and a
> serializing instruction.

Ah, I see. So the added smp_mb() would not be needed then, as long as we
know that the other CPUs either are currently running the IPI handler or
have executed it. IOW: they will execute IRET very soon or they just
executed it since the int3 have been written. I am a bit concerned about
NMIs coming in this race window, but as they need to have started after
we have put the breakpoint, that should be OK. (note: entry_*.S
modifications are needed to support nesting breakpoint handlers in NMIs)

> 
> > Hrm. Assuming we have a spinlock protecting all this, given that we
> > synchronize all cores at step (4) _after_ removing the breakpoint, and
> > given that the breakpoint handler is an interrupt gate (thus executes
> > with interrupts off), I am inclined to think that sending the IPIs at
> > the end of step (4) (and waiting for them to complete) should be enough
> > to ensure that all in-flight breakpoint handlers for this site have
> > completed their execution. This would mean that we only have to keep
> > track of a single site at a time. Or am I missing something ?
> 
> Yes: the whole point was that you can omit the synchronization in step 4
> if you leave the breakpoint handler in place (I said "omit step 5", but
> that wasn't really what I meant.)
> 
> That means that at the cost of two compares in the standard #BP handler,
> we can get away with only one IPI per atomic instruction poke.

OK. That makes sense now.

Thanks,

Mathieu

> 
> 	-hpa
> 
> 
> 
> -- 
> H. Peter Anvin, Intel Open Source Technology Center
> I work for Intel.  I don't speak on their behalf.
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Masami Hiramatsu]

H. Peter Anvin wrote:
> On 01/12/2010 08:26 AM, Jason Baron wrote:
>> Add text_poke_fixup() which takes a fixup address to where a processor
>> jumps if it hits the modifying address while code modifying.
>> text_poke_fixup() does following steps for this purpose.
>>
>>  1. Setup int3 handler for fixup.
>>  2. Put a breakpoint (int3) on the first byte of modifying region,
>>     and synchronize code on all CPUs.
>>  3. Modify other bytes of modifying region, and synchronize code on all CPUs.
>>  4. Modify the first byte of modifying region, and synchronize code
>>     on all CPUs.
>>  5. Clear int3 handler.
>>
> 
> We (Intel OTC) have been able to get an *unofficial* answer as to the
> validity of this procedure; specifically as it applies to Intel hardware
> (obviously).  We are working on getting an officially approved answer,
> but as far as we currently know, the procedure as outlined above should
> work on all Intel hardware.  In fact, we believe the synchronization in
> step 3 is in fact unnecessary (as the synchronization in step 4 provides
> sufficient guard.)

Good news! Thank you very much, Peter!

And actually, this patch is a bit older than I previously posted on LKML.

http://lkml.org/lkml/2009/12/18/312

Oops, I've forgotten update comment on patch... anyway, patch implementation
itself is updated and removed second sync_core_all.
I'll post it again with updated comment.

> In fact, if a suitable int3 handler is left permanently in place then
> step 5 is unnecessary as well.  This would slow down other uses of int3
> slightly, but might be a worthwhile tradeoff.

OK.

> Such a permanent int3 handler would need to keep track of two
> potentially-spurious breakpoints: the current and the previous.  The
> reason for needing two is that one could get a #BP from either the
> current or the previous modification site between the insertion of int3
> and the synchronization in step 2.  This, of course, assumes that the
> actual code poking is forcibly single-threaded (running under a spinlock
> or other mutex) -- if modifications are allowed to run in parallel you
> need to consider all possible current or stale #BP sites.

Sure, and since we are using fixmap for poking, we need to do this
under locking text_mutex.

Thank you!


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[H. Peter Anvin]

On 01/12/2010 06:06 PM, Mathieu Desnoyers wrote:
> * H. Peter Anvin (hpa@zytor.com) wrote:
>> On 01/12/2010 08:26 AM, Jason Baron wrote:
>>> Add text_poke_fixup() which takes a fixup address to where a processor
>>> jumps if it hits the modifying address while code modifying.
>>> text_poke_fixup() does following steps for this purpose.
>>>
>>>  1. Setup int3 handler for fixup.
>>>  2. Put a breakpoint (int3) on the first byte of modifying region,
>>>     and synchronize code on all CPUs.
>>>  3. Modify other bytes of modifying region, and synchronize code on all CPUs.
>>>  4. Modify the first byte of modifying region, and synchronize code
>>>     on all CPUs.
>>>  5. Clear int3 handler.
>>>
>>
>> We (Intel OTC) have been able to get an *unofficial* answer as to the
>> validity of this procedure; specifically as it applies to Intel hardware
>> (obviously).  We are working on getting an officially approved answer,
>> but as far as we currently know, the procedure as outlined above should
>> work on all Intel hardware.  In fact, we believe the synchronization in
>> step 3 is in fact unnecessary (as the synchronization in step 4 provides
>> sufficient guard.)
> 
> Hi Peter,
> 
> This is great news! Thanks to Intel OTC and yourself for looking into
> this. In the immediate values patches, I am doing the synchronization at
> the end of step (3) to ensure that all remote CPUs issue read memory
> barriers, so the stores to the instruction are done in this order:
> 
> spin lock
> store int3 to 1st byte
> smp_wmb()
> sync all cores
> store new instruction in all but 1st byte
> smp_wmb()
> issue smp_rmb() on all cores (a sync all cores has this effect)
> store new instruction to 1st byte
> send IPI to all cores (or call synchronize_sched()) to wait for all
>   breakpoint handlers to complete.
> spin unlock
> 
> So the question is: are these wmb/rmb pairs actually needed ?  As the
> instruction fetch is not performed by instructions per se, I doubt a
> rmb() will have any effect on them. I always prefer to stay on the safe
> side, but it wouldn't hurt to know.
> 

I don't think the smp_rmb() has any function.

However, you're being quite inconsistent in your terminology here.  The
assumption above is that the "synchronize code on all CPU" step is
sending an IPI to all cores and waiting for it to return, so that each
core has executed IPI/IRET before continuation.

It is *not* necessary to wait for the breakpoint handlers to return, as
long as they will get to IRET eventually, since IRET is a jump and a
serializing instruction.

> Hrm. Assuming we have a spinlock protecting all this, given that we
> synchronize all cores at step (4) _after_ removing the breakpoint, and
> given that the breakpoint handler is an interrupt gate (thus executes
> with interrupts off), I am inclined to think that sending the IPIs at
> the end of step (4) (and waiting for them to complete) should be enough
> to ensure that all in-flight breakpoint handlers for this site have
> completed their execution. This would mean that we only have to keep
> track of a single site at a time. Or am I missing something ?

Yes: the whole point was that you can omit the synchronization in step 4
if you leave the breakpoint handler in place (I said "omit step 5", but
that wasn't really what I meant.)

That means that at the cost of two compares in the standard #BP handler,
we can get away with only one IPI per atomic instruction poke.

	-hpa



-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Mathieu Desnoyers]

* H. Peter Anvin (hpa@zytor.com) wrote:
> On 01/12/2010 08:26 AM, Jason Baron wrote:
> > Add text_poke_fixup() which takes a fixup address to where a processor
> > jumps if it hits the modifying address while code modifying.
> > text_poke_fixup() does following steps for this purpose.
> > 
> >  1. Setup int3 handler for fixup.
> >  2. Put a breakpoint (int3) on the first byte of modifying region,
> >     and synchronize code on all CPUs.
> >  3. Modify other bytes of modifying region, and synchronize code on all CPUs.
> >  4. Modify the first byte of modifying region, and synchronize code
> >     on all CPUs.
> >  5. Clear int3 handler.
> > 
> 
> We (Intel OTC) have been able to get an *unofficial* answer as to the
> validity of this procedure; specifically as it applies to Intel hardware
> (obviously).  We are working on getting an officially approved answer,
> but as far as we currently know, the procedure as outlined above should
> work on all Intel hardware.  In fact, we believe the synchronization in
> step 3 is in fact unnecessary (as the synchronization in step 4 provides
> sufficient guard.)

Hi Peter,

This is great news! Thanks to Intel OTC and yourself for looking into
this. In the immediate values patches, I am doing the synchronization at
the end of step (3) to ensure that all remote CPUs issue read memory
barriers, so the stores to the instruction are done in this order:

spin lock
store int3 to 1st byte
smp_wmb()
sync all cores
store new instruction in all but 1st byte
smp_wmb()
issue smp_rmb() on all cores (a sync all cores has this effect)
store new instruction to 1st byte
send IPI to all cores (or call synchronize_sched()) to wait for all
  breakpoint handlers to complete.
spin unlock

So the question is: are these wmb/rmb pairs actually needed ?  As the
instruction fetch is not performed by instructions per se, I doubt a
rmb() will have any effect on them. I always prefer to stay on the safe
side, but it wouldn't hurt to know.

> 
> In fact, if a suitable int3 handler is left permanently in place then
> step 5 is unnecessary as well.  This would slow down other uses of int3
> slightly, but might be a worthwhile tradeoff.
> 
> Such a permanent int3 handler would need to keep track of two
> potentially-spurious breakpoints: the current and the previous.  The
> reason for needing two is that one could get a #BP from either the
> current or the previous modification site between the insertion of int3
> and the synchronization in step 2.  This, of course, assumes that the
> actual code poking is forcibly single-threaded (running under a spinlock
> or other mutex) -- if modifications are allowed to run in parallel you
> need to consider all possible current or stale #BP sites.

Hrm. Assuming we have a spinlock protecting all this, given that we
synchronize all cores at step (4) _after_ removing the breakpoint, and
given that the breakpoint handler is an interrupt gate (thus executes
with interrupts off), I am inclined to think that sending the IPIs at
the end of step (4) (and waiting for them to complete) should be enough
to ensure that all in-flight breakpoint handlers for this site have
completed their execution. This would mean that we only have to keep
track of a single site at a time. Or am I missing something ?

Thanks,

Mathieu

> 
> 	-hpa

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: [RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[H. Peter Anvin]

On 01/12/2010 08:26 AM, Jason Baron wrote:
> Add text_poke_fixup() which takes a fixup address to where a processor
> jumps if it hits the modifying address while code modifying.
> text_poke_fixup() does following steps for this purpose.
> 
>  1. Setup int3 handler for fixup.
>  2. Put a breakpoint (int3) on the first byte of modifying region,
>     and synchronize code on all CPUs.
>  3. Modify other bytes of modifying region, and synchronize code on all CPUs.
>  4. Modify the first byte of modifying region, and synchronize code
>     on all CPUs.
>  5. Clear int3 handler.
> 

We (Intel OTC) have been able to get an *unofficial* answer as to the
validity of this procedure; specifically as it applies to Intel hardware
(obviously).  We are working on getting an officially approved answer,
but as far as we currently know, the procedure as outlined above should
work on all Intel hardware.  In fact, we believe the synchronization in
step 3 is in fact unnecessary (as the synchronization in step 4 provides
sufficient guard.)

In fact, if a suitable int3 handler is left permanently in place then
step 5 is unnecessary as well.  This would slow down other uses of int3
slightly, but might be a worthwhile tradeoff.

Such a permanent int3 handler would need to keep track of two
potentially-spurious breakpoints: the current and the previous.  The
reason for needing two is that one could get a #BP from either the
current or the previous modification site between the insertion of int3
and the synchronization in step 2.  This, of course, assumes that the
actual code poking is forcibly single-threaded (running under a spinlock
or other mutex) -- if modifications are allowed to run in parallel you
need to consider all possible current or stale #BP sites.

	-hpa

[RFC PATCH 2/8] jump label v4 - x86: Introduce generic jump patching without stop_machine

[Jason Baron]

Add text_poke_fixup() which takes a fixup address to where a processor
jumps if it hits the modifying address while code modifying.
text_poke_fixup() does following steps for this purpose.

 1. Setup int3 handler for fixup.
 2. Put a breakpoint (int3) on the first byte of modifying region,
    and synchronize code on all CPUs.
 3. Modify other bytes of modifying region, and synchronize code on all CPUs.
 4. Modify the first byte of modifying region, and synchronize code
    on all CPUs.
 5. Clear int3 handler.

Thus, if some other processor execute modifying address when step2 to step4,
it will be jumped to fixup code.

This still has many limitations for modifying multi-instructions at once.
However, it is enough for 'a 5 bytes nop replacing with a jump' patching,
because;
 - Replaced instruction is just one instruction, which is executed atomically.
 - Replacing instruction is a jump, so we can set fixup address where the jump
   goes to.

Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
---
 arch/x86/include/asm/alternative.h |   12 ++++
 arch/x86/include/asm/kprobes.h     |    1 +
 arch/x86/kernel/alternative.c      |  120 ++++++++++++++++++++++++++++++++++++
 kernel/kprobes.c                   |    2 +-
 4 files changed, 134 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
index 3b5b828..9b856b7 100644
--- a/arch/x86/include/asm/alternative.h
+++ b/arch/x86/include/asm/alternative.h
@@ -166,4 +166,16 @@ static inline void apply_paravirt(struct paravirt_patch_site *start,
  */
 extern void *text_poke(void *addr, const void *opcode, size_t len);
 
+/*
+ * Setup int3 trap and fixup execution for cross-modifying on SMP case.
+ * If the other cpus execute modifying instruction, it will hit int3
+ * and go to fixup code. This just provides a minimal safety check.
+ * Additional checks/restrictions are required for completely safe
+ * cross-modifying.
+ */
+extern void *text_poke_fixup(void *addr, const void *opcode, size_t len,
+			     void *fixup);
+extern int text_patch_jump(void *addr, void *dest);
+extern void sync_core_all(void);
+
 #endif /* _ASM_X86_ALTERNATIVE_H */
diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
index eaec8ea..febab97 100644
--- a/arch/x86/include/asm/kprobes.h
+++ b/arch/x86/include/asm/kprobes.h
@@ -33,6 +33,7 @@ struct kprobe;
 typedef u8 kprobe_opcode_t;
 #define BREAKPOINT_INSTRUCTION	0xcc
 #define RELATIVEJUMP_OPCODE 0xe9
+#define RELATIVEJUMP_SIZE 5
 #define MAX_INSN_SIZE 16
 #define MAX_STACK_SIZE 64
 #define MIN_STACK_SIZE(ADDR)					       \
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 2589ea4..cf42144 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -4,6 +4,7 @@
 #include <linux/list.h>
 #include <linux/stringify.h>
 #include <linux/kprobes.h>
+#include <linux/kdebug.h>
 #include <linux/mm.h>
 #include <linux/vmalloc.h>
 #include <linux/memory.h>
@@ -554,3 +555,122 @@ void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
 	local_irq_restore(flags);
 	return addr;
 }
+
+/*
+ * On pentium series, Unsynchronized cross-modifying code
+ * operations can cause unexpected instruction execution results.
+ * So after code modified, we should synchronize it on each processor.
+ */
+static void __kprobes __local_sync_core(void *info)
+{
+	sync_core();
+}
+
+void __kprobes sync_core_all(void)
+{
+	on_each_cpu(__local_sync_core, NULL, 1);
+}
+
+/* Safely cross-code modifying with fixup address */
+static void *patch_fixup_from;
+static void *patch_fixup_addr;
+
+static int __kprobes patch_exceptions_notify(struct notifier_block *self,
+					      unsigned long val, void *data)
+{
+	struct die_args *args = data;
+	struct pt_regs *regs = args->regs;
+
+	if (likely(!patch_fixup_from))
+		return NOTIFY_DONE;
+
+	if (val != DIE_INT3 || !regs || user_mode_vm(regs) ||
+	    (unsigned long)patch_fixup_from != regs->ip)
+		return NOTIFY_DONE;
+
+	args->regs->ip = (unsigned long)patch_fixup_addr;
+	return NOTIFY_STOP;
+}
+
+/**
+ * text_poke_fixup() -- cross-modifying kernel text with fixup address.
+ * @addr:	Modifying address.
+ * @opcode:	New instruction.
+ * @len:	length of modifying bytes.
+ * @fixup:	Fixup address.
+ *
+ * Note: You must backup replaced instructions before calling this,
+ * if you need to recover it.
+ * Note: Must be called under text_mutex.
+ */
+void *__kprobes text_poke_fixup(void *addr, const void *opcode, size_t len,
+				void *fixup)
+{
+	static const unsigned char int3_insn = BREAKPOINT_INSTRUCTION;
+	static const int int3_size = sizeof(int3_insn);
+
+	/* Replacing 1 byte can be done atomically. */
+	if (unlikely(len <= 1))
+		return text_poke(addr, opcode, len);
+
+	/* Preparing */
+	patch_fixup_addr = fixup;
+	wmb();
+	patch_fixup_from = (u8 *)addr + int3_size; /* IP address after int3 */
+
+	/* Cap by an int3 */
+	text_poke(addr, &int3_insn, int3_size);
+	sync_core_all();
+
+	/* Replace tail bytes */
+	text_poke((char *)addr + int3_size, (const char *)opcode + int3_size,
+		  len - int3_size);
+	sync_core_all();
+
+	/* Replace int3 with head byte */
+	text_poke(addr, opcode, int3_size);
+	sync_core_all();
+
+	/* Cleanup */
+	patch_fixup_from = NULL;
+	wmb();
+	return addr;
+}
+
+/**
+ * text_patch_jump() -- cross-modifying kernel text with a relative jump
+ * @addr:	Address where jump from.
+ * @dest:	Address where jump to.
+ *
+ * Return 0 if succeeded to embed a jump. Otherwise, there is an error.
+ * Note: You must backup replaced instructions before calling this,
+ * if you need to recover it.
+ * Note: Must be called under text_mutex.
+ */
+int __kprobes text_patch_jump(void *addr, void *dest)
+{
+	unsigned char jmp_code[RELATIVEJUMP_SIZE];
+	s32 rel = (s32)((long)dest - ((long)addr + RELATIVEJUMP_SIZE));
+
+	if (!addr || !dest ||
+	    (long)rel != ((long)dest - ((long)addr + RELATIVEJUMP_SIZE)))
+		return -EINVAL;
+
+	jmp_code[0] = RELATIVEJUMP_OPCODE;
+	*(s32 *)(&jmp_code[1]) = rel;
+
+	text_poke_fixup(addr, jmp_code, RELATIVEJUMP_SIZE, dest);
+	return 0;
+}
+
+static struct notifier_block patch_exceptions_nb = {
+	.notifier_call = patch_exceptions_notify,
+	.priority = 0x7fffffff /* we need to be notified first */
+};
+
+static int __init patch_init(void)
+{
+	return register_die_notifier(&patch_exceptions_nb);
+}
+
+arch_initcall(patch_init);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index b7df302..9c4697f 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -898,7 +898,7 @@ EXPORT_SYMBOL_GPL(unregister_kprobes);
 
 static struct notifier_block kprobe_exceptions_nb = {
 	.notifier_call = kprobe_exceptions_notify,
-	.priority = 0x7fffffff /* we need to be notified first */
+	.priority = 0x7ffffff0 /* High priority, but not first.  */
 };
 
 unsigned long __weak arch_deref_entry_point(void *entry)
-- 
1.6.5.1