| Message ID | 20130424170702.GA1867@redhat.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 04/24/2013 10:07 AM, Oleg Nesterov wrote: > DR6_RESERVED and DR_CONTROL_RESERVED are used to clear the set > bits in the "unsigned long" data, make them long to ensure that > "&~" doesn't clear the upper bits. > > do_debug() and ptrace_write_dr7() which use DR*_RESERVED look > safe, but probably it makes sense to cleanup anyway. > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Oleg Nesterov <oleg@redhat.com> Can you please put in the description if this is a manifest or non-manifest bug and, if manifest, what the issues are? It greatly affects what we otherwise have to do to address the bug. Also, the -UL suffix is usually capitalized in our codebase. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/4/24 Oleg Nesterov <oleg@redhat.com>: > DR6_RESERVED and DR_CONTROL_RESERVED are used to clear the set > bits in the "unsigned long" data, make them long to ensure that > "&~" doesn't clear the upper bits. > > do_debug() and ptrace_write_dr7() which use DR*_RESERVED look > safe, but probably it makes sense to cleanup anyway. Agreed. The code looks safe, but the pattern is error prone. I'm all for that cleanup. Just something below: > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > --- > arch/x86/include/uapi/asm/debugreg.h | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h > index 3c0874d..75d07dd 100644 > --- a/arch/x86/include/uapi/asm/debugreg.h > +++ b/arch/x86/include/uapi/asm/debugreg.h > @@ -15,7 +15,7 @@ > are either reserved or not of interest to us. */ > > /* Define reserved bits in DR6 which are always set to 1 */ > -#define DR6_RESERVED (0xFFFF0FF0) > +#define DR6_RESERVED (0xFFFF0FF0ul) You told in an earlier email that intel manual says upper 32 bits of dr6 are reserved. In this case don't we need to expand the mask in 64 bits like is done for DR_CONTROL_RESERVED? Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/24/2013 03:48 PM, Frederic Weisbecker wrote: > 2013/4/24 Oleg Nesterov <oleg@redhat.com>: >> DR6_RESERVED and DR_CONTROL_RESERVED are used to clear the set >> bits in the "unsigned long" data, make them long to ensure that >> "&~" doesn't clear the upper bits. >> >> do_debug() and ptrace_write_dr7() which use DR*_RESERVED look >> safe, but probably it makes sense to cleanup anyway. > > Agreed. The code looks safe, but the pattern is error prone. I'm all > for that cleanup. > Just something below: > >> >> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> >> Signed-off-by: Oleg Nesterov <oleg@redhat.com> >> --- >> arch/x86/include/uapi/asm/debugreg.h | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h >> index 3c0874d..75d07dd 100644 >> --- a/arch/x86/include/uapi/asm/debugreg.h >> +++ b/arch/x86/include/uapi/asm/debugreg.h >> @@ -15,7 +15,7 @@ >> are either reserved or not of interest to us. */ >> >> /* Define reserved bits in DR6 which are always set to 1 */ >> -#define DR6_RESERVED (0xFFFF0FF0) >> +#define DR6_RESERVED (0xFFFF0FF0ul) > > You told in an earlier email that intel manual says upper 32 bits of > dr6 are reserved. > In this case don't we need to expand the mask in 64 bits like is done > for DR_CONTROL_RESERVED? > Arguably this would be a *good* use for ~ ... Instead of defining separate bitmasks for 32 and 64 bits have the reciprocal (non-reserved bits): #define DR6_RESERVED (~0x0000F00FUL) That does have the right value on both 32 and 64 bits. The leading zeroes aren't even really needed. Now, DR6 is a bit special in that a bunch of the reserved bits are hardwired to 1, not 0; I don't know offhand if that is true for bits [63:32]. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/4/25 H. Peter Anvin <hpa@zytor.com>: > On 04/24/2013 03:48 PM, Frederic Weisbecker wrote: >> You told in an earlier email that intel manual says upper 32 bits of >> dr6 are reserved. >> In this case don't we need to expand the mask in 64 bits like is done >> for DR_CONTROL_RESERVED? >> > > Arguably this would be a *good* use for ~ ... > > Instead of defining separate bitmasks for 32 and 64 bits have the > reciprocal (non-reserved bits): > > #define DR6_RESERVED (~0x0000F00FUL) > > That does have the right value on both 32 and 64 bits. The leading > zeroes aren't even really needed. Ah, looks better indeed. > > Now, DR6 is a bit special in that a bunch of the reserved bits are > hardwired to 1, not 0; I don't know offhand if that is true for bits > [63:32]. Hmm, good point, could it be a problem given that we clear the reserved dr6 bits on do_trap() and write that 'cleaned up" value back to "tsk->thread.debugreg6"? Probably not if those hardwired reserved bits are set to "1" on dr6 physical write whether those bits are cleared or not in their storage in thread struct before resuming the task? -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/24/2013 04:31 PM, Frederic Weisbecker wrote: >> >> Now, DR6 is a bit special in that a bunch of the reserved bits are >> hardwired to 1, not 0; I don't know offhand if that is true for bits >> [63:32]. > > Hmm, good point, could it be a problem given that we clear the > reserved dr6 bits on do_trap() and write that 'cleaned up" value back > to "tsk->thread.debugreg6"? Probably not if those hardwired reserved > bits are set to "1" on dr6 physical write whether those bits are > cleared or not in their storage in thread struct before resuming the > task? > OK, the SDM states that DR6[63:32] are reserved and must be written as zero (not one). So the quiescent 64-bit value of DR6 is 0x0000_0000_FFFF_0FF0. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/24, H. Peter Anvin wrote: > > On 04/24/2013 10:07 AM, Oleg Nesterov wrote: > > DR6_RESERVED and DR_CONTROL_RESERVED are used to clear the set > > bits in the "unsigned long" data, make them long to ensure that > > "&~" doesn't clear the upper bits. > > > > do_debug() and ptrace_write_dr7() which use DR*_RESERVED look > > safe, but probably it makes sense to cleanup anyway. > > > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com> > > Can you please put in the description if this is a manifest or > non-manifest bug and, if manifest, what the issues are? It greatly > affects what we otherwise have to do to address the bug. Sorry if it was not clear, I tried to explain in the changelog that this is only cleanup. Yes, dr6 should have all zeroes in 32-64 so the usage of DR6_RESERVED is safe. > Also, the -UL suffix is usually capitalized in our codebase. OK, iiuc otherwise you agree with this change. I'll send v2. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h index 3c0874d..75d07dd 100644 --- a/arch/x86/include/uapi/asm/debugreg.h +++ b/arch/x86/include/uapi/asm/debugreg.h @@ -15,7 +15,7 @@ are either reserved or not of interest to us. */ /* Define reserved bits in DR6 which are always set to 1 */ -#define DR6_RESERVED (0xFFFF0FF0) +#define DR6_RESERVED (0xFFFF0FF0ul) #define DR_TRAP0 (0x1) /* db0 */ #define DR_TRAP1 (0x2) /* db1 */ @@ -65,7 +65,7 @@ gdt or the ldt if we want to. I am not sure why this is an advantage */ #ifdef __i386__ -#define DR_CONTROL_RESERVED (0xFC00) /* Reserved by Intel */ +#define DR_CONTROL_RESERVED (0xFC00ul) /* Reserved by Intel */ #else #define DR_CONTROL_RESERVED (0xFFFFFFFF0000FC00UL) /* Reserved */ #endif
DR6_RESERVED and DR_CONTROL_RESERVED are used to clear the set bits in the "unsigned long" data, make them long to ensure that "&~" doesn't clear the upper bits. do_debug() and ptrace_write_dr7() which use DR*_RESERVED look safe, but probably it makes sense to cleanup anyway. Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Oleg Nesterov <oleg@redhat.com> --- arch/x86/include/uapi/asm/debugreg.h | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)