lkml.org 
[lkml]   [2008]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: 2.6.28-rc6-git1 -- BUG: unable to handle kernel paging request at ffff8800be8b0019
On Thu, 27 Nov 2008, Pekka Enberg wrote:
> On Thu, Nov 27, 2008 at 3:54 PM, Hugh Dickins <hugh@veritas.com> wrote:
> > I think you're looking at a 2.6.28-rc5 sprint_symbol() there:
> > the world has moved on since those days. I changed it to use the
> > supplied "buffer" instead of local "namebuf" in 2.6.28-rc6, so we
> > have to wonder if my patch is to blame - though I don't see it.
>
> Oh, right. I think I see where this is going. The buffer is coming
> from sysfs and is PAGE_SIZE long. In SLUB, we do check for overflows
> but list_locations() allows us to get as close as 100 bytes bytes from
> the edge of the page. Unfortunately, kallsyms_lookup() does:
>
> const char *kallsyms_lookup(unsigned long addr,
> unsigned long *symbolsize,
> unsigned long *offset,
> char **modname, char *namebuf)
> {
> namebuf[KSYM_NAME_LEN - 1] = 0;
> namebuf[0] = 0;
>
> where KSYM_NAME_LEN is 128 which will tip us over the page edge
> potentially triggering an oops.

Thanks for working that out.
Good find (yes, it was a find!) by Miles.

> The proper fix is probably to pass a
> length of namebuf to kallsyms_lookup() but it's probably best to
> revert the patch this late in the release cycle?

Several possibilities, I don't know which is the right one for now,
hope Andrew can judge.

I don't see passing the length of namebuf to kallsyms_lookup()
as called for: it could have been designed that way from the start,
but it wasn't, and I don't see this bug as any reason to change that.

Reverting my patch for now: that's certainly a reasonable possibility,
but leaves us with several other such bugs. Suggested patch below,
but the ftrace part of it worries me a little, since it's within a
structure and maybe it's a bad idea to enlarge that at this point;
I've also not _really_ done the arithmetic needed for the slub one.

An alternative quick just-for-now fix might be to remove that
namebuf[KSYM_NAME_LEN - 1] = 0;
from kallsyms_lookup(): as I understand it (please check), that
could only make sense in cases where the symbol is KSYM_NAME_LEN
long or longer - in which case, all of the places fixed in the
patch below would be causing corruption already, even without my
patch. I think. Maybe that "= 0" even serves no purpose at all?

Hugh

[PATCH] KSYM_SYMBOL_LEN fixes

Miles Lane tailing /sys files hit a BUG which Pekka Enberg has tracked to
my 966c8c12dc9e77f931e2281ba25d2f0244b06949 sprint_symbol(): use less stack
exposing a bug in slub's list_locations() - kallsyms_lookup() writes a 0
to namebuf[KSYM_NAME_LEN-1], but that was beyond the end of page provided.

The 100 slop which list_locations() allows at end of page looks roughly
enough for all the other stuff it might print after the symbol before
it checks again: break out KSYM_SYMBOL_LEN earlier than before.

Latencytop and ftrace and are using KSYM_NAME_LEN buffers where they need
KSYM_SYMBOL_LEN buffers, and vmallocinfo a 2*KSYM_NAME_LEN buffer where
it wants a KSYM_SYMBOL_LEN buffer: fix those before anyone copies them.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

fs/proc/base.c | 2 +-
include/linux/ftrace.h | 2 +-
kernel/latencytop.c | 2 +-
mm/slub.c | 2 +-
mm/vmalloc.c | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)

--- 2.6.28-rc6/fs/proc/base.c 2008-10-24 09:28:19.000000000 +0100
+++ linux/fs/proc/base.c 2008-11-27 16:39:26.000000000 +0000
@@ -371,7 +371,7 @@ static int lstats_show_proc(struct seq_f
task->latency_record[i].time,
task->latency_record[i].max);
for (q = 0; q < LT_BACKTRACEDEPTH; q++) {
- char sym[KSYM_NAME_LEN];
+ char sym[KSYM_SYMBOL_LEN];
char *c;
if (!task->latency_record[i].backtrace[q])
break;
--- 2.6.28-rc6/include/linux/ftrace.h 2008-11-02 23:17:56.000000000 +0000
+++ linux/include/linux/ftrace.h 2008-11-27 16:39:26.000000000 +0000
@@ -231,7 +231,7 @@ ftrace_init_module(unsigned long *start,

struct boot_trace {
pid_t caller;
- char func[KSYM_NAME_LEN];
+ char func[KSYM_SYMBOL_LEN];
int result;
unsigned long long duration; /* usecs */
ktime_t calltime;
--- 2.6.28-rc6/kernel/latencytop.c 2008-07-13 22:51:29.000000000 +0100
+++ linux/kernel/latencytop.c 2008-11-27 16:39:26.000000000 +0000
@@ -191,7 +191,7 @@ static int lstats_show(struct seq_file *
latency_record[i].time,
latency_record[i].max);
for (q = 0; q < LT_BACKTRACEDEPTH; q++) {
- char sym[KSYM_NAME_LEN];
+ char sym[KSYM_SYMBOL_LEN];
char *c;
if (!latency_record[i].backtrace[q])
break;
--- 2.6.28-rc6/mm/slub.c 2008-10-24 09:28:26.000000000 +0100
+++ linux/mm/slub.c 2008-11-27 16:39:27.000000000 +0000
@@ -3595,7 +3595,7 @@ static int list_locations(struct kmem_ca
for (i = 0; i < t.count; i++) {
struct location *l = &t.loc[i];

- if (len > PAGE_SIZE - 100)
+ if (len > PAGE_SIZE - KSYM_SYMBOL_LEN - 100)
break;
len += sprintf(buf + len, "%7ld ", l->count);

--- 2.6.28-rc6/mm/vmalloc.c 2008-11-21 11:09:24.000000000 +0000
+++ linux/mm/vmalloc.c 2008-11-27 16:39:27.000000000 +0000
@@ -1705,7 +1705,7 @@ static int s_show(struct seq_file *m, vo
v->addr, v->addr + v->size, v->size);

if (v->caller) {
- char buff[2 * KSYM_NAME_LEN];
+ char buff[KSYM_SYMBOL_LEN];

seq_putc(m, ' ');
sprint_symbol(buff, (unsigned long)v->caller);

\
 
 \ /
  Last update: 2008-11-27 18:37    [W:0.112 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site