Re: [git pull] kgdb light, v5

[Ingo Molnar]

* 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).

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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/