Re: [git pull] kgdb light, v5
- From: Ingo Molnar <mingo@xxxxxxx>
- Date: Sun, 10 Feb 2008 21:29:30 +0100
* Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
+static int kgdb_get_mem(char *addr, unsigned char *buf, int count)
{
+ if ((unsigned long)addr < TASK_SIZE)
+ return -EFAULT;
+ return probe_kernel_read(buf, addr, count);
}
Ok, so this is a pretty function after all the cleanups, but I
actually don't think that "if ((unsigned long)addr < TASK_SIZE)" is
really even asked for.
Why not let kgdb look at user memory? I'd argue that in a lot of
cases, it might be quite nice to do, to see what user arguments in
memory are etc etc (think things like futexes, where user memory
contents really do matter).
So I'd suggest getting rid of the whole "kgdb_{get|set}_mem()"
functions, and just using "probe_kernel_{read|write}()" directly
instead.
ok, on a second thought: kgdb_{get|set}_mem() is _only_ used to validate
and set the software breakpoint (int3). And i think kgdb correctly
restricts that to kernel-space addresses only - you can typo an address
down into user-space and overwrite user-space memory and not know what
hit you ... [you can still explicitly touch user-space memory, but that
has to be done intentionally]
So to reduce the confusion i've removed these functions and open-coded
the probe_kernel_*() functions into kgdb_validate_break_address() and
kgdb_arch_set_breakpoint().
all other places already use probe_kernel_{read|write}. (Now, there are
a few stray TASK_SIZE checks still, i'll double check them and convert
them to access_ok() checks.)
btw., based on your previous comment about alignment, i found another
function that used weird alignment checks, kgdb_hex2mem():
if (count == 2 && ((long)mem & 1) == 0)
err = probe_kernel_write(mem, tmp_raw, 2);
else if (count == 4 && ((long)mem & 3) == 0)
err = probe_kernel_write(mem, tmp_raw, 4);
else if (count == 8 && ((long)mem & 7) == 0)
err = probe_kernel_write(mem, tmp_raw, 8);
else
err = probe_kernel_write(mem, tmp_raw, count);
return err;
}
I just converted it to:
return probe_kernel_write(mem, tmp_raw, count);
which looks _a lot_ cleaner.
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- Follow-Ups:
- Re: [git pull] kgdb light, v5
- From: Ingo Molnar
- Re: [git pull] kgdb light, v5
- References:
- [0/6] kgdb light
- From: Ingo Molnar
- Re: [0/6] kgdb light
- From: Sam Ravnborg
- [git pull] kgdb light, v5
- From: Ingo Molnar
- Re: [git pull] kgdb light, v5
- From: Ray Lee
- Re: [git pull] kgdb light, v5
- From: Jan Kiszka
- Re: [git pull] kgdb light, v5
- From: Linus Torvalds
- [0/6] kgdb light
- Prev by Date: Re: [PATCH] Change pci_raw_ops to pci_raw_read/write
- Next by Date: Re: [PATCH][USBATM]: convert heavy init dances to kthread API
- Previous by thread: Re: [git pull] kgdb light, v5
- Next by thread: Re: [git pull] kgdb light, v5
- Index(es):
Relevant Pages
|
|
