* Silence compiler warning in arch/x86/kvm/emulate.c @ 2015-08-29 21:49 Valdis Kletnieks 2016-02-19 11:11 ` Aurelien Jarno 0 siblings, 1 reply; 8+ messages in thread From: Valdis Kletnieks @ 2015-08-29 21:49 UTC (permalink / raw) To: Gleb Natapov, Paolo Bonzini; +Cc: x86, kvm, linux-kernel, Aruna Hewapathirane Compiler warning: CC [M] arch/x86/kvm/emulate.o arch/x86/kvm/emulate.c: In function "__do_insn_fetch_bytes": arch/x86/kvm/emulate.c:814:9: warning: "linear" may be used uninitialized in this function [-Wmaybe-uninitialized] GCC is smart enough to realize that the inlined __linearize may return before setting the value of linear, but not smart enough to realize the same X86EMU_CONTINUE blocks actual use of the value. However, the value of 'linear' can only be set to one value, so hoisting the one line of code upwards makes GCC happy with the code. Reported-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> Tested-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> --- a/arch/x86/kvm/emulate.c.dist 2015-08-11 14:10:05.366061993 -0400 +++ b/arch/x86/kvm/emulate.c 2015-08-29 13:43:13.014163958 -0400 @@ -650,6 +650,7 @@ static __always_inline int __linearize(s u16 sel; la = seg_base(ctxt, addr.seg) + addr.ea; + *linear = la; *max_size = 0; switch (mode) { case X86EMUL_MODE_PROT64: @@ -693,7 +694,6 @@ static __always_inline int __linearize(s } if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0)) return emulate_gp(ctxt, 0); - *linear = la; return X86EMUL_CONTINUE; bad: if (addr.seg == VCPU_SREG_SS) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Silence compiler warning in arch/x86/kvm/emulate.c 2015-08-29 21:49 Silence compiler warning in arch/x86/kvm/emulate.c Valdis Kletnieks @ 2016-02-19 11:11 ` Aurelien Jarno 2016-02-19 12:05 ` Paolo Bonzini 2016-02-19 16:45 ` Aurelien Jarno 0 siblings, 2 replies; 8+ messages in thread From: Aurelien Jarno @ 2016-02-19 11:11 UTC (permalink / raw) To: Valdis Kletnieks Cc: Gleb Natapov, Paolo Bonzini, x86, kvm, linux-kernel, Aruna Hewapathirane On 2015-08-29 17:49, Valdis Kletnieks wrote: > Compiler warning: > > CC [M] arch/x86/kvm/emulate.o > arch/x86/kvm/emulate.c: In function "__do_insn_fetch_bytes": > arch/x86/kvm/emulate.c:814:9: warning: "linear" may be used uninitialized in this function [-Wmaybe-uninitialized] > > GCC is smart enough to realize that the inlined __linearize may return before > setting the value of linear, but not smart enough to realize the same > X86EMU_CONTINUE blocks actual use of the value. However, the value of > 'linear' can only be set to one value, so hoisting the one line of code > upwards makes GCC happy with the code. > > Reported-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> > Tested-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> > Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> > > --- a/arch/x86/kvm/emulate.c.dist 2015-08-11 14:10:05.366061993 -0400 > +++ b/arch/x86/kvm/emulate.c 2015-08-29 13:43:13.014163958 -0400 > @@ -650,6 +650,7 @@ static __always_inline int __linearize(s > u16 sel; > > la = seg_base(ctxt, addr.seg) + addr.ea; > + *linear = la; > *max_size = 0; > switch (mode) { > case X86EMUL_MODE_PROT64: > @@ -693,7 +694,6 @@ static __always_inline int __linearize(s > } > if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0)) > return emulate_gp(ctxt, 0); > - *linear = la; > return X86EMUL_CONTINUE; > bad: > if (addr.seg == VCPU_SREG_SS) > Unfortunately this patch broke GNU/Hurd when running under KVM. It fails to boot almost immediately. I haven't debug it more, but it looks like *linear should not always be written. This can easily be reproduced by trying to boot Debian Installer from this ISO: http://ftp.debian-ports.org/debian-cd/hurd-i386/debian-hurd-2015/debian-hurd-2015-i386-CD-1.iso Aurelien -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Silence compiler warning in arch/x86/kvm/emulate.c 2016-02-19 11:11 ` Aurelien Jarno @ 2016-02-19 12:05 ` Paolo Bonzini 2016-02-19 17:04 ` Aurelien Jarno 2016-02-19 16:45 ` Aurelien Jarno 1 sibling, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2016-02-19 12:05 UTC (permalink / raw) To: Valdis Kletnieks, Gleb Natapov, x86, kvm, linux-kernel, Aruna Hewapathirane On 19/02/2016 12:11, Aurelien Jarno wrote: > On 2015-08-29 17:49, Valdis Kletnieks wrote: >> Compiler warning: >> >> CC [M] arch/x86/kvm/emulate.o >> arch/x86/kvm/emulate.c: In function "__do_insn_fetch_bytes": >> arch/x86/kvm/emulate.c:814:9: warning: "linear" may be used uninitialized in this function [-Wmaybe-uninitialized] >> >> GCC is smart enough to realize that the inlined __linearize may return before >> setting the value of linear, but not smart enough to realize the same >> X86EMU_CONTINUE blocks actual use of the value. However, the value of >> 'linear' can only be set to one value, so hoisting the one line of code >> upwards makes GCC happy with the code. >> >> Reported-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> >> Tested-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> >> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> >> >> --- a/arch/x86/kvm/emulate.c.dist 2015-08-11 14:10:05.366061993 -0400 >> +++ b/arch/x86/kvm/emulate.c 2015-08-29 13:43:13.014163958 -0400 >> @@ -650,6 +650,7 @@ static __always_inline int __linearize(s >> u16 sel; >> >> la = seg_base(ctxt, addr.seg) + addr.ea; >> + *linear = la; >> *max_size = 0; >> switch (mode) { >> case X86EMUL_MODE_PROT64: >> @@ -693,7 +694,6 @@ static __always_inline int __linearize(s >> } >> if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0)) >> return emulate_gp(ctxt, 0); >> - *linear = la; >> return X86EMUL_CONTINUE; >> bad: >> if (addr.seg == VCPU_SREG_SS) >> > > Unfortunately this patch broke GNU/Hurd when running under KVM. It fails > to boot almost immediately. I haven't debug it more, but it looks like > *linear should not always be written. This can easily be reproduced by > trying to boot Debian Installer from this ISO: > > http://ftp.debian-ports.org/debian-cd/hurd-i386/debian-hurd-2015/debian-hurd-2015-i386-CD-1.iso The bug is that la can be changed by the "la &= (u32)-1" line. So the fix could be like: diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index 1505587d06e9..b9b09fec173b 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -650,10 +650,10 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, u16 sel; la = seg_base(ctxt, addr.seg) + addr.ea; - *linear = la; *max_size = 0; switch (mode) { case X86EMUL_MODE_PROT64: + *linear = la; if (is_noncanonical_address(la)) goto bad; @@ -662,6 +662,7 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, goto bad; break; default: + *linear = la = (u32)la; usable = ctxt->ops->get_segment(ctxt, &sel, &desc, NULL, addr.seg); if (!usable) @@ -689,7 +690,6 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, if (size > *max_size) goto bad; } - la &= (u32)-1; break; } if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0)) Can you test it? Paolo ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Silence compiler warning in arch/x86/kvm/emulate.c 2016-02-19 12:05 ` Paolo Bonzini @ 2016-02-19 17:04 ` Aurelien Jarno 0 siblings, 0 replies; 8+ messages in thread From: Aurelien Jarno @ 2016-02-19 17:04 UTC (permalink / raw) To: Paolo Bonzini Cc: Valdis Kletnieks, Gleb Natapov, x86, kvm, linux-kernel, Aruna Hewapathirane On 2016-02-19 13:05, Paolo Bonzini wrote: > > > On 19/02/2016 12:11, Aurelien Jarno wrote: > > On 2015-08-29 17:49, Valdis Kletnieks wrote: > >> Compiler warning: > >> > >> CC [M] arch/x86/kvm/emulate.o > >> arch/x86/kvm/emulate.c: In function "__do_insn_fetch_bytes": > >> arch/x86/kvm/emulate.c:814:9: warning: "linear" may be used uninitialized in this function [-Wmaybe-uninitialized] > >> > >> GCC is smart enough to realize that the inlined __linearize may return before > >> setting the value of linear, but not smart enough to realize the same > >> X86EMU_CONTINUE blocks actual use of the value. However, the value of > >> 'linear' can only be set to one value, so hoisting the one line of code > >> upwards makes GCC happy with the code. > >> > >> Reported-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> > >> Tested-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> > >> Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> > >> > >> --- a/arch/x86/kvm/emulate.c.dist 2015-08-11 14:10:05.366061993 -0400 > >> +++ b/arch/x86/kvm/emulate.c 2015-08-29 13:43:13.014163958 -0400 > >> @@ -650,6 +650,7 @@ static __always_inline int __linearize(s > >> u16 sel; > >> > >> la = seg_base(ctxt, addr.seg) + addr.ea; > >> + *linear = la; > >> *max_size = 0; > >> switch (mode) { > >> case X86EMUL_MODE_PROT64: > >> @@ -693,7 +694,6 @@ static __always_inline int __linearize(s > >> } > >> if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0)) > >> return emulate_gp(ctxt, 0); > >> - *linear = la; > >> return X86EMUL_CONTINUE; > >> bad: > >> if (addr.seg == VCPU_SREG_SS) > >> > > > > Unfortunately this patch broke GNU/Hurd when running under KVM. It fails > > to boot almost immediately. I haven't debug it more, but it looks like > > *linear should not always be written. This can easily be reproduced by > > trying to boot Debian Installer from this ISO: > > > > http://ftp.debian-ports.org/debian-cd/hurd-i386/debian-hurd-2015/debian-hurd-2015-i386-CD-1.iso > > The bug is that la can be changed by the "la &= (u32)-1" line. > > So the fix could be like: > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index 1505587d06e9..b9b09fec173b 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -650,10 +650,10 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, > u16 sel; > > la = seg_base(ctxt, addr.seg) + addr.ea; > - *linear = la; > *max_size = 0; > switch (mode) { > case X86EMUL_MODE_PROT64: > + *linear = la; > if (is_noncanonical_address(la)) > goto bad; > > @@ -662,6 +662,7 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, > goto bad; > break; > default: > + *linear = la = (u32)la; > usable = ctxt->ops->get_segment(ctxt, &sel, &desc, NULL, > addr.seg); > if (!usable) > @@ -689,7 +690,6 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, > if (size > *max_size) > goto bad; > } > - la &= (u32)-1; > break; > } > if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0)) > > > Can you test it? Sorry about the other mail, I missed this one. I have just tested you patch above, and I confirm it works fine. Thanks. Tested-by: Aurelien Jarno <aurelien@aurel32.net> -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Silence compiler warning in arch/x86/kvm/emulate.c 2016-02-19 11:11 ` Aurelien Jarno 2016-02-19 12:05 ` Paolo Bonzini @ 2016-02-19 16:45 ` Aurelien Jarno 2016-02-19 17:54 ` Valdis.Kletnieks 1 sibling, 1 reply; 8+ messages in thread From: Aurelien Jarno @ 2016-02-19 16:45 UTC (permalink / raw) To: Valdis Kletnieks, Gleb Natapov, Paolo Bonzini, x86, kvm, linux-kernel, Aruna Hewapathirane On 2016-02-19 12:11, Aurelien Jarno wrote: > On 2015-08-29 17:49, Valdis Kletnieks wrote: > > Compiler warning: > > > > CC [M] arch/x86/kvm/emulate.o > > arch/x86/kvm/emulate.c: In function "__do_insn_fetch_bytes": > > arch/x86/kvm/emulate.c:814:9: warning: "linear" may be used uninitialized in this function [-Wmaybe-uninitialized] > > > > GCC is smart enough to realize that the inlined __linearize may return before > > setting the value of linear, but not smart enough to realize the same > > X86EMU_CONTINUE blocks actual use of the value. However, the value of > > 'linear' can only be set to one value, so hoisting the one line of code > > upwards makes GCC happy with the code. > > > > Reported-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> > > Tested-by: Aruna Hewapathirane <aruna.hewapathirane@gmail.com> > > Signed-off-by: Valdis Kletnieks <valdis.kletnieks@vt.edu> > > > > --- a/arch/x86/kvm/emulate.c.dist 2015-08-11 14:10:05.366061993 -0400 > > +++ b/arch/x86/kvm/emulate.c 2015-08-29 13:43:13.014163958 -0400 > > @@ -650,6 +650,7 @@ static __always_inline int __linearize(s > > u16 sel; > > > > la = seg_base(ctxt, addr.seg) + addr.ea; > > + *linear = la; > > *max_size = 0; > > switch (mode) { > > case X86EMUL_MODE_PROT64: > > @@ -693,7 +694,6 @@ static __always_inline int __linearize(s > > } > > if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0)) > > return emulate_gp(ctxt, 0); > > - *linear = la; > > return X86EMUL_CONTINUE; > > bad: > > if (addr.seg == VCPU_SREG_SS) > > > > Unfortunately this patch broke GNU/Hurd when running under KVM. It fails > to boot almost immediately. I haven't debug it more, but it looks like > *linear should not always be written. Actually the same patch with a bit more context shows the issue: > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > index e7a4fde..b372a75 100644 > --- a/arch/x86/kvm/emulate.c > +++ b/arch/x86/kvm/emulate.c > @@ -647,12 +647,13 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, > bool usable; > ulong la; > u32 lim; > u16 sel; > > la = seg_base(ctxt, addr.seg) + addr.ea; > + *linear = la; The assignation is moved here... > *max_size = 0; > switch (mode) { > case X86EMUL_MODE_PROT64: > if (is_noncanonical_address(la)) > goto bad; > > @@ -690,13 +691,12 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, > } > la &= (u32)-1; ... while the value of la might be modified in between. > break; > } > if (insn_aligned(ctxt, size) && ((la & (size - 1)) != 0)) > return emulate_gp(ctxt, 0); > - *linear = la; > return X86EMUL_CONTINUE; > bad: > if (addr.seg == VCPU_SREG_SS) > return emulate_ss(ctxt, 0); > else > return emulate_gp(ctxt, 0); One possibility would be to assign it both at the beginning of the function and at the original location should fix the bug and prevent GCC to issue a warning. Aurelien -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Silence compiler warning in arch/x86/kvm/emulate.c 2016-02-19 16:45 ` Aurelien Jarno @ 2016-02-19 17:54 ` Valdis.Kletnieks 2016-02-19 17:56 ` Paolo Bonzini 0 siblings, 1 reply; 8+ messages in thread From: Valdis.Kletnieks @ 2016-02-19 17:54 UTC (permalink / raw) To: Aurelien Jarno Cc: Gleb Natapov, Paolo Bonzini, x86, kvm, linux-kernel, Aruna Hewapathirane [-- Attachment #1: Type: text/plain, Size: 1470 bytes --] On Fri, 19 Feb 2016 17:45:48 +0100, Aurelien Jarno said: > On 2016-02-19 12:11, Aurelien Jarno wrote: > Actually the same patch with a bit more context shows the issue: > > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c > > index e7a4fde..b372a75 100644 > > --- a/arch/x86/kvm/emulate.c > > +++ b/arch/x86/kvm/emulate.c > > @@ -647,12 +647,13 @@ static __always_inline int __linearize(struct x86_emu late_ctxt *ctxt, > > bool usable; > > ulong la; > > u32 lim; > > u16 sel; > > > > la = seg_base(ctxt, addr.seg) + addr.ea; > > + *linear = la; > > The assignation is moved here... > > > *max_size = 0; > > switch (mode) { > > case X86EMUL_MODE_PROT64: > > if (is_noncanonical_address(la)) > > goto bad; > > > > @@ -690,13 +691,12 @@ static __always_inline int __linearize(struct x86_emulate_ctxt *ctxt, > > } > > la &= (u32)-1; > > ... while the value of la might be modified in between. (trying to reconstruct my thought process from 6 months ago. I remember staring at that, and I convinced myself it was still OK to move the assignment.) la can get changed here - but there's 2 cases to consider. If it's in a 32-bit kernel, anding with -1 is a no-op. Now if we're on a 64-bit kernel, the 'and' clears the high 32 bits. But under what conditions is 'la' a 64-bit quantity that has any bits set in the high 32 bits (meaning it's a pointer to something over the 4G line) - but it's still valid to smash those bits? [-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Silence compiler warning in arch/x86/kvm/emulate.c 2016-02-19 17:54 ` Valdis.Kletnieks @ 2016-02-19 17:56 ` Paolo Bonzini 2016-02-20 0:33 ` Valdis.Kletnieks 0 siblings, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2016-02-19 17:56 UTC (permalink / raw) To: Valdis.Kletnieks, Aurelien Jarno Cc: Gleb Natapov, x86, kvm, linux-kernel, Aruna Hewapathirane -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 19/02/2016 18:54, Valdis.Kletnieks@vt.edu wrote: > la can get changed here - but there's 2 cases to consider. If it's > in a 32-bit kernel, anding with -1 is a no-op. > > Now if we're on a 64-bit kernel, the 'and' clears the high 32 > bits. > > But under what conditions is 'la' a 64-bit quantity that has any > bits set in the high 32 bits (meaning it's a pointer to something > over the 4G line) - but it's still valid to smash those bits? That can happen for example if there is a non-zero segment base. Then the linear address wraps at 4G. Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWx1cxAAoJEL/70l94x66DFBcIAIkWZ7KaTyh3gfy5Cur7aukA 8NkEKvxZxO46lQAiKxf5ORTdBCdtZDjUjbxTVJoguITK/nnXuBLQIP/aeDhfHdhz BTYumgH+QV+kmZfn7mwgY5omS05Qx08DmdpQ1jyu1Y1aPVBv6FlsoWcHFrA+oXI2 wtit0OejbPJ9gT4dv1S/etuJvzdINQ/Y4fh/ulkyJJRIw5vvEgW+PN81UNCiSust w1zkljlfoU4he54IWHa0R1Am/uQBmWRuhvzaMZVKdkGlrN/jJo4ObR5DX+qPujPB sGSw2HTh8p4IuLwAJ0PvZZPNag+6vdOv9jxJuFpRNQGZLYY5eHL1IFVHaESv0vQ= =ssTD -----END PGP SIGNATURE----- ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Silence compiler warning in arch/x86/kvm/emulate.c 2016-02-19 17:56 ` Paolo Bonzini @ 2016-02-20 0:33 ` Valdis.Kletnieks 0 siblings, 0 replies; 8+ messages in thread From: Valdis.Kletnieks @ 2016-02-20 0:33 UTC (permalink / raw) To: Paolo Bonzini Cc: Aurelien Jarno, Gleb Natapov, x86, kvm, linux-kernel, Aruna Hewapathirane [-- Attachment #1: Type: text/plain, Size: 1022 bytes --] On Fri, 19 Feb 2016 18:56:05 +0100, Paolo Bonzini said: > On 19/02/2016 18:54, Valdis.Kletnieks@vt.edu wrote: > > But under what conditions is 'la' a 64-bit quantity that has any > > bits set in the high 32 bits (meaning it's a pointer to something > > over the 4G line) - but it's still valid to smash those bits? > > That can happen for example if there is a non-zero segment base. Then > the linear address wraps at 4G. Gaah. Obviously, the concept that software could actually depend on a segment base pointing at the 3G line (or whatever) to wrap around and be used to address memory down in the first gig of RAM was too bizarre for my brain to visualize. :) The IBM S/360 with 24 bit addresses and S/370 with 24 or 31 bit addresses would allow some instructions (most famously MVCL Move Character Long) to start operating at the high end of memory and wrap around to the beginning. The system documentation was pretty clear that although this *worked*, it was probably not what you actually wanted to do.... :) [-- Attachment #2: Type: application/pgp-signature, Size: 848 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-02-20 0:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-29 21:49 Silence compiler warning in arch/x86/kvm/emulate.c Valdis Kletnieks 2016-02-19 11:11 ` Aurelien Jarno 2016-02-19 12:05 ` Paolo Bonzini 2016-02-19 17:04 ` Aurelien Jarno 2016-02-19 16:45 ` Aurelien Jarno 2016-02-19 17:54 ` Valdis.Kletnieks 2016-02-19 17:56 ` Paolo Bonzini 2016-02-20 0:33 ` Valdis.Kletnieks
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).