Messages in this thread | | | Date | Sun, 10 Feb 2008 21:19:04 +0100 | From | Ingo Molnar <> | Subject | Re: [git pull] kgdb light, v5 |
| |
* Linus Torvalds <torvalds@linux-foundation.org> 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).
yes. We should allow kgdb to look at just about anything that can be done safely - and we've got all the necessary protections against pagefaults via pagefault_disable().
> So I'd suggest getting rid of the whole "kgdb_{get|set}_mem()" > functions, and just using "probe_kernel_{read|write}()" directly > instead. > > (Not that I necessarily love those names either, but whatever..) > > The TASK_SIZE checks make more sense in kgdb_validate_break_address() > and friends, where it actually does make sense to check that it's > really a *kernel* address. > > But even there, I'm not sure if the right check is to compare against > TASK_SIZE, since kernel and user memory addresses can in theory be > distinct (that's why we have "set_fs()" historically, and while it's > no longer true on x86 and hasn't been in a long time, the kernel > conceptually allows it - see my previous reply about that whole > get_fs/set_fs thing in the definition of probe_kernel_read/write).
hm, is access_ok() safe on all architectures from irq context? That's the cross-arch equivalent of TASK_SIZE checks normally.
> > + if (count == 2 && ((long)mem & 1) == 0) > > + err = probe_kernel_read(tmp, mem, 2); > > + else if (count == 4 && ((long)mem & 3) == 0) > > + err = probe_kernel_read(tmp, mem, 4); > > + else if (count == 8 && ((long)mem & 7) == 0) > > + err = probe_kernel_read(tmp, mem, 8); > > + else > > + err = probe_kernel_read(tmp, mem, count); > > There's absolutely no reason to care about the alignment, since if you > now use "probe_kernel_read()", the sane thing to do is to just do > > err = probe_kernel_read(tmp, mem, count); > if (!err) { > while (count > 0) { > buf = pack_hex_byte(buf, *tmp); > tmp++; > count--; > } > > and you're all done. No?
yes, the full function now looks like this:
int kgdb_mem2hex(char *mem, char *buf, int count) { char *tmp; int err;
/* * We use the upper half of buf as an intermediate buffer for the * raw memory copy. Hex conversion will work against this one. */ tmp = buf + count;
err = probe_kernel_read(tmp, mem, count); if (!err) { while (count > 0) { buf = pack_hex_byte(buf, *tmp); tmp++; count--; }
*buf = 0; }
return err; }
i'll test this a bit.
Ingo
| |