Re: [git pull] kgdb light, v5

[Linus Torvalds <torvalds@linux-foundation.org>]

On Sun, 10 Feb 2008, Jan Kiszka 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.

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

> +	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?

		Linus