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
