RE: [PATCH] x86/mce: Fix set_mce_nospec() to avoid #GP fault
From: Linus Torvalds > ... > You could literally do something like > > /* Make it canonical in case we flipped the high bit */ > addr = (long)(addr<<1)>>1; Isn't it safer to use a mask and let the compiler decide if two shifts are a good implementation? addr &= ~HIGH_MAGIC_BIT; ISTR fixing a bug in gld where it had (foo << 16) >> 16 instead of foo & 0x. Didn't work well on 64bit. (I may need to recompile that old gcc/gld again. Failed miserably last time I tried.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
RE: [PATCH] x86/mce: Fix set_mce_nospec() to avoid #GP fault
From: Linus Torvalds > ... > You could literally do something like > > /* Make it canonical in case we flipped the high bit */ > addr = (long)(addr<<1)>>1; Isn't it safer to use a mask and let the compiler decide if two shifts are a good implementation? addr &= ~HIGH_MAGIC_BIT; ISTR fixing a bug in gld where it had (foo << 16) >> 16 instead of foo & 0x. Didn't work well on 64bit. (I may need to recompile that old gcc/gld again. Failed miserably last time I tried.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] x86/mce: Fix set_mce_nospec() to avoid #GP fault
On Thu, 30 Aug 2018, Linus Torvalds wrote: > On Thu, Aug 30, 2018 at 6:49 PM Tony Luck wrote: > > > > Just checking "do we have a non-canonical address" at the bottom of that > > call stack and flipping bit 63 back on again seems like a bad idea. > > You could literally do something like > > /* Make it canonical in case we flipped the high bit */ > addr = (long)(addr<<1)>>1; > > in the call to clflush and it magically does the right thing. > > Pretty? No. But with a big comment about what is going on and why it's > done, I think it's prettier than your much bigger patch. > > I dunno. It does strike me as a bit hacky, but I'd rather have a > *small* one-liner hack that generates two instructions, than add a > complex hack that modifies the page tables three times and has a > serializing instruction in it. > > Both are subtle fixes for a subtle issue, but one seems pretty > harmless in comparison. > > Hmm? > > But I'll bow to the x86 maintainers. The above is fugly, but it has the charm of simplicity and I assume it's going to be useful for other places as well. With a big fat comment WHY we are doing it it's not that horrible. We have all the other L1TF places where we fiddle with bits in non-obvious ways, so having another instance of magic bit fiddling is not that big of a problem. Thanks, tglx
Re: [PATCH] x86/mce: Fix set_mce_nospec() to avoid #GP fault
On Thu, 30 Aug 2018, Linus Torvalds wrote: > On Thu, Aug 30, 2018 at 6:49 PM Tony Luck wrote: > > > > Just checking "do we have a non-canonical address" at the bottom of that > > call stack and flipping bit 63 back on again seems like a bad idea. > > You could literally do something like > > /* Make it canonical in case we flipped the high bit */ > addr = (long)(addr<<1)>>1; > > in the call to clflush and it magically does the right thing. > > Pretty? No. But with a big comment about what is going on and why it's > done, I think it's prettier than your much bigger patch. > > I dunno. It does strike me as a bit hacky, but I'd rather have a > *small* one-liner hack that generates two instructions, than add a > complex hack that modifies the page tables three times and has a > serializing instruction in it. > > Both are subtle fixes for a subtle issue, but one seems pretty > harmless in comparison. > > Hmm? > > But I'll bow to the x86 maintainers. The above is fugly, but it has the charm of simplicity and I assume it's going to be useful for other places as well. With a big fat comment WHY we are doing it it's not that horrible. We have all the other L1TF places where we fiddle with bits in non-obvious ways, so having another instance of magic bit fiddling is not that big of a problem. Thanks, tglx
Re: [PATCH] x86/mce: Fix set_mce_nospec() to avoid #GP fault
On Thu, Aug 30, 2018 at 6:49 PM Tony Luck wrote: > > Just checking "do we have a non-canonical address" at the bottom of that > call stack and flipping bit 63 back on again seems like a bad idea. You could literally do something like /* Make it canonical in case we flipped the high bit */ addr = (long)(addr<<1)>>1; in the call to clflush and it magically does the right thing. Pretty? No. But with a big comment about what is going on and why it's done, I think it's prettier than your much bigger patch. I dunno. It does strike me as a bit hacky, but I'd rather have a *small* one-liner hack that generates two instructions, than add a complex hack that modifies the page tables three times and has a serializing instruction in it. Both are subtle fixes for a subtle issue, but one seems pretty harmless in comparison. Hmm? But I'll bow to the x86 maintainers. Linus
Re: [PATCH] x86/mce: Fix set_mce_nospec() to avoid #GP fault
On Thu, Aug 30, 2018 at 6:49 PM Tony Luck wrote: > > Just checking "do we have a non-canonical address" at the bottom of that > call stack and flipping bit 63 back on again seems like a bad idea. You could literally do something like /* Make it canonical in case we flipped the high bit */ addr = (long)(addr<<1)>>1; in the call to clflush and it magically does the right thing. Pretty? No. But with a big comment about what is going on and why it's done, I think it's prettier than your much bigger patch. I dunno. It does strike me as a bit hacky, but I'd rather have a *small* one-liner hack that generates two instructions, than add a complex hack that modifies the page tables three times and has a serializing instruction in it. Both are subtle fixes for a subtle issue, but one seems pretty harmless in comparison. Hmm? But I'll bow to the x86 maintainers. Linus
Re: [PATCH] x86/mce: Fix set_mce_nospec() to avoid #GP fault
On Thu, Aug 30, 2018 at 6:30 PM Linus Torvalds wrote: > > On Thu, Aug 30, 2018 at 2:45 PM Tony Luck wrote: > > > > Fix is to move one step at a time. First mark the page not present > > (using the decoy address). Then it is safe to use the actual address > > of the 1:1 mapping to mark it "uc", and finally as present. > > Can't we do it in one step, but make sure that he clflush gets the real > address? > I'd like to, but I'd need to have a way to mark the address as needing fixing as it gets passed from set_memory_uc() to _set_memory_uc() to change_page_attr_set() to change_page_attr_set_clear() Just checking "do we have a non-canonical address" at the bottom of that call stack and flipping bit 63 back on again seems like a bad idea. But adding extra flag arguments is majorly ugly too. -Tony
Re: [PATCH] x86/mce: Fix set_mce_nospec() to avoid #GP fault
On Thu, Aug 30, 2018 at 6:30 PM Linus Torvalds wrote: > > On Thu, Aug 30, 2018 at 2:45 PM Tony Luck wrote: > > > > Fix is to move one step at a time. First mark the page not present > > (using the decoy address). Then it is safe to use the actual address > > of the 1:1 mapping to mark it "uc", and finally as present. > > Can't we do it in one step, but make sure that he clflush gets the real > address? > I'd like to, but I'd need to have a way to mark the address as needing fixing as it gets passed from set_memory_uc() to _set_memory_uc() to change_page_attr_set() to change_page_attr_set_clear() Just checking "do we have a non-canonical address" at the bottom of that call stack and flipping bit 63 back on again seems like a bad idea. But adding extra flag arguments is majorly ugly too. -Tony
Re: [PATCH] x86/mce: Fix set_mce_nospec() to avoid #GP fault
On Thu, Aug 30, 2018 at 2:45 PM Tony Luck wrote: > > Fix is to move one step at a time. First mark the page not present > (using the decoy address). Then it is safe to use the actual address > of the 1:1 mapping to mark it "uc", and finally as present. Can't we do it in one step, but make sure that he clflush gets the real address? Linus
Re: [PATCH] x86/mce: Fix set_mce_nospec() to avoid #GP fault
On Thu, Aug 30, 2018 at 2:45 PM Tony Luck wrote: > > Fix is to move one step at a time. First mark the page not present > (using the decoy address). Then it is safe to use the actual address > of the 1:1 mapping to mark it "uc", and finally as present. Can't we do it in one step, but make sure that he clflush gets the real address? Linus
