* Linus Torvalds <[EMAIL PROTECTED]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
