Linus Torvalds wrote:
quoted text
>=20
> 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;
>> =20
>> +	return probe_kernel_read(buf, addr, count);
>>  }
>=20
> Ok, so this is a pretty function after all the cleanups, but I actually=
=20
quoted text
> don't think that "if ((unsigned long)addr < TASK_SIZE)" is really even =

quoted text
> asked for.
>=20
> Why not let kgdb look at user memory? I'd argue that in a lot of cases,=
 it=20
quoted text
> might be quite nice to do, to see what user arguments in memory are etc=
=20
quoted text
> etc (think things like futexes, where user memory contents really do=20
> matter).
>=20
> So I'd suggest getting rid of the whole "kgdb_{get|set}_mem()" function=
s,=20
quoted text
> and just using "probe_kernel_{read|write}()" directly instead.

Makes indeed more sense.

quoted text
>=20
> (Not that I necessarily love those names either, but whatever..)
>=20
> The TASK_SIZE checks make more sense in kgdb_validate_break_address() a=
nd=20
quoted text
> friends, where it actually does make sense to check that it's really a =

quoted text
> *kernel* address.
>=20
> But even there, I'm not sure if the right check is to compare against=20
> TASK_SIZE, since kernel and user memory addresses can in theory be=20
> distinct (that's why we have "set_fs()" historically, and while it's no=
=20
quoted text
> longer true on x86 and hasn't been in a long time, the kernel conceptua=
lly=20
quoted text
> allows it - see my previous reply about that whole get_fs/set_fs thing =
in=20
quoted text
> the definition of probe_kernel_read/write).
>=20
>> +	if (count =3D=3D 2 && ((long)mem & 1) =3D=3D 0)
>> +		err =3D probe_kernel_read(tmp, mem, 2);
>> +	else if (count =3D=3D 4 && ((long)mem & 3) =3D=3D 0)
>> +		err =3D probe_kernel_read(tmp, mem, 4);
>> +	else if (count =3D=3D 8 && ((long)mem & 7) =3D=3D 0)
>> +		err =3D probe_kernel_read(tmp, mem, 8);
>> +	else
>> +		err =3D probe_kernel_read(tmp, mem, count);
>=20
> There's absolutely no reason to care about the alignment, since if you =
now=20
quoted text
> use "probe_kernel_read()", the sane thing to do is to just do
>=20
> 	err =3D probe_kernel_read(tmp, mem, count);
> 	if (!err) {
> 		while (count > 0) {
> 			buf =3D pack_hex_byte(buf, *tmp);
> 			tmp++;
> 			count--;
> 	}
>=20
> and you're all done. No?

Maybe, maybe not. I followed the comment in the original code, saying=20
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=20
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