Re: [git pull] kgdb light, v5

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2658 bytes --]

Linus Torvalds wrote:
> 
> 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.

Makes indeed more sense.

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

Maybe, maybe not. I followed the comment in the original code, saying 
that we need word-wise access for I/O memory poking. Can I assume across 
a all archs that __copy_to/from_user will not perform byte accesses if 
count is 2, 4, or 8? I would be glad if we can kill other couple of line.

Ingo, if you are close to an editor, please pick those up? Here are some 
offline things cooking on my side...

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]