Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
On 8/21/07, Andi Kleen <[EMAIL PROTECTED]> wrote: > On Tue, Aug 21, 2007 at 04:30:10AM -0300, Glauber de Oliveira Costa wrote: > > On 8/20/07, Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote: > > > Chris Wright wrote: > > > > That did get backed out (at least the part that broke paravirt patching) > > > > in 602033ed5907a59ce86f709082a35be047743a86. Linus' tree should be > > > > working fine right now with d34fda4a84c18402640a1a2342d6e6d9829e6db7 > > > > committed, and can be further refined with the patch below that's just > > > > waiting on some further testing. > > > > > > > > > > I don't think this is necessary. It isn't worth complicating the > > > interface to avoid the memcpy. > > > > > > J > > Damn, > > > > I can't believe I've just lost a night tracking the issue, without > > seeing the discussion here ;-) > > I came out to this very same conclusion, and was about to send a patch > > that fixes it, by doing a memcpy before starting the instruction > > replacement. > > > > (I wouldn't say anything, as this is solved, but my night have to get > > some value, after all! ;-) > > > > So I'm with Jeremy. We don't lose too much by putting a memcpy there, > > this code is not exactly critical. It also seems cleaner, and less > > error prone. I have a patch ready here, but I think by this time, you > > guys have too ;-) > > x86-64 also has a __inline_memcpy that is guaranteed inlined. It was > originally for such cases when memcpy didn't work. Could be added to i386 > too if there is need > Andi, this is not the case that memcpy did not work. (I got the same issue for paravirt_ops x86_64, and btw, that's why I have not yet sent a new patch). The thing is that if that the paravirt replacement function returns, it will return with the temporary buffer empty, will write the buffer with text_poke(), and thus, put crap into the code. So adding a memcpy to the buffer guarantees that in the worst case, it will contain the original instruction. The normal memcpy works neatly. -- Glauber de Oliveira Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
On Tue, Aug 21, 2007 at 04:30:10AM -0300, Glauber de Oliveira Costa wrote: > On 8/20/07, Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote: > > Chris Wright wrote: > > > That did get backed out (at least the part that broke paravirt patching) > > > in 602033ed5907a59ce86f709082a35be047743a86. Linus' tree should be > > > working fine right now with d34fda4a84c18402640a1a2342d6e6d9829e6db7 > > > committed, and can be further refined with the patch below that's just > > > waiting on some further testing. > > > > > > > I don't think this is necessary. It isn't worth complicating the > > interface to avoid the memcpy. > > > > J > Damn, > > I can't believe I've just lost a night tracking the issue, without > seeing the discussion here ;-) > I came out to this very same conclusion, and was about to send a patch > that fixes it, by doing a memcpy before starting the instruction > replacement. > > (I wouldn't say anything, as this is solved, but my night have to get > some value, after all! ;-) > > So I'm with Jeremy. We don't lose too much by putting a memcpy there, > this code is not exactly critical. It also seems cleaner, and less > error prone. I have a patch ready here, but I think by this time, you > guys have too ;-) x86-64 also has a __inline_memcpy that is guaranteed inlined. It was originally for such cases when memcpy didn't work. Could be added to i386 too if there is need -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
On 8/20/07, Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote: > Chris Wright wrote: > > That did get backed out (at least the part that broke paravirt patching) > > in 602033ed5907a59ce86f709082a35be047743a86. Linus' tree should be > > working fine right now with d34fda4a84c18402640a1a2342d6e6d9829e6db7 > > committed, and can be further refined with the patch below that's just > > waiting on some further testing. > > > > I don't think this is necessary. It isn't worth complicating the > interface to avoid the memcpy. > > J Damn, I can't believe I've just lost a night tracking the issue, without seeing the discussion here ;-) I came out to this very same conclusion, and was about to send a patch that fixes it, by doing a memcpy before starting the instruction replacement. (I wouldn't say anything, as this is solved, but my night have to get some value, after all! ;-) So I'm with Jeremy. We don't lose too much by putting a memcpy there, this code is not exactly critical. It also seems cleaner, and less error prone. I have a patch ready here, but I think by this time, you guys have too ;-) -- Glauber de Oliveira Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
On 8/20/07, Jeremy Fitzhardinge [EMAIL PROTECTED] wrote: Chris Wright wrote: That did get backed out (at least the part that broke paravirt patching) in 602033ed5907a59ce86f709082a35be047743a86. Linus' tree should be working fine right now with d34fda4a84c18402640a1a2342d6e6d9829e6db7 committed, and can be further refined with the patch below that's just waiting on some further testing. I don't think this is necessary. It isn't worth complicating the interface to avoid the memcpy. J Damn, I can't believe I've just lost a night tracking the issue, without seeing the discussion here ;-) I came out to this very same conclusion, and was about to send a patch that fixes it, by doing a memcpy before starting the instruction replacement. (I wouldn't say anything, as this is solved, but my night have to get some value, after all! ;-) So I'm with Jeremy. We don't lose too much by putting a memcpy there, this code is not exactly critical. It also seems cleaner, and less error prone. I have a patch ready here, but I think by this time, you guys have too ;-) -- Glauber de Oliveira Costa. Free as in Freedom http://glommer.net The less confident you are, the more serious you have to act. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
On Tue, Aug 21, 2007 at 04:30:10AM -0300, Glauber de Oliveira Costa wrote: On 8/20/07, Jeremy Fitzhardinge [EMAIL PROTECTED] wrote: Chris Wright wrote: That did get backed out (at least the part that broke paravirt patching) in 602033ed5907a59ce86f709082a35be047743a86. Linus' tree should be working fine right now with d34fda4a84c18402640a1a2342d6e6d9829e6db7 committed, and can be further refined with the patch below that's just waiting on some further testing. I don't think this is necessary. It isn't worth complicating the interface to avoid the memcpy. J Damn, I can't believe I've just lost a night tracking the issue, without seeing the discussion here ;-) I came out to this very same conclusion, and was about to send a patch that fixes it, by doing a memcpy before starting the instruction replacement. (I wouldn't say anything, as this is solved, but my night have to get some value, after all! ;-) So I'm with Jeremy. We don't lose too much by putting a memcpy there, this code is not exactly critical. It also seems cleaner, and less error prone. I have a patch ready here, but I think by this time, you guys have too ;-) x86-64 also has a __inline_memcpy that is guaranteed inlined. It was originally for such cases when memcpy didn't work. Could be added to i386 too if there is need -Andi - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
On 8/21/07, Andi Kleen [EMAIL PROTECTED] wrote: On Tue, Aug 21, 2007 at 04:30:10AM -0300, Glauber de Oliveira Costa wrote: On 8/20/07, Jeremy Fitzhardinge [EMAIL PROTECTED] wrote: Chris Wright wrote: That did get backed out (at least the part that broke paravirt patching) in 602033ed5907a59ce86f709082a35be047743a86. Linus' tree should be working fine right now with d34fda4a84c18402640a1a2342d6e6d9829e6db7 committed, and can be further refined with the patch below that's just waiting on some further testing. I don't think this is necessary. It isn't worth complicating the interface to avoid the memcpy. J Damn, I can't believe I've just lost a night tracking the issue, without seeing the discussion here ;-) I came out to this very same conclusion, and was about to send a patch that fixes it, by doing a memcpy before starting the instruction replacement. (I wouldn't say anything, as this is solved, but my night have to get some value, after all! ;-) So I'm with Jeremy. We don't lose too much by putting a memcpy there, this code is not exactly critical. It also seems cleaner, and less error prone. I have a patch ready here, but I think by this time, you guys have too ;-) x86-64 also has a __inline_memcpy that is guaranteed inlined. It was originally for such cases when memcpy didn't work. Could be added to i386 too if there is need Andi, this is not the case that memcpy did not work. (I got the same issue for paravirt_ops x86_64, and btw, that's why I have not yet sent a new patch). The thing is that if that the paravirt replacement function returns, it will return with the temporary buffer empty, will write the buffer with text_poke(), and thus, put crap into the code. So adding a memcpy to the buffer guarantees that in the worst case, it will contain the original instruction. The normal memcpy works neatly. -- Glauber de Oliveira Costa. Free as in Freedom http://glommer.net The less confident you are, the more serious you have to act. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
Chris Wright wrote: > That did get backed out (at least the part that broke paravirt patching) > in 602033ed5907a59ce86f709082a35be047743a86. Linus' tree should be > working fine right now with d34fda4a84c18402640a1a2342d6e6d9829e6db7 > committed, and can be further refined with the patch below that's just > waiting on some further testing. > I don't think this is necessary. It isn't worth complicating the interface to avoid the memcpy. J - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
Chris Wright wrote: That did get backed out (at least the part that broke paravirt patching) in 602033ed5907a59ce86f709082a35be047743a86. Linus' tree should be working fine right now with d34fda4a84c18402640a1a2342d6e6d9829e6db7 committed, and can be further refined with the patch below that's just waiting on some further testing. I don't think this is necessary. It isn't worth complicating the interface to avoid the memcpy. J - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
* Rusty Russell ([EMAIL PROTECTED]) wrote: > Then back out 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177 too, which broke > lguest booting, and this tried to fix. That did get backed out (at least the part that broke paravirt patching) in 602033ed5907a59ce86f709082a35be047743a86. Linus' tree should be working fine right now with d34fda4a84c18402640a1a2342d6e6d9829e6db7 committed, and can be further refined with the patch below that's just waiting on some further testing. thanks, -chris -- Subject: [PATCH] x86: skip paravirt patching when appropriate From: Chris Wright <[EMAIL PROTECTED]> commit d34fda4a84c18402640a1a2342d6e6d9829e6db7 was a little overkill in the case where a paravirt patcher chooses to leave patch site unpatched. Instead of copying original instructions to temp buffer then back to patch site, simply skip patching those sites altogether. Cc: Jeremy Fitzhardinge <[EMAIL PROTECTED]> Cc: Rusty Russell <[EMAIL PROTECTED]> Cc: Zach Amsden <[EMAIL PROTECTED]> Signed-off-by: Chris Wright <[EMAIL PROTECTED]> --- arch/i386/kernel/alternative.c |4 ++-- arch/i386/kernel/paravirt.c| 10 +- arch/i386/kernel/vmi.c |4 ++-- include/asm-i386/paravirt.h|3 +++ 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/arch/i386/kernel/alternative.c b/arch/i386/kernel/alternative.c index 9f4ac8b..b81d87e 100644 --- a/arch/i386/kernel/alternative.c +++ b/arch/i386/kernel/alternative.c @@ -366,10 +366,10 @@ void apply_paravirt(struct paravirt_patch_site *start, unsigned int used; BUG_ON(p->len > MAX_PATCH_LEN); - /* prep the buffer with the original instructions */ - memcpy(insnbuf, p->instr, p->len); used = paravirt_ops.patch(p->instrtype, p->clobbers, insnbuf, (unsigned long)p->instr, p->len); + if (used == PV_NO_PATCH) + continue; BUG_ON(used > p->len); diff --git a/arch/i386/kernel/paravirt.c b/arch/i386/kernel/paravirt.c index 739cfb2..a36ce34 100644 --- a/arch/i386/kernel/paravirt.c +++ b/arch/i386/kernel/paravirt.c @@ -122,7 +122,7 @@ unsigned paravirt_patch_nop(void) unsigned paravirt_patch_ignore(unsigned len) { - return len; + return PV_NO_PATCH; } struct branch { @@ -139,9 +139,9 @@ unsigned paravirt_patch_call(void *insnbuf, unsigned long delta = (unsigned long)target - (addr+5); if (tgt_clobbers & ~site_clobbers) - return len; /* target would clobber too much for this site */ + return PV_NO_PATCH; /* target would clobber too much for this site */ if (len < 5) - return len; /* call too long for patch site */ + return PV_NO_PATCH; /* call too long for patch site */ b->opcode = 0xe8; /* call */ b->delta = delta; @@ -157,7 +157,7 @@ unsigned paravirt_patch_jmp(const void *target, void *insnbuf, unsigned long delta = (unsigned long)target - (addr+5); if (len < 5) - return len; /* call too long for patch site */ + return PV_NO_PATCH; /* call too long for patch site */ b->opcode = 0xe9; /* jmp */ b->delta = delta; @@ -196,7 +196,7 @@ unsigned paravirt_patch_insns(void *insnbuf, unsigned len, unsigned insn_len = end - start; if (insn_len > len || start == NULL) - insn_len = len; + insn_len = PV_NO_PATCH; else memcpy(insnbuf, start, insn_len); diff --git a/arch/i386/kernel/vmi.c b/arch/i386/kernel/vmi.c index 18673e0..27ae004 100644 --- a/arch/i386/kernel/vmi.c +++ b/arch/i386/kernel/vmi.c @@ -118,7 +118,7 @@ static unsigned patch_internal(int call, unsigned len, void *insnbuf, case VMI_RELOCATION_NONE: /* leave native code in place */ - break; + return PV_NO_PATCH; default: BUG(); @@ -153,7 +153,7 @@ static unsigned vmi_patch(u8 type, u16 clobbers, void *insns, default: break; } - return len; + return PV_NO_PATCH; } /* CPUID has non-C semantics, and paravirt-ops API doesn't match hardware ISA */ diff --git a/include/asm-i386/paravirt.h b/include/asm-i386/paravirt.h index 9fa3fa9..b26794f 100644 --- a/include/asm-i386/paravirt.h +++ b/include/asm-i386/paravirt.h @@ -252,6 +252,9 @@ extern struct paravirt_ops paravirt_ops; #define paravirt_alt(insn_string) \ _paravirt_alt(insn_string, "%c[paravirt_typenum]", "%c[paravirt_clobber]") +enum { + PV_NO_PATCH = -1 +}; unsigned paravirt_patch_nop(void); unsigned paravirt_patch_ignore(unsigned len); unsigned paravirt_patch_call(void *insnbuf, - To unsubscribe from this list: send the line "unsubscribe
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
On Fri, 2007-08-17 at 17:05 -0700, Jeremy Fitzhardinge wrote: > Andi Kleen wrote: > > Commit 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177 "x86: Fix alternatives > > and kprobes to remap write-protected kernel text" uses code which is > > being patched for patching. > > > > In particular, paravirt_ops does patching in two stages: first it > > calls paravirt_ops.patch, then it fills any remaining instructions > > with nop_out(). nop_out calls text_poke() which calls > > lookup_address() which calls pgd_val() (aka paravirt_ops.pgd_val): > > that call site is one of the places we patch. > > > > If we always do patching as one single call to text_poke(), we only > > need make sure we're not patching the memcpy in text_poke itself. > > This means the prototype to paravirt_ops.patch needs to change, to > > marshal the new code into a buffer rather than patching in place as it > > does now. It also means all patching goes through text_poke(), which > > is known to be safe (apply_alternatives is also changed to make a > > single patch). > > > > Hi Andi, > > This patch breaks Xen booting. I get infinite recursive faults during > patching when this patch is present. If I boot with > "noreplace-paravirt" it works OK, and it works as expected if I back > this patch out. I haven't tracked down the exact failure mode; its a > little hard to debug because it overwrites all kernel memory with > recursive fault stackframes and then finally traps out to Xen when it > hits the bottom of memory. > > I think we should back this one out before .23. Then back out 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177 too, which broke lguest booting, and this tried to fix. Rusty. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
On Fri, 2007-08-17 at 17:05 -0700, Jeremy Fitzhardinge wrote: Andi Kleen wrote: Commit 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177 x86: Fix alternatives and kprobes to remap write-protected kernel text uses code which is being patched for patching. In particular, paravirt_ops does patching in two stages: first it calls paravirt_ops.patch, then it fills any remaining instructions with nop_out(). nop_out calls text_poke() which calls lookup_address() which calls pgd_val() (aka paravirt_ops.pgd_val): that call site is one of the places we patch. If we always do patching as one single call to text_poke(), we only need make sure we're not patching the memcpy in text_poke itself. This means the prototype to paravirt_ops.patch needs to change, to marshal the new code into a buffer rather than patching in place as it does now. It also means all patching goes through text_poke(), which is known to be safe (apply_alternatives is also changed to make a single patch). Hi Andi, This patch breaks Xen booting. I get infinite recursive faults during patching when this patch is present. If I boot with noreplace-paravirt it works OK, and it works as expected if I back this patch out. I haven't tracked down the exact failure mode; its a little hard to debug because it overwrites all kernel memory with recursive fault stackframes and then finally traps out to Xen when it hits the bottom of memory. I think we should back this one out before .23. Then back out 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177 too, which broke lguest booting, and this tried to fix. Rusty. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
* Rusty Russell ([EMAIL PROTECTED]) wrote: Then back out 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177 too, which broke lguest booting, and this tried to fix. That did get backed out (at least the part that broke paravirt patching) in 602033ed5907a59ce86f709082a35be047743a86. Linus' tree should be working fine right now with d34fda4a84c18402640a1a2342d6e6d9829e6db7 committed, and can be further refined with the patch below that's just waiting on some further testing. thanks, -chris -- Subject: [PATCH] x86: skip paravirt patching when appropriate From: Chris Wright [EMAIL PROTECTED] commit d34fda4a84c18402640a1a2342d6e6d9829e6db7 was a little overkill in the case where a paravirt patcher chooses to leave patch site unpatched. Instead of copying original instructions to temp buffer then back to patch site, simply skip patching those sites altogether. Cc: Jeremy Fitzhardinge [EMAIL PROTECTED] Cc: Rusty Russell [EMAIL PROTECTED] Cc: Zach Amsden [EMAIL PROTECTED] Signed-off-by: Chris Wright [EMAIL PROTECTED] --- arch/i386/kernel/alternative.c |4 ++-- arch/i386/kernel/paravirt.c| 10 +- arch/i386/kernel/vmi.c |4 ++-- include/asm-i386/paravirt.h|3 +++ 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/arch/i386/kernel/alternative.c b/arch/i386/kernel/alternative.c index 9f4ac8b..b81d87e 100644 --- a/arch/i386/kernel/alternative.c +++ b/arch/i386/kernel/alternative.c @@ -366,10 +366,10 @@ void apply_paravirt(struct paravirt_patch_site *start, unsigned int used; BUG_ON(p-len MAX_PATCH_LEN); - /* prep the buffer with the original instructions */ - memcpy(insnbuf, p-instr, p-len); used = paravirt_ops.patch(p-instrtype, p-clobbers, insnbuf, (unsigned long)p-instr, p-len); + if (used == PV_NO_PATCH) + continue; BUG_ON(used p-len); diff --git a/arch/i386/kernel/paravirt.c b/arch/i386/kernel/paravirt.c index 739cfb2..a36ce34 100644 --- a/arch/i386/kernel/paravirt.c +++ b/arch/i386/kernel/paravirt.c @@ -122,7 +122,7 @@ unsigned paravirt_patch_nop(void) unsigned paravirt_patch_ignore(unsigned len) { - return len; + return PV_NO_PATCH; } struct branch { @@ -139,9 +139,9 @@ unsigned paravirt_patch_call(void *insnbuf, unsigned long delta = (unsigned long)target - (addr+5); if (tgt_clobbers ~site_clobbers) - return len; /* target would clobber too much for this site */ + return PV_NO_PATCH; /* target would clobber too much for this site */ if (len 5) - return len; /* call too long for patch site */ + return PV_NO_PATCH; /* call too long for patch site */ b-opcode = 0xe8; /* call */ b-delta = delta; @@ -157,7 +157,7 @@ unsigned paravirt_patch_jmp(const void *target, void *insnbuf, unsigned long delta = (unsigned long)target - (addr+5); if (len 5) - return len; /* call too long for patch site */ + return PV_NO_PATCH; /* call too long for patch site */ b-opcode = 0xe9; /* jmp */ b-delta = delta; @@ -196,7 +196,7 @@ unsigned paravirt_patch_insns(void *insnbuf, unsigned len, unsigned insn_len = end - start; if (insn_len len || start == NULL) - insn_len = len; + insn_len = PV_NO_PATCH; else memcpy(insnbuf, start, insn_len); diff --git a/arch/i386/kernel/vmi.c b/arch/i386/kernel/vmi.c index 18673e0..27ae004 100644 --- a/arch/i386/kernel/vmi.c +++ b/arch/i386/kernel/vmi.c @@ -118,7 +118,7 @@ static unsigned patch_internal(int call, unsigned len, void *insnbuf, case VMI_RELOCATION_NONE: /* leave native code in place */ - break; + return PV_NO_PATCH; default: BUG(); @@ -153,7 +153,7 @@ static unsigned vmi_patch(u8 type, u16 clobbers, void *insns, default: break; } - return len; + return PV_NO_PATCH; } /* CPUID has non-C semantics, and paravirt-ops API doesn't match hardware ISA */ diff --git a/include/asm-i386/paravirt.h b/include/asm-i386/paravirt.h index 9fa3fa9..b26794f 100644 --- a/include/asm-i386/paravirt.h +++ b/include/asm-i386/paravirt.h @@ -252,6 +252,9 @@ extern struct paravirt_ops paravirt_ops; #define paravirt_alt(insn_string) \ _paravirt_alt(insn_string, %c[paravirt_typenum], %c[paravirt_clobber]) +enum { + PV_NO_PATCH = -1 +}; unsigned paravirt_patch_nop(void); unsigned paravirt_patch_ignore(unsigned len); unsigned paravirt_patch_call(void *insnbuf, - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
Andi Kleen wrote: >> This patch breaks Xen booting. >> > > Check the latest git head. Does it still break? Yes, that's with latest git head. J - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
* Linus Torvalds ([EMAIL PROTECTED]) wrote: > On Sat, 18 Aug 2007, Chris Wright wrote: > > > > > > Check the latest git head. Does it still break? > > > > Yeah, this is the latest git. The broken commit is Rusty's patch which, > > after Linus reverted the write-protected remap changes, is no longer > > necessary. AFAICT patching is writing garbage into the insn stream. > > I suspect it's copying an uninitialized temp buffer. > > Can you send me the revert patch that is verified to work? Now that I understand the problem, I do have a very simple (slightly overkill) fix for paravirt patching. This can be cleaned up to avoid the copies when they aren't needed, but that will take a little more auditing of the various patchers. If you still prefer a revert I've got one handy, and we can re-visit this all post .23. thanks, -chris -- Subject: [PATCH] x86: properly initialize temp insn buffer for paravirt patching From: Chris Wright <[EMAIL PROTECTED]> With commit ab144f5ec64c42218a555ec1dbde6b60cf2982d6 the patching code now collects the complete new instruction stream into a temp buffer before finally patching in the new insns. In some cases the paravirt patchers will choose to leave the patch site unpatched (length mismatch, clobbers mismatch, etc). This causes the new patching code to copy an uninitialized temp buffer, i.e. garbage, to the callsite. Simply make sure to always initialize the buffer with the original instruction stream. A better fix is to audit all the patchers and return proper length so that apply_paravirt() can skip copies when we leave the patch site untouched. Signed-off-by: Chris Wright <[EMAIL PROTECTED]> --- arch/i386/kernel/alternative.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/arch/i386/kernel/alternative.c b/arch/i386/kernel/alternative.c index 1b66d5c..9f4ac8b 100644 --- a/arch/i386/kernel/alternative.c +++ b/arch/i386/kernel/alternative.c @@ -366,6 +366,8 @@ void apply_paravirt(struct paravirt_patch_site *start, unsigned int used; BUG_ON(p->len > MAX_PATCH_LEN); + /* prep the buffer with the original instructions */ + memcpy(insnbuf, p->instr, p->len); used = paravirt_ops.patch(p->instrtype, p->clobbers, insnbuf, (unsigned long)p->instr, p->len); - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
On Sat, 18 Aug 2007, Chris Wright wrote: > > > > Check the latest git head. Does it still break? > > Yeah, this is the latest git. The broken commit is Rusty's patch which, > after Linus reverted the write-protected remap changes, is no longer > necessary. AFAICT patching is writing garbage into the insn stream. > I suspect it's copying an uninitialized temp buffer. Can you send me the revert patch that is verified to work? Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
* Andi Kleen ([EMAIL PROTECTED]) wrote: > > This patch breaks Xen booting. > > Check the latest git head. Does it still break? Yeah, this is the latest git. The broken commit is Rusty's patch which, after Linus reverted the write-protected remap changes, is no longer necessary. AFAICT patching is writing garbage into the insn stream. I suspect it's copying an uninitialized temp buffer. thanks, -chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
> This patch breaks Xen booting. Check the latest git head. Does it still break? -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
This patch breaks Xen booting. Check the latest git head. Does it still break? -Andi - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
* Andi Kleen ([EMAIL PROTECTED]) wrote: This patch breaks Xen booting. Check the latest git head. Does it still break? Yeah, this is the latest git. The broken commit is Rusty's patch which, after Linus reverted the write-protected remap changes, is no longer necessary. AFAICT patching is writing garbage into the insn stream. I suspect it's copying an uninitialized temp buffer. thanks, -chris - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
On Sat, 18 Aug 2007, Chris Wright wrote: Check the latest git head. Does it still break? Yeah, this is the latest git. The broken commit is Rusty's patch which, after Linus reverted the write-protected remap changes, is no longer necessary. AFAICT patching is writing garbage into the insn stream. I suspect it's copying an uninitialized temp buffer. Can you send me the revert patch that is verified to work? Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
* Linus Torvalds ([EMAIL PROTECTED]) wrote: On Sat, 18 Aug 2007, Chris Wright wrote: Check the latest git head. Does it still break? Yeah, this is the latest git. The broken commit is Rusty's patch which, after Linus reverted the write-protected remap changes, is no longer necessary. AFAICT patching is writing garbage into the insn stream. I suspect it's copying an uninitialized temp buffer. Can you send me the revert patch that is verified to work? Now that I understand the problem, I do have a very simple (slightly overkill) fix for paravirt patching. This can be cleaned up to avoid the copies when they aren't needed, but that will take a little more auditing of the various patchers. If you still prefer a revert I've got one handy, and we can re-visit this all post .23. thanks, -chris -- Subject: [PATCH] x86: properly initialize temp insn buffer for paravirt patching From: Chris Wright [EMAIL PROTECTED] With commit ab144f5ec64c42218a555ec1dbde6b60cf2982d6 the patching code now collects the complete new instruction stream into a temp buffer before finally patching in the new insns. In some cases the paravirt patchers will choose to leave the patch site unpatched (length mismatch, clobbers mismatch, etc). This causes the new patching code to copy an uninitialized temp buffer, i.e. garbage, to the callsite. Simply make sure to always initialize the buffer with the original instruction stream. A better fix is to audit all the patchers and return proper length so that apply_paravirt() can skip copies when we leave the patch site untouched. Signed-off-by: Chris Wright [EMAIL PROTECTED] --- arch/i386/kernel/alternative.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/arch/i386/kernel/alternative.c b/arch/i386/kernel/alternative.c index 1b66d5c..9f4ac8b 100644 --- a/arch/i386/kernel/alternative.c +++ b/arch/i386/kernel/alternative.c @@ -366,6 +366,8 @@ void apply_paravirt(struct paravirt_patch_site *start, unsigned int used; BUG_ON(p-len MAX_PATCH_LEN); + /* prep the buffer with the original instructions */ + memcpy(insnbuf, p-instr, p-len); used = paravirt_ops.patch(p-instrtype, p-clobbers, insnbuf, (unsigned long)p-instr, p-len); - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
Andi Kleen wrote: This patch breaks Xen booting. Check the latest git head. Does it still break? Yes, that's with latest git head. J - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
* Jeremy Fitzhardinge ([EMAIL PROTECTED]) wrote: > This patch breaks Xen booting. I get infinite recursive faults during > patching when this patch is present. If I boot with > "noreplace-paravirt" it works OK, and it works as expected if I back > this patch out. I haven't tracked down the exact failure mode; its a > little hard to debug because it overwrites all kernel memory with > recursive fault stackframes and then finally traps out to Xen when it > hits the bottom of memory. > > I think we should back this one out before .23. I agree (second time this has broken during .23 devel). thanks, -chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
Andi Kleen wrote: > Commit 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177 "x86: Fix alternatives > and kprobes to remap write-protected kernel text" uses code which is > being patched for patching. > > In particular, paravirt_ops does patching in two stages: first it > calls paravirt_ops.patch, then it fills any remaining instructions > with nop_out(). nop_out calls text_poke() which calls > lookup_address() which calls pgd_val() (aka paravirt_ops.pgd_val): > that call site is one of the places we patch. > > If we always do patching as one single call to text_poke(), we only > need make sure we're not patching the memcpy in text_poke itself. > This means the prototype to paravirt_ops.patch needs to change, to > marshal the new code into a buffer rather than patching in place as it > does now. It also means all patching goes through text_poke(), which > is known to be safe (apply_alternatives is also changed to make a > single patch). > Hi Andi, This patch breaks Xen booting. I get infinite recursive faults during patching when this patch is present. If I boot with "noreplace-paravirt" it works OK, and it works as expected if I back this patch out. I haven't tracked down the exact failure mode; its a little hard to debug because it overwrites all kernel memory with recursive fault stackframes and then finally traps out to Xen when it hits the bottom of memory. I think we should back this one out before .23. J - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
Andi Kleen wrote: Commit 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177 x86: Fix alternatives and kprobes to remap write-protected kernel text uses code which is being patched for patching. In particular, paravirt_ops does patching in two stages: first it calls paravirt_ops.patch, then it fills any remaining instructions with nop_out(). nop_out calls text_poke() which calls lookup_address() which calls pgd_val() (aka paravirt_ops.pgd_val): that call site is one of the places we patch. If we always do patching as one single call to text_poke(), we only need make sure we're not patching the memcpy in text_poke itself. This means the prototype to paravirt_ops.patch needs to change, to marshal the new code into a buffer rather than patching in place as it does now. It also means all patching goes through text_poke(), which is known to be safe (apply_alternatives is also changed to make a single patch). Hi Andi, This patch breaks Xen booting. I get infinite recursive faults during patching when this patch is present. If I boot with noreplace-paravirt it works OK, and it works as expected if I back this patch out. I haven't tracked down the exact failure mode; its a little hard to debug because it overwrites all kernel memory with recursive fault stackframes and then finally traps out to Xen when it hits the bottom of memory. I think we should back this one out before .23. J - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
* Jeremy Fitzhardinge ([EMAIL PROTECTED]) wrote: This patch breaks Xen booting. I get infinite recursive faults during patching when this patch is present. If I boot with noreplace-paravirt it works OK, and it works as expected if I back this patch out. I haven't tracked down the exact failure mode; its a little hard to debug because it overwrites all kernel memory with recursive fault stackframes and then finally traps out to Xen when it hits the bottom of memory. I think we should back this one out before .23. I agree (second time this has broken during .23 devel). thanks, -chris - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
Commit 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177 "x86: Fix alternatives and kprobes to remap write-protected kernel text" uses code which is being patched for patching. In particular, paravirt_ops does patching in two stages: first it calls paravirt_ops.patch, then it fills any remaining instructions with nop_out(). nop_out calls text_poke() which calls lookup_address() which calls pgd_val() (aka paravirt_ops.pgd_val): that call site is one of the places we patch. If we always do patching as one single call to text_poke(), we only need make sure we're not patching the memcpy in text_poke itself. This means the prototype to paravirt_ops.patch needs to change, to marshal the new code into a buffer rather than patching in place as it does now. It also means all patching goes through text_poke(), which is known to be safe (apply_alternatives is also changed to make a single patch). AK: fix compilation on x86-64 (bad rusty!) AK: fix boot on x86-64 (sigh) AK: merged with other patches Signed-off-by: Rusty Russell <[EMAIL PROTECTED]> Signed-off-by: Andi Kleen <[EMAIL PROTECTED]> --- arch/i386/kernel/alternative.c | 33 -- arch/i386/kernel/paravirt.c| 52 - arch/i386/kernel/vmi.c | 35 --- arch/i386/xen/enlighten.c | 12 + drivers/lguest/lguest.c|9 +++ include/asm-i386/paravirt.h| 16 +++- 6 files changed, 90 insertions(+), 67 deletions(-) Index: linux/arch/i386/kernel/alternative.c === --- linux.orig/arch/i386/kernel/alternative.c +++ linux/arch/i386/kernel/alternative.c @@ -11,6 +11,8 @@ #include #include +#define MAX_PATCH_LEN (255-1) + #ifdef CONFIG_HOTPLUG_CPU static int smp_alt_once; @@ -148,7 +150,8 @@ static unsigned char** find_nop_table(vo #endif /* CONFIG_X86_64 */ -static void nop_out(void *insns, unsigned int len) +/* Use this to add nops to a buffer, then text_poke the whole buffer. */ +static void add_nops(void *insns, unsigned int len) { unsigned char **noptable = find_nop_table(); @@ -156,7 +159,7 @@ static void nop_out(void *insns, unsigne unsigned int noplen = len; if (noplen > ASM_NOP_MAX) noplen = ASM_NOP_MAX; - text_poke(insns, noptable[noplen], noplen); + memcpy(insns, noptable[noplen], noplen); insns += noplen; len -= noplen; } @@ -174,15 +177,15 @@ extern u8 *__smp_locks[], *__smp_locks_e void apply_alternatives(struct alt_instr *start, struct alt_instr *end) { struct alt_instr *a; - u8 *instr; - int diff; + char insnbuf[MAX_PATCH_LEN]; DPRINTK("%s: alt table %p -> %p\n", __FUNCTION__, start, end); for (a = start; a < end; a++) { + u8 *instr = a->instr; BUG_ON(a->replacementlen > a->instrlen); + BUG_ON(a->instrlen > sizeof(insnbuf)); if (!boot_cpu_has(a->cpuid)) continue; - instr = a->instr; #ifdef CONFIG_X86_64 /* vsyscall code is not mapped yet. resolve it manually. */ if (instr >= (u8 *)VSYSCALL_START && instr < (u8*)VSYSCALL_END) { @@ -191,9 +194,10 @@ void apply_alternatives(struct alt_instr __FUNCTION__, a->instr, instr); } #endif - memcpy(instr, a->replacement, a->replacementlen); - diff = a->instrlen - a->replacementlen; - nop_out(instr + a->replacementlen, diff); + memcpy(insnbuf, a->replacement, a->replacementlen); + add_nops(insnbuf + a->replacementlen, +a->instrlen - a->replacementlen); + text_poke(instr, insnbuf, a->instrlen); } } @@ -215,16 +219,18 @@ static void alternatives_smp_lock(u8 **s static void alternatives_smp_unlock(u8 **start, u8 **end, u8 *text, u8 *text_end) { u8 **ptr; + char insn[1]; if (noreplace_smp) return; + add_nops(insn, 1); for (ptr = start; ptr < end; ptr++) { if (*ptr < text) continue; if (*ptr > text_end) continue; - nop_out(*ptr, 1); + text_poke(*ptr, insn, 1); }; } @@ -351,6 +357,7 @@ void apply_paravirt(struct paravirt_patc struct paravirt_patch_site *end) { struct paravirt_patch_site *p; + char insnbuf[MAX_PATCH_LEN]; if (noreplace_paravirt) return; @@ -358,13 +365,15 @@ void apply_paravirt(struct paravirt_patc for (p = start; p < end; p++) { unsigned int used; - used = paravirt_ops.patch(p->instrtype, p->clobbers, p->instr, -
[PATCH] [5/12] x86_64: Make patching more robust, fix paravirt issue
Commit 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177 x86: Fix alternatives and kprobes to remap write-protected kernel text uses code which is being patched for patching. In particular, paravirt_ops does patching in two stages: first it calls paravirt_ops.patch, then it fills any remaining instructions with nop_out(). nop_out calls text_poke() which calls lookup_address() which calls pgd_val() (aka paravirt_ops.pgd_val): that call site is one of the places we patch. If we always do patching as one single call to text_poke(), we only need make sure we're not patching the memcpy in text_poke itself. This means the prototype to paravirt_ops.patch needs to change, to marshal the new code into a buffer rather than patching in place as it does now. It also means all patching goes through text_poke(), which is known to be safe (apply_alternatives is also changed to make a single patch). AK: fix compilation on x86-64 (bad rusty!) AK: fix boot on x86-64 (sigh) AK: merged with other patches Signed-off-by: Rusty Russell [EMAIL PROTECTED] Signed-off-by: Andi Kleen [EMAIL PROTECTED] --- arch/i386/kernel/alternative.c | 33 -- arch/i386/kernel/paravirt.c| 52 - arch/i386/kernel/vmi.c | 35 --- arch/i386/xen/enlighten.c | 12 + drivers/lguest/lguest.c|9 +++ include/asm-i386/paravirt.h| 16 +++- 6 files changed, 90 insertions(+), 67 deletions(-) Index: linux/arch/i386/kernel/alternative.c === --- linux.orig/arch/i386/kernel/alternative.c +++ linux/arch/i386/kernel/alternative.c @@ -11,6 +11,8 @@ #include asm/mce.h #include asm/nmi.h +#define MAX_PATCH_LEN (255-1) + #ifdef CONFIG_HOTPLUG_CPU static int smp_alt_once; @@ -148,7 +150,8 @@ static unsigned char** find_nop_table(vo #endif /* CONFIG_X86_64 */ -static void nop_out(void *insns, unsigned int len) +/* Use this to add nops to a buffer, then text_poke the whole buffer. */ +static void add_nops(void *insns, unsigned int len) { unsigned char **noptable = find_nop_table(); @@ -156,7 +159,7 @@ static void nop_out(void *insns, unsigne unsigned int noplen = len; if (noplen ASM_NOP_MAX) noplen = ASM_NOP_MAX; - text_poke(insns, noptable[noplen], noplen); + memcpy(insns, noptable[noplen], noplen); insns += noplen; len -= noplen; } @@ -174,15 +177,15 @@ extern u8 *__smp_locks[], *__smp_locks_e void apply_alternatives(struct alt_instr *start, struct alt_instr *end) { struct alt_instr *a; - u8 *instr; - int diff; + char insnbuf[MAX_PATCH_LEN]; DPRINTK(%s: alt table %p - %p\n, __FUNCTION__, start, end); for (a = start; a end; a++) { + u8 *instr = a-instr; BUG_ON(a-replacementlen a-instrlen); + BUG_ON(a-instrlen sizeof(insnbuf)); if (!boot_cpu_has(a-cpuid)) continue; - instr = a-instr; #ifdef CONFIG_X86_64 /* vsyscall code is not mapped yet. resolve it manually. */ if (instr = (u8 *)VSYSCALL_START instr (u8*)VSYSCALL_END) { @@ -191,9 +194,10 @@ void apply_alternatives(struct alt_instr __FUNCTION__, a-instr, instr); } #endif - memcpy(instr, a-replacement, a-replacementlen); - diff = a-instrlen - a-replacementlen; - nop_out(instr + a-replacementlen, diff); + memcpy(insnbuf, a-replacement, a-replacementlen); + add_nops(insnbuf + a-replacementlen, +a-instrlen - a-replacementlen); + text_poke(instr, insnbuf, a-instrlen); } } @@ -215,16 +219,18 @@ static void alternatives_smp_lock(u8 **s static void alternatives_smp_unlock(u8 **start, u8 **end, u8 *text, u8 *text_end) { u8 **ptr; + char insn[1]; if (noreplace_smp) return; + add_nops(insn, 1); for (ptr = start; ptr end; ptr++) { if (*ptr text) continue; if (*ptr text_end) continue; - nop_out(*ptr, 1); + text_poke(*ptr, insn, 1); }; } @@ -351,6 +357,7 @@ void apply_paravirt(struct paravirt_patc struct paravirt_patch_site *end) { struct paravirt_patch_site *p; + char insnbuf[MAX_PATCH_LEN]; if (noreplace_paravirt) return; @@ -358,13 +365,15 @@ void apply_paravirt(struct paravirt_patc for (p = start; p end; p++) { unsigned int used; - used = paravirt_ops.patch(p-instrtype, p-clobbers, p-instr, - p-len); +