From: Junxiao Bi <jun...@or...> - 2012-08-22 03:00:30
|
If one kernel path is using KM_USER0 slot and is interrupted by the oprofile nmi, then in copy_from_user_nmi(), the KM_USER0 slot will be overwrite and cleared to zero at last, when the control return to the original kernel path, it will access an invalid virtual address and trigger a crash. Cc: Robert Richter <rob...@am...> Cc: Greg KH <gr...@li...> Cc: st...@vg... Signed-off-by: Junxiao Bi <jun...@or...> Hi, Please review this patch. It is for linux-2.6.32.y stable branch not for mainline. Thanks, Junxiao. --- arch/x86/oprofile/backtrace.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c index 829edf0..b50a280 100644 --- a/arch/x86/oprofile/backtrace.c +++ b/arch/x86/oprofile/backtrace.c @@ -71,9 +71,9 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n) offset = addr & (PAGE_SIZE - 1); size = min(PAGE_SIZE - offset, n - len); - map = kmap_atomic(page, KM_USER0); + map = kmap_atomic(page, KM_NMI); memcpy(to, map+offset, size); - kunmap_atomic(map, KM_USER0); + kunmap_atomic(map, KM_NMI); put_page(page); len += size; -- 1.7.9.5 |
From: Robert R. <rob...@am...> - 2012-08-22 07:23:17
|
On 22.08.12 10:21:07, Junxiao Bi wrote: > If one kernel path is using KM_USER0 slot and is interrupted by > the oprofile nmi, then in copy_from_user_nmi(), the KM_USER0 slot > will be overwrite and cleared to zero at last, when the control > return to the original kernel path, it will access an invalid > virtual address and trigger a crash. > > Cc: Robert Richter <rob...@am...> > Cc: Greg KH <gr...@li...> > Cc: st...@vg... > Signed-off-by: Junxiao Bi <jun...@or...> > > Hi, Please review this patch. > > It is for linux-2.6.32.y stable branch not for mainline. I am not sure if there will be any .32 stable release in the future, but this could be at least for .34 or if there is one for .27 and .35. > > Thanks, > Junxiao. > --- > arch/x86/oprofile/backtrace.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) We should implement the perf version here, which does a: int type = in_nmi() ? KM_NMI : KM_IRQ0; See arch/x86/kernel/cpu/perf_event.c. -Robert > > diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c > index 829edf0..b50a280 100644 > --- a/arch/x86/oprofile/backtrace.c > +++ b/arch/x86/oprofile/backtrace.c > @@ -71,9 +71,9 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n) > offset = addr & (PAGE_SIZE - 1); > size = min(PAGE_SIZE - offset, n - len); > > - map = kmap_atomic(page, KM_USER0); > + map = kmap_atomic(page, KM_NMI); > memcpy(to, map+offset, size); > - kunmap_atomic(map, KM_USER0); > + kunmap_atomic(map, KM_NMI); > put_page(page); > > len += size; > -- > 1.7.9.5 > > -- Advanced Micro Devices, Inc. Operating System Research Center |
From: Junxiao Bi <jun...@or...> - 2012-08-22 08:05:25
|
On 08/22/2012 03:07 PM, Robert Richter wrote: > On 22.08.12 10:21:07, Junxiao Bi wrote: >> If one kernel path is using KM_USER0 slot and is interrupted by >> the oprofile nmi, then in copy_from_user_nmi(), the KM_USER0 slot >> will be overwrite and cleared to zero at last, when the control >> return to the original kernel path, it will access an invalid >> virtual address and trigger a crash. >> >> Cc: Robert Richter <rob...@am...> >> Cc: Greg KH <gr...@li...> >> Cc: st...@vg... >> Signed-off-by: Junxiao Bi <jun...@or...> >> >> Hi, Please review this patch. >> >> It is for linux-2.6.32.y stable branch not for mainline. > I am not sure if there will be any .32 stable release in the future, > but this could be at least for .34 or if there is one for .27 and .35. > >> Thanks, >> Junxiao. >> --- >> arch/x86/oprofile/backtrace.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > We should implement the perf version here, which does a: > > int type = in_nmi() ? KM_NMI : KM_IRQ0; > > See arch/x86/kernel/cpu/perf_event.c. KM_NMI seems OK for this since this function is only called by oprofile backtrace which is in nmi context. > -Robert > >> diff --git a/arch/x86/oprofile/backtrace.c b/arch/x86/oprofile/backtrace.c >> index 829edf0..b50a280 100644 >> --- a/arch/x86/oprofile/backtrace.c >> +++ b/arch/x86/oprofile/backtrace.c >> @@ -71,9 +71,9 @@ copy_from_user_nmi(void *to, const void __user *from, unsigned long n) >> offset = addr & (PAGE_SIZE - 1); >> size = min(PAGE_SIZE - offset, n - len); >> >> - map = kmap_atomic(page, KM_USER0); >> + map = kmap_atomic(page, KM_NMI); >> memcpy(to, map+offset, size); >> - kunmap_atomic(map, KM_USER0); >> + kunmap_atomic(map, KM_NMI); >> put_page(page); >> >> len += size; >> -- >> 1.7.9.5 >> >> |
From: Willy T. <w...@1w...> - 2012-08-22 08:12:48
|
On Wed, Aug 22, 2012 at 09:07:38AM +0200, Robert Richter wrote: > On 22.08.12 10:21:07, Junxiao Bi wrote: > > If one kernel path is using KM_USER0 slot and is interrupted by > > the oprofile nmi, then in copy_from_user_nmi(), the KM_USER0 slot > > will be overwrite and cleared to zero at last, when the control > > return to the original kernel path, it will access an invalid > > virtual address and trigger a crash. > > > > Cc: Robert Richter <rob...@am...> > > Cc: Greg KH <gr...@li...> > > Cc: st...@vg... > > Signed-off-by: Junxiao Bi <jun...@or...> > > > > Hi, Please review this patch. > > > > It is for linux-2.6.32.y stable branch not for mainline. > > I am not sure if there will be any .32 stable release in the future, > but this could be at least for .34 or if there is one for .27 and .35. Yes, .32.60 is currently being worked on right now. I'm having difficulties assigning more time to work on it during the summer but it is making progress. sorry for the overly long delays :-( Willy |
From: Junxiao Bi <jun...@or...> - 2012-08-22 07:53:01
|
On 08/22/2012 03:46 PM, Willy Tarreau wrote: > On Wed, Aug 22, 2012 at 10:21:07AM +0800, Junxiao Bi wrote: >> If one kernel path is using KM_USER0 slot and is interrupted by >> the oprofile nmi, then in copy_from_user_nmi(), the KM_USER0 slot >> will be overwrite and cleared to zero at last, when the control >> return to the original kernel path, it will access an invalid >> virtual address and trigger a crash. >> >> Cc: Robert Richter <rob...@am...> >> Cc: Greg KH <gr...@li...> >> Cc: st...@vg... >> Signed-off-by: Junxiao Bi <jun...@or...> >> >> Hi, Please review this patch. >> >> It is for linux-2.6.32.y stable branch not for mainline. > BTW, is there a mainline equivalent commit ID ? A mainline ID is > normally needed to merge anything into stable branches to ensure > that no fix is lost when people upgrade. If this fix is part of > another mainline commit, that's fine too. No, mainline doesn't have this issue. It used another way to implement kmap_atomic, it doesn't need the KM_USER0/KM_NMI0 parameter. > > Thanks, > Willy > |
From: Jonathan N. <jrn...@gm...> - 2012-08-22 07:58:06
|
Junxiao Bi wrote: > No, mainline doesn't have this issue. It used another way to implement > kmap_atomic, it doesn't need the KM_USER0/KM_NMI0 parameter. So, do all kernels >= 2.6.32.y without commit 3e4d3af501cccdc8a8cca41bdbe57d54ad7e7e73 need this? Thanks, Jonathan |
From: Junxiao Bi <jun...@or...> - 2012-08-22 08:10:17
|
On 08/22/2012 03:58 PM, Jonathan Nieder wrote: > Junxiao Bi wrote: > >> No, mainline doesn't have this issue. It used another way to implement >> kmap_atomic, it doesn't need the KM_USER0/KM_NMI0 parameter. > So, do all kernels >= 2.6.32.y without commit > 3e4d3af501cccdc8a8cca41bdbe57d54ad7e7e73 need this? Yes, if commit a0e3e70243f5b270bc3eca718f0a9fa5e6b8262e is backported in the kernel. > > Thanks, > Jonathan |
From: Willy T. <w...@1w...> - 2012-08-22 08:12:41
|
On Wed, Aug 22, 2012 at 10:21:07AM +0800, Junxiao Bi wrote: > If one kernel path is using KM_USER0 slot and is interrupted by > the oprofile nmi, then in copy_from_user_nmi(), the KM_USER0 slot > will be overwrite and cleared to zero at last, when the control > return to the original kernel path, it will access an invalid > virtual address and trigger a crash. > > Cc: Robert Richter <rob...@am...> > Cc: Greg KH <gr...@li...> > Cc: st...@vg... > Signed-off-by: Junxiao Bi <jun...@or...> > > Hi, Please review this patch. > > It is for linux-2.6.32.y stable branch not for mainline. BTW, is there a mainline equivalent commit ID ? A mainline ID is normally needed to merge anything into stable branches to ensure that no fix is lost when people upgrade. If this fix is part of another mainline commit, that's fine too. Thanks, Willy |
From: Robert R. <rob...@am...> - 2012-08-22 07:54:56
|
On 22.08.12 09:46:52, Willy Tarreau wrote: > On Wed, Aug 22, 2012 at 10:21:07AM +0800, Junxiao Bi wrote: > > If one kernel path is using KM_USER0 slot and is interrupted by > > the oprofile nmi, then in copy_from_user_nmi(), the KM_USER0 slot > > will be overwrite and cleared to zero at last, when the control > > return to the original kernel path, it will access an invalid > > virtual address and trigger a crash. > > > > Cc: Robert Richter <rob...@am...> > > Cc: Greg KH <gr...@li...> > > Cc: st...@vg... > > Signed-off-by: Junxiao Bi <jun...@or...> > > > > Hi, Please review this patch. > > > > It is for linux-2.6.32.y stable branch not for mainline. > > BTW, is there a mainline equivalent commit ID ? A mainline ID is > normally needed to merge anything into stable branches to ensure > that no fix is lost when people upgrade. If this fix is part of > another mainline commit, that's fine too. This is a stable-only fix for kernels up to .36 as mainline changed in between. The code for copy_from_user_nmi() should be the same as in arch/x86/kernel/cpu/perf_event.c. -Robert -- Advanced Micro Devices, Inc. Operating System Research Center |