Date: Wed, 20 Sep 2006 01:56:05 +0400 From: Ruslan Ermilov <ru@freebsd.org> To: Maksim Yevmenkin <maksim.yevmenkin@gmail.com> Cc: cvs-src@freebsd.org, Marius Strobl <marius@freebsd.org>, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/dev/kbdmux kbdmux.c Message-ID: <20060919215605.GB44298@rambler-co.ru> In-Reply-To: <bb4a86c70609191415m59a9ef89x91f25e6912339390@mail.gmail.com> References: <200609191303.k8JD3AHl050783@repoman.freebsd.org> <bb4a86c70609190944o2438a4a4vae7b3bb2332522ee@mail.gmail.com> <20060919190645.GA23068@rambler-co.ru> <bb4a86c70609191236j13fe1563w123cb046261ee129@mail.gmail.com> <20060919194819.GA23360@rambler-co.ru> <bb4a86c70609191300t3bea8380qa1898d1a89b6fc27@mail.gmail.com> <20060919203608.GE23360@rambler-co.ru> <bb4a86c70609191415m59a9ef89x91f25e6912339390@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--cmJC7u66zC7hs+87 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 19, 2006 at 02:15:56PM -0700, Maksim Yevmenkin wrote: > >> sparc64 crashes with (intptr_t *) casting (tested locally) > >> > >We need to find *where*, and find a proper fix. >=20 > that will take some time, as ultra5 is very slow at compiling kernel :) >=20 No need to, I know *where*. The problem is indeed with these brain-damaged IOCTLs that are passed as "int" from userland and as "int *" from kernelland. > >not crash !=3D work properly. Can you print the value of argument > >to KDSETLED and verify it's correct? Print it both as > >(int)*(intptr_t *)arg and *(int *)arg, and compare. >=20 > ok, it works from user's point of view, i.e. i press caps, num lock > etc. and the light comes on/off on keyboard(s). >=20 Yes, it works as long as you don't use these IOCTLs from userland (which you typically don't). > >> i tested (int *) casting on sparc64 with SETLED, SETREPEAT and it > >> works fine. i made sure that parameters passed via ioctl() are all > >> correct. i will do the same test on amd64 before committing. > >> > >KDSETREPEAT _does_ use *(int *) in the committed code, as it's > >a normal "_IOW('K', 102, keyboard_repeat_t)" which takes a pointer > >as an argument. >=20 > yes, so? >=20 I just confirmed that it's okay that it uses *(int *)arg for KDSETREPEAT. > >KDSETLED isn't used outside the kernel, so I assume you tested it > >only when it's called from within the kernel? If so, try passing > >it from useland to see the endianness problem; I'm pretty sure it > >will fire. If it does, there are two options: >=20 > ok, i see what you are saying now. i will try to use KDSETLED from=20 > userspace. >=20 I already did, thanks to your vkbd(4): # uname -p sparc64 # ./a < /dev/vkbdctl0 current mode =3D 1 current mode =3D 0 > >- Fix in-kernel callers of KDSETLED to behave like userland. > >- Fix the definition of KDSETLED to be: > > > >#define KDSETLED _IOW('K', 66, int) > > > >and then fix kbdmux.c to use *(int *) when it accesses KDSETLED's > >argument. >=20 > ok >=20 Since it's obvious that it doesn't "work" on sparc64 in all cases, we need to choose > >Similarly for KDSKBSTATE. KDSETRAD should still be accessed > >through (int)*(intptr_t *)arg to stay binary compatible. >=20 > or perhaps as keyboard_repeat_t >=20 KDSETRAD cannot use keyboard_repeat_t. It's an old interface for binary compatibility (FWICT). If we change it we: 1) break the old API and binary compatibility 2) provide two identical IOCTLs what's the point? It's also used (well, not probably in real life) by kbdcontrol.c. =2E/usr.sbin/kbdcontrol/kbdcontrol.c: if (ioctl(0, KDSETRAD, (d= << 5) | r)) > >> also, like i said before, all other keyboard drivers, including > >> sunkbd(4) use (int *) cast and NOT (intptr_t *), and it seems to work > >> just fine. > >> > >I think this problem is unnoticeable because you're talking about > >the KDSETLED here, and it's only used inside the kernel, and > >inside the kernel it's passed as a "pointer to int", while in > >case of userland it would be passed as a 64-bit value on the > >stack (on sparc64), and causing an endianness problem when > >accessed through *(int *)arg. >=20 > from quick search, i can see that SETLED, KBSKBMODE and KDBSKBSTATE > are called from kernel with (caddr_t) &foo, i.e. pointer to int value. >=20 Yes, in all cases. And this doesn't match the IOCTL's prototype and hence userland. > from another quick search in /usr/src/{usr.}{s}bin/ i do not see any > userspace code that uses these ioctl() directly (and, yes, it does not > mean that it does not exists). >=20 > so, i propose >=20 > 1) we change kbd/syscons/etc kernel code back to its default > assumption that KBxxx ioctls defined as _IO(foo, bar) accept 3rd > parameter as a pointer to int. >=20 Yes. > 2) we change >=20 > KDSETLED _IO('K', 66, /* int */) >=20 > to >=20 > KDSETLED _IO('K', 66, /* (int *) */) >=20 > so its "documented". >=20 No, this isn't going to work and is plain incorrect. We need to change it to "_IOW('K', 66, int)" to make it work. That means that instead of ioctl(0, KDSETLED, mode) applications will have to do: ioctl(0, KDSETLED, &mode) Then they will match the kernel. Let me explain (again) how _IO*'s are treated by ioctl(). _IOW('K', 66, int) means: the argument is a pointer to sizeof(int) bytes of user memory. sysctl() will malloc sizeof(int) bytes of memory in the kernel, and copyin() the data from userland. _IO('K', 66 /*, whatever */) means: the argument is passed by a value as an "int". The kernel then "emulates" the pointer by assigning the pointer variable "data" an on-stack address of this argument. The relevant code in sys_generic.c: : caddr_t data, memp; [...] : if (size > 0) { : memp =3D malloc((u_long)size, M_IOCTLOPS, M_WAITOK); : data =3D memp; : } else { : memp =3D NULL; : data =3D (void *)&uap->data; : } : if (com & IOC_IN) { : error =3D copyin(uap->data, data, (u_int)size); : if (error) { : free(memp, M_IOCTLOPS); : return (error); : } :=20 Here, "size" will be 4 for _IOW, and 0 for _IO. In case of _IO, and the passed argument of say "8", and sparc64, the memory "data" points to will be "0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 8". If we access it through *(int *)data, it will be first 8 bytes which are 0. If we access it as (int)*(intptr_t *)data it will be (int)8, i.e., the passed value. > 3) take an "ostrich approach" (put our heads in the sand) to any 3rd > party application that might use SETLED, KBSKBMODE etc. ioctls > directly. >=20 > the above requires minimal code change, which is, imo, is a good thing. >=20 The proper fix would be to fix the kernel to pass an argument by value, like the userland does. But from the practical point of view, it may make sense to change the API and say that these IOCTLs now take a pointer type argument. I'm not sure. If you know of any applications (mine test util not counting :-) that use KDSKBMODE/KDSETLED/KDSKBSTATE, then we should probably fix the kernel callers. Cheers, --=20 Ruslan Ermilov ru@FreeBSD.org FreeBSD committer --cmJC7u66zC7hs+87 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.5 (FreeBSD) iD8DBQFFEGd1qRfpzJluFF4RAuXbAJ4x0X6LBUbvj2YUOxOEJ6b+jSsJQgCgit9B g8FPG+jVf2yj01/7tz1Z2H0= =hFri -----END PGP SIGNATURE----- --cmJC7u66zC7hs+87--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060919215605.GB44298>